Go: testing: panic in Init if flag.Parse has already been called

[vearutop]

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

[bcmills]

What was the behavior with Go 1.12? (Is this a regression?)

[vearutop]

I see this issue as a regression, go1.12 did not fail. So I could run tests with additional custom (application relevant) flags.

[vearutop]

I think this is likely happening due to changed init precedence, that test framework init ran after app-level init.

[bcmills]

Looks like it's probably due to CL 173722, which was intended to address #21051.

[bcmills]

CC @bradfitz

[vearutop]

OK, so if that is an intended behavior I can fix my code by adding a go1.13 build tagged init() with testing.Init() call inside.

[bcmills]

I don't think that a breaking change was intended. This seems like something that should be fixed before Go 1.13 is released.

[bradfitz]

We knew that fbc6a972226f889d2ab1150468755615098ee80f was going to break some people. It's arguably on the fence on whether it's inside the go1compat umbrella (it's half API, half tooling) and we guessed it would affect very few people. (generally more advanced people who could read the docs/release notes & work around)

That said, I haven't looked at this particular issue yet. I'll let @cespare have a look first.

[bcmills]

It seems possible to fix this by putting the call to testing.Init in an earlier init function injected into the test variant of the package, rather than in testing.MainStart.

If I understand correctly, the init functions within a package run in the order in which the source files were presented to the compiler, right?

[cespare]

Fun. I'll take a look today.

[cespare]

What we (or at least I) intended with #21051 / CL 173722 was:

• Anyone using go test shouldn't observe any difference or need to explicitly call testing.Init.
• People using testing outside of go test (notably, testing.Benchmark within a package main) would need to use testing.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:

var _ = func() bool {
    testing.Init()
    return true
}()

(Putting this in a variable declaration expression means that it happens before flag.Parse is called in a variable initialization as well as an init function. Calling flag.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.

If I understand correctly, the init functions within a package run in the order in which the source files were presented to the compiler, right?

Yeah, and same for variable declarations, modulo initialization dependencies. I'll ensure that the synthesized file is passed to the compiler first.

[gopherbot]

Change https://golang.org/cl/176098 mentions this issue: cmd/go: move automatic testing.Init call into generated test code

[vearutop]

@cespare I'm afraid this is still broken.

Given somewhere/init.go:

package somewhere

import (
    "flag"
)

var StopOnFailure bool

func init() {
    flag.BoolVar(&StopOnFailure, "stop-on-failure", false,
        "Stop processing on first failed scenario.")

    flag.Parse()
}

And mytest/my_test.go:

package my_test

import (
    "fmt"
    "testing"

    "path/to/somewhere"
)

func TestSomething(t *testing.T) {
    _ = somewhere.StopOnFailure
}

func BenchmarkSomething(b *testing.B) {
    b.ReportAllocs()
    for i := 0; i < b.N; i++ {
        _ = fmt.Sprintf("%d", i)
    }
}

gotip (go version devel +1e3ffb0c90 Tue May 14 22:30:48 2019 +0000 darwin/amd64) fails:

gotip test ./mytest/...
flag provided but not defined: -test.testlogfile
Usage of /var/folders/lb/j0gz8jln36z4frqkrzmkdb0c0000gp/T/go-build623057355/b001/mytest.test:
  -stop-on-failure
        Stop processing on first failed scenario.
FAIL path/to/mytest 0.005s
FAIL

gotip test -bench=. ./mytest/...
flag provided but not defined: -test.bench
Usage of /var/folders/lb/j0gz8jln36z4frqkrzmkdb0c0000gp/T/go-build638505735/b001/mytest.test:
  -stop-on-failure
        Stop processing on first failed scenario.
exit status 2
FAIL     path/to/mytest 0.005s
FAIL

[cespare]

@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 the somewhere 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:

~/apps/godev/bin/go test
flag provided but not defined: -test.timeout
Usage of /tmp/go-build059416027/b001/aaa.test:
  -stop-on-failure
        Stop processing on first failed scenario.
exit status 2
FAIL    aaa     0.001s

(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 the testing package:

package testing
func init() { Init() }

(This would be very simple with a test build tag, but we decided in #21360 not to do that.)

@bcmills thoughts?

[vearutop]

Not sure to understand:

And indeed, it so happens that in Go 1.12 there were some initialization changes that already broke this code

With go1.12.5 that example case never fails for me.

[cespare]

Sorry, I meant to say that there are some changes on tip (that will be in Go 1.13). I updated my comment.

[vearutop]

And also I tried adding _ "testing" to the imports:

package somewhere

import (
    "flag"
    _ "testing"
)

var StopOnFailure bool

func init() {
    flag.BoolVar(&StopOnFailure, "stop-on-failure", false,
        "Stop processing on first failed scenario.")

    flag.Parse()
}

but it did not help, same failure on gotip.

[cespare]

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:

• You need to fix your code if it depends on testing being initialized first without actually importing it
• We need to fix Go so that any package (not just the package under test) sees that the test flags have been registered when running as a test

[ianlancetaylor]

It's hard to understand why code would call flag.Init in an init function that is not in the main 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.

[bcmills]

I think that a solution is to move the testinginit workaround from the package under test into the testing package.

That might be worth doing anyway, just because it would reduce the number of injected calls to testing.Init().

[vearutop]

@ianlancetaylor my_test.go is owned by application but uses external library (somewhere) to run suite. Library imposes suite control with command line flags that are configured in library's init() so that app itself does not have to own irrelevant domain.

Alternatively env vars can be used instead of flags, though I'm not sure what is violated when flag.* is called outside main package.

[ianlancetaylor]

Alternatively env vars can be used instead of flags, though I'm not sure what is violated when flag.* is called outside main package.

To be clear, we are talking only about flag.Parse here, not flag.*. The flag package is designed so that multiple packages can define flags, without those packages being aware of each other. It's essential that flag.Parse be called only after all flags are defined, meaning only after all packages are initialized. The normal practice is that the main package calls flag.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 an init 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.

[bcmills]

I think it's important that we allow flag.Parse in an init function within a *_test.go file, since (in most cases) that file fills the same role as a main package.

I agree that it's not as important to support flag.Parse in an arbitrary init function outside of main or any *_test.go file.

[vearutop]

In my humble opinion Go standard library should not race with user code breaking existing code, hopefully that can be achieved without much penalty.

[bradfitz]

I think it's important that we allow flag.Parse in an init function within a *_test.go file, since (in most cases) that file fills the same role as a main package.

Using https://golang.org/pkg/testing/#hdr-Main is a bit cleaner.

[VojtechVitek]

~We're hitting flag provided but not defined: -test.testlogfile issue, even though we call flag.Parse() in the TestMain() 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 :D

[mvdan]

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 if flag.Parse is called before the main package's init; this would be similar to what was done in https://golang.org/cl/177419

[ianlancetaylor]

@mvdan I agree: we should panic in testing.Init if flag.Parse has already been called.

[mvdan]

Great - I'll send a patch this week for 1.13.

[mvdan]

@ianlancetaylor I've only just realised that the panic idea is contradictory. A check in testing.Init may never work, as the func init() { flag.Parse() } in another package has already run and made the whole program exit.

I think the check could still fire under some specific circumstances, but I think most of the flag.Parse misuses would continue to give the confusing error. I'm not sure that there's an easy way to deal with all of those, without resorting to hacks like changing flag's behavior if built as part of a test binary.

[ianlancetaylor]

Seems to me that the panic would still catch the problem in the normal case of running go test with no arguments. Or does go test always pass some arguments that would not be recognized?

[mvdan]

Yes, for example it seems like -test.testlogfile is always provided.

[ianlancetaylor]

Ah, well.

@bcmills: Is this really a release blocker for 1.13?

[bcmills]

I think we should still move the workaround into the testing package as @cespare suggested in https://github.com/golang/go/issues/31859#issuecomment-492563946.

I don't know whether it needs to block the release — without that it's a larger-than-necessary regression, but I don't know how much larger.

[mvdan]

If we went ahead with @cespare's suggestion above, wouldn't the testing package be different when built as part of a test binary versus as part of a non-test binary? Not that that's a bad thing per se, but it could lead to confusing behavior or unnecessarily increase build times.

Throwing another idea out there - what if go test picked up when a test binary failed with exactly an error like flag provided but not defined: -test.*? Then it could add a helpful error at the bottom, or replace the flag error output entirely. Given that the test output follows a format, I think the number of false positives would be practically zero, and we wouldn't have to mess with build tags.

[cespare]

Hi -- 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

wouldn't the testing package be different when built as part of a test binary versus as part of a non-test binary? Not that that's a bad thing per se, but it could lead to confusing behavior or unnecessarily increase build times.

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.

we wouldn't have to mess with build tags.

To be clear, my suggested workaround does not involve build tags.

[mvdan]

That is, go test would insert this file into the testing package

I think I'm getting confused by this line. If it's not done via a build tag, how would this work? Would go test make a copy of the testing package with this extra file, then build the test binary with that temporary package?

In any case, I think it's going to be easier to see once you send a CL :)

[gopherbot]

Change https://golang.org/cl/184923 mentions this issue: cmd/go: automatically call testing.Init under 'go test'

[cespare]

Sorry for the delay. I had to experiment with a few different approaches.

@mvdan actually, what I ended up with is related to build tags, though I'm not sure whether it counts as "messing" with them.

[mvdan]

Thanks - I just left a reply :)

What you sent as a CL was indeed what I had in mind when I mentioned build tags. I personally think this is too magical (see my comment on the CL), and in the end not worth doing to fix calling flag.Parse in libraries, which has always been broken one way or another.

[bcmills]

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 call flag.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”.)

[gopherbot]

Change https://golang.org/cl/186817 mentions this issue: Revert "cmd/go: move automatic testing.Init call into generated test code"

[gopherbot]

Change https://golang.org/cl/186818 mentions this issue: doc/go1.13: mention the effect of testing.Init on package initialization

[cespare]

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

[rsc]

Filed #33190, thanks for the suggestion @cespare.

[gopherbot]

Change https://golang.org/cl/187738 mentions this issue: cmd/golangorg: remove call to flag.Parse in regtest init func

[bcmills]

Pinning this issue ahead of the Go 1.13 release: we've gotten a couple more duplicates since go1.13rc1.

[cespare]

Re-pinning this issue, for the same reason @bcmills originally did.

FYI @davecheney and everyone else who participated in the little pinning/unpinning skirmish.

[mvdan]

@hyangah did you unpin this on purpose? :)

[hyangah]

@mvdan Sorry it was an accident, and thanks!

[ALTree]

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.

[tsuna]

In case anyone else lands here and is looking for a simple answer:

With Go 1.13 you can't do this anymore:

// foo_test.go
func init() {
    if !flag.Parsed() {
        flag.Parse()
    }
}

You must do this instead:

// foo_test.go
func TestMain(m *testing.M) {
    flag.Parse()
    os.Exit(m.Run())
}

[integrii]

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 the init() 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 so flag.Parse() can only be called in package main or something.

[ianlancetaylor]

I'm afraid that I have to disagree with your premise. We never told people to call flag.Parse in init. We always told people to call it in main. Calling flag.Parse in init doesn't make sense, as it means that all flags added by subsequent init functions would be unknown. That is true even in the main package.

[integrii]

Thanks for your reply @ianlancetaylor. I do agree that I overspoke when referencing flag.Parse() being called in init(). I understand that was never recommended by official go packages and examples now.

However, I do still think we can do more to prevent people from spending hours fighting this without adding anything complex to go. This is especially important because this used to work and now does not. The user may be wrong (I was), but this change still breaks things unexpectedly.

How about some or all of the following items?:

• Add something to the flag package docs to steer people away here and here
• Prioritizing #33190 (go vet warnings)
• Adding a note to the next release documentation

[bcmills]

Adding a note to the next release documentation

There was one in the Go 1.13 release notes (https://golang.org/doc/go1.13#testing):

Testing flags are now registered in the new Init function, which is invoked by the generated main function for the test. As a result, testing flags are now only registered when running a test binary, and packages that call flag.Parse during package initialization may cause tests to fail.

(That's not to say that we shouldn't also clarify documentation and implement the vet check, of course.)

[GreatSnoopy]

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:

• what if multiple packages of the application need for the init()-ialization part parameters that are to be extracted from the command line via Parse?. We initially implemented that with a special singleton object that was created in a configuration package that was imported by every other package needing to access some command line parameters. The configuration package did the flag.Parse in its init() function and everything worked just fine, until this change.
  Now we had to re-implement the initialization to be explicitly called via a chain of events that follow back to an initial call in the application's main function that actually triggers the flag.Parse() at a later time just to make it compatible with the new rules.
  So what I'm saying is that this change renders unusable any package init()-ialization that would be dependent on cli parameters. I don't think this should be the case, this seems an artificial limitation to me.
  I do understand the reasoning for this change, but maybe something else can be made to address this scenario as well? Maybe change in the flag.Parse() implementation to allow multiple interleaved calls to flag.Var and flag.Parse without generating errors?

[mvdan]

@GreatSnoopy this change shipped two releases ago, over a year ago - I think if you have further suggestions or bugs, you should file a new issue.

[GreatSnoopy]

Sorry for that, not trying to reopen the issue, just to ask a question that is closely related to this particular issue and I thought this would be the most appropriate place. I will create a new issue.