doc: don't use os.Setenv in examples #1548

kolyshkin · 2025-04-14T20:51:57Z

Related: #838

The official documentation https://onsi.github.io/ginkgo/#spec-cleanup-aftereach-and-defercleanup promotes the use of os.Setenv to set per-test-case environment variables. This is dangerous, since the environment is per-process, meaning any other tests running in parallel will have the same environment. Go's testing package solved this, providing Setenv since Go 1.17. If used together with t.Parallel, it errors out.

In Ginkgo, one can use something like:

t := GinkgoT()
t.Setenv(newDir)

It seems that documentation should document this way, instead of promoting os.Setenv use in tests.

The text was updated successfully, but these errors were encountered:

onsi · 2025-04-14T21:19:15Z

actually - this isn't a problem in ginkgo because it implements per-process parallelization. you can actually use os.Setenv and run tests in parallel without issue.

kolyshkin · 2025-04-15T23:37:57Z

actually - this isn't a problem in ginkgo because it implements per-process parallelization. you can actually use os.Setenv and run tests in parallel without issue.

Nice, I did not know that, thanks for the explanation.

Yet, the cleanup in the docs mentioned above is still not necessary? If that is so, should the doc be fixed anyway?

onsi · 2025-04-16T00:02:38Z

hey @kolyshkin - while it's true that one could call GinkgoT().Setenv(newDir) and rely on the implementation of Setenv to perform the clean-up afterwards my main goal in this section of the docs is to teach people about DeferCleanup and to illustrate how easy it is to set up and tear down resources side-by-side in a BeforeEach. os.Setenv is a convenient exemplar for that purpose.

kolyshkin · 2025-04-16T00:23:06Z

actually - this isn't a problem in ginkgo because it implements per-process parallelization. you can actually use os.Setenv and run tests in parallel without issue.

Just to clarify (and thank you for your help in advance) -- is it true that each It is executed in a separate process having distinct environment, so one can use os.Setenv freely and without any need to undo the changes? Is it also the case if they use go test instead of ginkgo to run tests?

hey @kolyshkin - while it's true that one could call GinkgoT().Setenv(newDir) and rely on the implementation of Setenv to perform the clean-up afterwards my main goal in this section of the docs is to teach people about DeferCleanup and to illustrate how easy it is to set up and tear down resources side-by-side in a BeforeEach. os.Setenv is a convenient exemplar for that purpose.

Yes, I do understand that it's just to illustrate things, but given that

one can use t.Setenv to avoid cleanup entirely;
since ginkgo implements per-process parallelization, such cleanup is not at all necessary;

this makes it a subpar example, since:

people who don't know the above points would think this is how it should be done and end up writing some complex setup/cleanup code which could be way simpler;
people who are aware would wonder if such cleanup is really necessary.

Any example is a piece of documentation in itself, and official documentation is often considered a "best practices" document. Also, people tend to overlook details and just follow the examples.

That's why I think using os.Setenv in setup/cleanup examples is "ok but not great".

onsi · 2025-04-16T15:44:28Z

hey @kolyshkin sorry for the confusion. the documentation covers how Ginkgo runs specs in parallel so i recommend reading that.

in brief, by default invoking ginkgo launches one process that runs the test in series. running ginkgo -p spins up multiple worker processes (roughly one per core) and these proceed to handle the specs in parallel. within each worker specs run in series. this ensures the memory space and environment of each worker is isolated but tests within a given worker can adjust that workers environment in ways that could break subsequent tests. so cleanup is necessary to ensure the tests running in series in a given worker don't interfere. you learn more about ginkgo's expectation of spec independence here and if you're interested in the topic i cover various patterns for managing parallel tests (particularly tests that access shared external resources) here

As for the example - t.Setenv was introduced in Go 1.17 a few years after these docs were written. I wouldn't say that GinkgoT().Setenv is the "right way" necessarily. i'd prefer to teach people the underlying tools and abstractions and enable them to use them and build on top of them without worrying too much about implementation details like "t.Setenv will clean up the environment for you."

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: don't use os.Setenv in examples #1548

doc: don't use os.Setenv in examples #1548

doc: don't use os.Setenv in examples #1548

doc: don't use os.Setenv in examples #1548

Comments