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

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

Comments

@perillo
Copy link
Contributor

@perillo 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 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.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 14, 2021

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

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 14, 2021

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

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 14, 2021

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

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 14, 2021

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

gopherbot pushed a commit that referenced this issue Apr 14, 2021
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
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
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

@gopherbot gopherbot commented Apr 15, 2021

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
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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants