Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: testing: add TB.Setenv() #41260

Open
sagikazarmark opened this issue Sep 7, 2020 · 3 comments
Open

proposal: testing: add TB.Setenv() #41260

sagikazarmark opened this issue Sep 7, 2020 · 3 comments
Labels
Projects
Milestone

Comments

@sagikazarmark
Copy link

@sagikazarmark sagikazarmark commented Sep 7, 2020

Tests often verify behavior that rely on environment variables. Unfortunately, people often don't realize that setting environment variables in tests using os.Setenv will set those variables for the entire lifetime of the process. Things get even more complicated when tests are executed in parallel.

As a result, tests either fail when they shouldn't or worse: don't fail when they should.

In the first scenario (failing test), debugging the test probably leads people to the above conclusion, resulting in a code like this:

func TestSomething(t *testing.T) {
	os.Setenv("KEY", "VALUE")
	defer os.Unsetenv("ENV")
}

It's not necessarily bad, since it's explicit, but it doesn't handle errors and with multiple env vars it would be a lot of unnecessary copy-pasting of boilerplate code.

In the second scenario, it's quite easy to introduce and leave bugs in the code (I've just fixed one today).

I propose adding a Setenv method to testing.TB, *testing.T, and *testing.B something like this:

// Setenv sets an environment variable for the lifetime of the test.
// It also disables running this test in parallel with other tests.
// The environment variable is automatically cleaned up when the test exits.
func (t *T) Setenv(key string, value string) {
	if t.isParallel {
		panic("Setenv: test cannot be run in parallel with others")
	}
	
	t.disableParallel = true

	err := os.Setenv(key, value)
	if err != nil {
		t.Fatalf("Setenv: %v", err)
	}
	
	t.Cleanup(func() {
		os.Unsetenv(key)
	})
}
@mdlayher
Copy link
Member

@mdlayher mdlayher commented Sep 7, 2020

IMHO fetching data from environment variables should only ever be done in package main, and then plumbed throughout the rest of your program using regular Go data types. I don't think this would encourage good code hygiene since environment variables are effectively globals.

@ianlancetaylor ianlancetaylor changed the title testing: add TB.Setenv() proposal: testing: add TB.Setenv() Sep 7, 2020
@gopherbot gopherbot added this to the Proposal milestone Sep 7, 2020
@gopherbot gopherbot added the Proposal label Sep 7, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Sep 7, 2020
@mvdan
Copy link
Member

@mvdan mvdan commented Sep 7, 2020

As much as I agree with @mdlayher, I should also note that we've gotten a lot of good ideas from @frankban and @rogpeppe via https://github.com/frankban/quicktest such as Cleanup and Mkdir. And they have Setenv and Unsetenv too.

Perhaps they can give some input before this proposal gets a decision.

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Sep 8, 2020

@mdlayher

IMHO fetching data from environment variables should only ever be done in package main, and then plumbed throughout the rest of your program using regular Go data types. I don't think this would encourage good code hygiene since environment variables are effectively globals.

Even if that's the case, it's still a good idea to test that code in main that fetches the data from environment variables, and for that, a Setenv primitive can be very helpful. Moreover, sometimes there's no way to plumb things through without breaking backward compatibility, and, bad practice or not, environment variables can be a pragmatic solution in that case.

Although I haven't used it a huge amount (I count 73 uses in my code base), I've found this primitive to be very useful at times. It's not entirely trivial to get right either, because code can be sensitive to whether an environment variable is present vs empty, so just calling os.Setenv with the old value isn't always sufficient.

FWIW almost all of the uses were to test code that was there precisely to get environment variables and turn them into configuration available to the rest of the system.

I support this proposal (with modifications to unset instead as reset when appropriate) particularly with the isParallel check, which isn't something that external code can do, and nicely guards against a potential pitfall.

@rsc rsc moved this from Incoming to Active in Proposals Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.