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 Unsetenv to call os.Unsetenv and then restore the value using Cleanup #52817

Open
favonia opened this issue May 10, 2022 · 8 comments

Comments

@favonia
Copy link

@favonia favonia commented May 10, 2022

I wish to have t.Unsetenv(key) that unsets environment variables and automatically restores their values using Cleanup, in a way similar to t.Setenv(key, value). Here is the hypothetical usage:

func TestNoValue(t *testing.T) {
	// unset the environment variable ENV_KEY during the test
	t.Unsetenv("ENV_KEY")

	// ... more code here ...
}

A possible implementation:

func (c *common) Unsetenv(key string) {
	c.checkFuzzFn("Unsetenv")
	prevValue, prevSet := os.LookupEnv(key)

	if err := os.Unsetenv(key); err != nil {
		c.Fatalf("cannot unset environment variable: %v", err)
	}

	if prevSet {
		c.Cleanup(func() { os.Setenv(key, prevValue) })
	} else {
		c.Cleanup(func() { os.Unsetenv(key) })
	}
}

func (t *T) Unsetenv(key) {
	if t.isParallel {
		panic("testing: t.Unsetenv called after t.Parallel; cannot unset environment variables in parallel tests")
	}

	t.isEnvSet = true

	t.common.Unsetenv(key)
}
@gopherbot gopherbot added this to the Proposal milestone May 10, 2022
@mvdan
Copy link
Member

@mvdan mvdan commented May 10, 2022

Does t.Setenv(key, "") not work in most cases? I understand that programs can still use os.LookupEnv to differentiate an unset variable from a set-and-empty variable, but it's not very common at all. At the end of the day, these API helpers are for common needs, and I can't say that I've personally ever needed to call os.Unsetenv as opposed to os.Setenv or t.Setenv.

@favonia
Copy link
Author

@favonia favonia commented May 10, 2022

I can't say that I've personally ever needed to call os.Unsetenv as opposed to os.Setenv or t.Setenv.

@mvdan Thanks for your comment. I can find these lines in this repository that erase environment variables during testing:

go/src/cmd/go/go_test.go

Lines 277 to 280 in 6fd0520

os.Unsetenv("GOFLAGS")
os.Unsetenv("GOBIN")
os.Unsetenv("GOPATH")
os.Unsetenv("GIT_ALLOW_PROTOCOL")
// Set up the system environment variables
for i := range undefEnvList {
os.Unsetenv(undefEnvList[i])
}
os.Unsetenv("CGO_TEST_ALLOW")

I can't really say how common it is, but Go itself is using it for testing.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals May 10, 2022
@rsc
Copy link
Contributor

@rsc rsc commented May 11, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals May 11, 2022
@natefinch
Copy link
Contributor

@natefinch natefinch commented May 11, 2022

Honestly, most code (aside from reading configuration at application startup) probably shouldn't be reading from the environment anyway. It's the ultimate global variable.

I'm not sure that adding test helpers for things you shouldn't usually do is the best idea.

That being said, if we did want something like this, I think there's a simpler solution. The proposed function is doing two things, but only really needs to do one thing. Most of the time, test authors just want to ensure they leave the environment the way it was when the test started. This would do that, much more simply, IMO:

// CleanupEnv reverts all changes to environment variables at the end of this test.
func (t *testing.T) CleanupEnv() {
    vals := os.Environ()
    t.Cleanup(func(){ 
        os.Clearenv()
        for _, v := range vals {
            parts := strings.SplitN(v, "=", 2)
            os.Setenv(parts[0], parts[1])
        }
    })
}

(with probably better error handling)

@rsc
Copy link
Contributor

@rsc rsc commented May 11, 2022

@natefinch you're not wrong, but given that we already have t.Setenv, adding t.CleanupEnv separately seems like having two confusingly similar ways to do something.

I still don't think we need t.Unsetenv though.

@rsc
Copy link
Contributor

@rsc rsc commented May 18, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals May 18, 2022
@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented May 19, 2022

I agree that this isn't common enough to be that useful, especially as it's entirely trivial to do without the functionality built in:

t.Setenv(x, "")
os.Unsetenv(x)

@rogpeppe rogpeppe reopened this May 19, 2022
@favonia
Copy link
Author

@favonia favonia commented May 19, 2022

t.Setenv(x, "")
os.Unsetenv(x)

@rogpeppe Thanks for the tip. Somehow I missed this. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Likely Decline
Development

No branches or pull requests

6 participants