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.

Sign up for GitHub

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

testing: T.Setenv can be called after T.Parallel #55128

Closed
noi opened this issue Sep 18, 2022 · 7 comments
Closed

testing: T.Setenv can be called after T.Parallel #55128

noi opened this issue Sep 18, 2022 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@noi
Copy link
Contributor

noi commented Sep 18, 2022 •
edited
Loading

What version of Go are you using (go version)?

$ go version
go version go1.19.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="$HOME/.cache/go-build"
GOENV="$HOME/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="$HOME/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="$HOME/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="$HOME/dev/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1463438795=/tmp/go-build -gno-record-gcc-switches"

What did you do?

T.Setenv will panic if it be called after T.Parallel but it can be called in T.Run.

Sample code:
https://go.dev/play/p/54VhJKYpb_q

What did you expect to see?

I think it should panic even in T.Run.

What did you see instead?

Those tests become flaky without panic.

Others

I'm trying to fix this issue now.
@ianlancetaylor
Copy link
Member

I'm not sure what is wrong with calling t.Setenv in a function passed to t.Run. It would be wrong if the function calls t.Parallel, but if it doesn't, it should work reliably. It may not do what you want, but it shouldn't be flaky. Do I misunderstand?

@ZekeLu
Copy link
Contributor

ZekeLu commented Sep 18, 2022

I think noi@ means that a sub test is flaky when it calls t.Setenv while its parent has called t.Parallel. This looks plausible.

@ianlancetaylor
Copy link
Member

Ah, yes, I see. We need to walk the parent list or something.

@noi
Copy link
Contributor Author

noi commented Sep 18, 2022

Thank you for your replying!
I'm sorry for confusing you by my bad English. ZekeLu is right, I wanted to say like that.

I'm trying to fix this issue by passing isAncestorParalell to sub test in t.Run.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/431101 mentions this issue: testing: fix T.Setenv for not calling to it in parallel tests

@noi
Copy link
Contributor Author

noi commented Sep 18, 2022

I sent my changes, so I would appreciate it if you could review it when you have time🙇‍♂️

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 19, 2022
@cherrymui cherrymui added this to the Backlog milestone Sep 19, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/434115 mentions this issue: testing: update description of Setenv

gopherbot pushed a commit that referenced this issue Sep 29, 2022
@noi @gopherbot
testing: update description of Setenv
a14ac86 
Add the description of Setenv that it cannot use if the test have
parallel ancestors.

Fixes #55128

Change-Id: Ia5a1deaa1a3116d1ebb439600a7d316c7d155412
Reviewed-on: https://go-review.googlesource.com/c/go/+/434115
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@ZekeLu @ianlancetaylor @gopherbot @cherrymui @noi