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

Jump to bottom

all: replace os.Setenv with T.Setenv in tests #45448

Open
perillo opened this issue Apr 8, 2021 · 6 comments
Open

all: replace os.Setenv with T.Setenv in tests #45448

perillo opened this issue Apr 8, 2021 · 6 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@perillo
Copy link
Contributor

perillo commented Apr 8, 2021

TB.Setenv was added recently, but no changes have been submitted to update the tests.

Here is a list of all tests using os.Setenv (obtained using grep -F -l "os.Setenv" **/*_test.go):

std

crypto/x509/root_unix_test.go
go/build/build_test.go
go/internal/srcimporter/srcimporter_test.go
internal/execabs/execabs_test.go
net/http/transport_test.go
net/parse_test.go
os/exec/lp_unix_test.go
syscall/exec_unix_test.go
testing/testing_test.go
time/zoneinfo_test.go
time/zoneinfo_unix_test.go

cmd

cmd/cover/cover_test.go
cmd/go/go_test.go
cmd/go/internal/cache/cache_test.go
cmd/go/internal/generate/generate_test.go
cmd/go/internal/modload/query_test.go
cmd/go/internal/vcs/vcs_test.go
cmd/go/internal/work/security_test.go
cmd/internal/pkgpath/pkgpath_test.go
cmd/objdump/objdump_test.go

For the cmd module, however, no tests actually restore the original environment variable, so I will not touch them.

For the std module I plan to only use T.Setenv on this pattern:

defer os.Setenv(key, origValue)
os.Setenv(key, value)

There are some other cases like:

I'm not sure if T.Setenv should be used in a sub test.

There are a few case when the environment variables are not restored, like testing/testing_test.go and net/http/transport_test.go, but probably it is ok.
@perillo perillo changed the title all: replace os.Setenv with T.Setenv all: replace os.Setenv with T.Setenv in tests Apr 8, 2021
@perillo
Copy link
Contributor Author

perillo commented Apr 8, 2021

There seems to be an issue in os/env_unix_test.go.
In TestSetenvUnixEinval (https://github.com/golang/go/blob/master/src/os/env_unix_test.go#L25) Unsetenv is not called, and in TestExpandEnvShellSpecialVar (https://github.com/golang/go/blob/master/src/os/env_unix_test.go#L46) the error from Setenv is ignored.

@dmitshur dmitshur added this to the Backlog milestone Apr 9, 2021
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure. labels Apr 9, 2021
@gopherbot
Copy link

Change https://golang.org/cl/310030 mentions this issue: go/build: replace os.Setenv with T.Setenv

@gopherbot
Copy link

Change https://golang.org/cl/310031 mentions this issue: os/exec: replace os.Setenv with T.Setenv

@gopherbot
Copy link

Change https://golang.org/cl/310032 mentions this issue: time: replace os.Setenv with T.Setenv

@perillo
Copy link
Contributor Author

perillo commented Apr 14, 2021

In the end there were only a few cases where os.Setenv could be replaced with T.Setenv.

@perillo perillo mentioned this issue Apr 14, 2021
Closed
gopherbot pushed a commit that referenced this issue Apr 14, 2021
@perillo @ianlancetaylor
os/exec: replace os.Setenv with T.Setenv
492faae 
Updates #45448

Change-Id: I570e9894c4976d0a875388ef9ea4a2aa31b78013
Reviewed-on: https://go-review.googlesource.com/c/go/+/310031
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Apr 14, 2021
@perillo @ianlancetaylor
time: replace os.Setenv with T.Setenv
f18715c 
Updates #45448

Change-Id: Ic096fe1c58c124fb8d84ee15c9446e7ed060b24f
Reviewed-on: https://go-review.googlesource.com/c/go/+/310032
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Michael Knyszek <mknyszek@google.com>
gopherbot pushed a commit that referenced this issue Apr 14, 2021
@perillo @ianlancetaylor
go/build: replace os.Setenv with T.Setenv
d27bb8b 
Updates #45448

Change-Id: I4d8e5d7e57818355ef2bc33b57ddf9c8b8da3e62
Reviewed-on: https://go-review.googlesource.com/c/go/+/310030
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Trust: Michael Knyszek <mknyszek@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/310212 mentions this issue: time: add missing "os" import to zoneinfo_test.go

gopherbot pushed a commit that referenced this issue Apr 15, 2021
@neild
time: add missing "os" import to zoneinfo_test.go
566a87c 
Updates #45448

Change-Id: I2e79ae6b9cf43a481aa703578712619ea344e421
Reviewed-on: https://go-review.googlesource.com/c/go/+/310212
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@dmitshur @perillo @gopherbot