go: testing: panic in Init if flag.Parse has already been called
What version of Go are you using (go version
)?
$ go version go version devel +b41eee4 Sun May 5 15:17:52 2019 +0000 darwin/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 GOARCH="amd64" GOBIN="" GOCACHE="/Users/viacheslav.poturaev/Library/Caches/go-build" GOENV="/Users/viacheslav.poturaev/Library/Preferences/go/env" GOEXE="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="darwin" GONOPROXY="" GONOSUMDB="" GOOS="darwin" GOPATH="/Users/viacheslav.poturaev/go" GOPROXY="direct" GOROOT="/Users/viacheslav.poturaev/sdk/gotip" GOSUMDB="off" GOTMPDIR="" GOTOOLDIR="/Users/viacheslav.poturaev/sdk/gotip/pkg/tool/darwin_amd64" GCCGO="gccgo" AR="ar" CC="clang" CXX="clang++" CGO_ENABLED="1" GOMOD="/Users/viacheslav.poturaev/go/src/github.com/hellofresh/ro-kit/cc/go.mod" 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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/lb/j0gz8jln36z4frqkrzmkdb0c0000gp/T/go-build279729567=/tmp/go-build -gno-record-gcc-switches -fno-common"
What did you do?
Ran test having custom command line flags, minimal example:
package my_test
import (
"flag"
"testing"
)
func init() {
flag.Parse()
}
func TestSomething(t *testing.T) {}
What did you expect to see?
go1.12 test .
ok github.com/bla/cc 0.007s
What did you see instead?
gotip test .
flag provided but not defined: -test.testlogfile
Usage of /var/folders/lb/j0gz8jln36z4frqkrzmkdb0c0000gp/T/go-build200454244/b001/cc.test:
FAIL github.com/bla/cc 0.007s
About this issue
- Original URL
- State: closed
- Created 5 years ago
- Reactions: 12
- Comments: 61 (50 by maintainers)
Links to this issue
Commits related to this issue
- Revert "cmd/go: move automatic testing.Init call into generated test code" This reverts CL 176098. Reason for revert: added complexity, but did not completely fix the underlying problem. A complete ... — committed to golang/go by bcmills 5 years ago
- cmd/golangorg: remove call to flag.Parse in regtest init func flag.Parse during init is always incorrect (other packages may want to register flags). Updates golang/go#31859 Change-Id: Id65bfaab520... — committed to golang/website by andybons 5 years ago
- test: don't call flag.Parse in init this addresses https://github.com/golang/go/issues/31859 — committed to markus-wa/demoinfocs-golang by markus-wa 5 years ago
- test: don't call flag.Parse in init this addresses https://github.com/golang/go/issues/31859 — committed to markus-wa/demoinfocs-golang by markus-wa 5 years ago
- crypto,les,p2p,tests: flags workaround in tests This change is releated to: https://github.com/golang/go/issues/31859 — committed to etclabscore/multi-geth-fork by tzdybal 5 years ago
- Fix tests for go1.13 See https://github.com/golang/go/issues/31859#issuecomment-532415187 Also increase timeouts in TestUnitEncodeCertIDGood as the aggressive timeouts there make the test flaky. Ch... — committed to observeinc/gosnowflake by tsuna 5 years ago
- Fix tests for go1.13 See https://github.com/golang/go/issues/31859#issuecomment-532415187 Also increase timeouts in TestUnitEncodeCertIDGood as the aggressive timeouts there make the test flaky. Ch... — committed to observeinc/gosnowflake by tsuna 5 years ago
- Fix "make test" under go 1.13 https://github.com/golang/go/issues/31859 — committed to ledgerwatch/erigon by yperbasis 5 years ago
- Fix tests for go1.13 See https://github.com/golang/go/issues/31859#issuecomment-532415187 Also increase timeouts in TestUnitEncodeCertIDGood as the aggressive timeouts there make the test flaky. Ch... — committed to snowflakedb/gosnowflake by tsuna 5 years ago
- Update the GetAllResources method to return a slice of resources and an error 1. Why is this change necessary? Because if an error occurs there is no way to handle it in the log.Fatal handle of the e... — committed to pitakill/azure_resources by pitakill 5 years ago
- Update the GetAllByGroupName method to return a slice of resources and an error 1. Why is this change necessary? Because if an error occurs there is no way to handle it in the log.Fatal handle of the... — committed to pitakill/azure_resources by pitakill 5 years ago
- Update the GetAllByGroupName method 1. Why is this change necessary? Because if an error occurs there is no way to handle it in the log.Fatal handle of the error 2. How does it address the issue? Re... — committed to pitakill/azure_resources by pitakill 5 years ago
- Update the GetAllByGroupName method 1. Why is this change necessary? Because if an error occurs there is no way to handle it in the log.Fatal handle of the error 2. How does it address the issue? Re... — committed to pitakill/azure_resources by pitakill 5 years ago
- go 13 test fix "flag provided, but not defined" https://github.com/golang/go/issues/31859#issuecomment-489889428 — committed to aerokube/selenoid-ui by lanwen 5 years ago
- Don't retun error on removing state when receiving messages When receiving messages the state for the message is removed. If no state is found an error was thrown. This should not cause an error as t... — committed to vacp2p/mvds by cammellos 5 years ago
- fix: tests with go 1.12 https://github.com/golang/go/issues/31859 — committed to lidel/meowkov by lidel 5 years ago
- fix: go 1.13 changed package import behaviour, causing test flags to be parsed before they were declared https://github.com/golang/go/issues/31859 In case anyone else lands here and is looking for a s... — committed to aeternity/aepp-sdk-go by randomshinichi 5 years ago
- fix: go 1.13 changed package import behaviour, causing test flags to be parsed before they were declared https://github.com/golang/go/issues/31859 In case anyone else lands here and is looking for a s... — committed to aeternity/aepp-sdk-go by randomshinichi 5 years ago
In case anyone else lands here and is looking for a simple answer:
With Go 1.13 you can’t do this anymore:
You must do this instead:
What we (or at least I) intended with #21051 / CL 173722 was:
go test
shouldn’t observe any difference or need to explicitly calltesting.Init
.testing
outside ofgo test
(notably,testing.Benchmark
within a package main) would need to usetesting.Init
if they want to use test flags.So @vearutop this means that you shouldn’t need to use
testing.Init
in your example code. In any case, it certainly shouldn’t fail with such a mysterious error.As @bcmills alluded to, the problem is that code which calls flag.Parse during initialization won’t get the test flags. The initialization of the user’s test package (“ptest”/“pxtest” in the go tool internal parlance) happens before the package main test runner initialization (“testmain”) so there’s nothing we can do in the generated testmain code to fix this.
The workaround that makes sense to me, which is what I think @bcmills is suggesting, is that we can insert a bit of code into the ptest/pxtest packages before we build them:
(Putting this in a variable declaration expression means that it happens before
flag.Parse
is called in a variable initialization as well as aninit
function. Callingflag.Parse
as part of variable declaration is ill-advised, of course, but it does work today.)I think this is pretty straightforward. I’ll send a CL soon.
Yeah, and same for variable declarations, modulo initialization dependencies. I’ll ensure that the synthesized file is passed to the compiler first.
The design of that pin feature is horrible… the pinned issue appears in the main issues page and it looks like a “notification” of some sort, with a prominent “X” in the upper-right area that people will press thinking they’re just removing a notification… instead, they unpin the issue and usually they don’t even notice they performed a public, global action.
Re-pinning this issue, for the same reason @bcmills originally did.
FYI @davecheney and everyone else who participated in the little pinning/unpinning skirmish.
I know this is closed and I’m late to the party, but I want to express how sad this makes me. Go has gone this long (until 1.13) telling people to put
flag.Parse()
in theinit()
and then all the sudden the behavior is silently broken. Do we expect all those users to find this thread? We will never know how many users we pissed off or alienated with this one.I would go as far as to say that this is very close to being against the principle of “no breaking language changes until go 2”. The fact that turning this failure into a panic is “too disruptive” tells you exactly how bad this is. instead of those users being greeted with an error, we just confuse them with broken behavior and maybe they’ll end up finding this thread some day. This is not the “go” I remember.
Just because we feel like it should be new tribal knowledge that
flag.Parse()
shouldn’t be called in init does not mean we can just break the user experience.Just look above this comment at the long list of people whose evenings have been ruined by this.
From a high level of programming obviousness, calling
flag.Parse()
in init makes perfect sense. If the problem is that other people may have added flags or something in a downstream package, make it soflag.Parse()
can only be called inpackage main
or something.It’s hard to understand why code would call
flag.Init
in aninit
function that is not in themain
package. That will ignore flags defined in other packages that are initialized later. I think I would want to see a clear explanation as to why we need to cater to that case at all, since it seems broken by design.I’m afraid that I have to disagree with your premise. We never told people to call
flag.Parse
ininit
. We always told people to call it inmain
. Callingflag.Parse
ininit
doesn’t make sense, as it means that all flags added by subsequentinit
functions would be unknown. That is true even in the main package.I talked with @rsc and @jayconrod today about what to do about this. Given the fact that CL 176098 didn’t completely resolve the breakage, and the complexity of both that and CL 184923, we’re inclined to roll back CL 176098 and take the position that it’s just not appropriate for
init
functions to callflag.Parse
.(I think that’s roughly in line with @mvdan’s opinion that the fixes are “too magical […] and in the end not worth doing”.)
To be clear, we are talking only about
flag.Parse
here, notflag.*
. The flag package is designed so that multiple packages can define flags, without those packages being aware of each other. It’s essential thatflag.Parse
be called only after all flags are defined, meaning only after all packages are initialized. The normal practice is that the main package callsflag.Parse
, because the main package is the only package that is sure to run after all packages are initialized.Your program may well be designed so that it is OK to call
flag.Parse
in aninit
function of some non-main package. But most programs do not and cannot work that way. So I am questioning how much we should twist the testing package to support that use case. Of course if the change to the testing package is simple, then that is fine. But I would not like to see a lot of complexity added to support a very unusual use case.Hello, I know I drop late to this, we also stumbled into this issue and while we kind of worked around it, I feel that what we do now to address this is a bit more convoluted than it used/needs to be. So the essence of the problem is the following question:
@bcmills that sounds like a great resolution to me.
Ideally I think we’d reject flag.Parse during init entirely (by panicking), but I’m sure that’s too disruptive and backwards-incompatible of a change to do now. The issues in this thread brought to my attention how common this is, and https://github.com/golang/go/issues/31859#issuecomment-492563946 makes me wonder how many cases there are in the wild where such code only happens to work as intended due to unspecified package initialization order (see #31636).
Perhaps it would be appropriate to check for flag.Parse during init as a vet check or in some other static analysis tool.
~We’re hitting
flag provided but not defined: -test.testlogfile
issue, even though we callflag.Parse()
in theTestMain()
function and we don’t do any flag related calls anywhere else.~~Do you think we might be hitting the same or a different issue?~
EDIT: It turns out that we, in fact, did call
flags.Parse(os.Args[1:])
somewhere in the runtime (really big code base). What a stupid code; triggering a stupid side effect 😃Now, I wish Go 2 restricted
flags.Parse()
into Main/TestMain functions only 😄There was one in the Go 1.13 release notes (https://golang.org/doc/go1.13#testing):
(That’s not to say that we shouldn’t also clarify documentation and implement the
vet
check, of course.)We too hit this at work, and I agree that we shouldn’t have been doing it in the first place - a library that declares a flag shouldn’t also call
flag.Parse
.I personally would close this issue as fixed, but I’d make it much simpler for developers upgrading to 1.13 to notice what’s wrong and know how to fix it. For example, we could make the
testing
package panic ifflag.Parse
is called before the main package’sinit
; this would be similar to what was done in https://golang.org/cl/177419Hi – sorry, I’ve been meaning to revisit this issue but haven’t had time recently. I’ll try to put together a CL which implements the approach I mentioned and then we can decide if we want to put it in or do something else for 1.13. I’ll have that in the next 2 days.
@mvdan
Can you be more specific about confusing behavior? I’m not sure how you’d observe the difference.
Build times shouldn’t be an issue; the build cache should still cache both versions.
To be clear, my suggested workaround does not involve build tags.
@mvdan I agree: we should panic in testing.Init if flag.Parse has already been called.
Using https://golang.org/pkg/testing/#hdr-Main is a bit cleaner.
Right, I’m saying that without the
_ "testing"
import your repro code is buggy due to depending on undefined behavior, and that bugginess is exposed by the init ordering changes unrelated to my testing changes that are on tip.By adding the
_ "testing"
import, your repro code no longer depends on undefined behavior, but it was broken by my change.So:
testing
being initialized first without actually importing itSorry, I meant to say that there are some changes on tip (that will be in Go 1.13). I updated my comment.
@vearutop thanks for bringing it to my attention.
It turns out that this particular repro code relies on undefined behavior: it assumes that the
testing
package is initialized before thesomewhere
package. There’s nothing in the spec that says it will be. And indeed, it so happens that on tip there are some initialization changes that already broke this code before any of my testing changes landed; see https://golang.org/cl/161337, https://golang.org/cl/170318, and related discussion over on #31636. If I run the repro with Go at 8515d9cf656dedce3dbcb09ac7dc00f036e454d3, which is this commit where CL 170318 went in and before any of my testing changes, I get a failure:(And if I run with
-race
, it sometimes fails and sometimes succeeds.)However, this all kind of doesn’t matter because you could fix your repro code by making
somewhere
import testing, perhaps just by adding_ "testing"
to the imports. Then you’re guaranteed that testing is initialized first, and so it works up until my changes break it. So this is a problem.To state it more generally, the “testinginit” workaround I did over in https://golang.org/cl/176098 ensures that testing.Init is called before the package under test is initialized. It does not ensure that testing.Init is called before that package’s dependencies are initialized, so if they call flag.Parse during initialization they may not see the testing flags.
I think that a solution is to move the testinginit workaround from the package under test into the
testing
package. That is,go test
would insert this file into thetesting
package:(This would be very simple with a
test
build tag, but we decided in #21360 not to do that.)@bcmills thoughts?