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

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

Closed
vearutop opened this issue May 6, 2019 · 61 comments
Closed

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

vearutop opened this issue May 6, 2019 · 61 comments

Comments

@vearutop
Copy link
Contributor

@vearutop vearutop commented May 6, 2019

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
Copy link
Member

@bcmills bcmills commented May 6, 2019

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

Loading

@bcmills bcmills changed the title test fails if flag.Parse() called in init cmd/go: go test fails if flag.Parse() called in init May 6, 2019
@bcmills bcmills added this to the Go1.13 milestone May 6, 2019
@vearutop
Copy link
Contributor Author

@vearutop vearutop commented May 6, 2019

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

Loading

@vearutop
Copy link
Contributor Author

@vearutop vearutop commented May 6, 2019

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

Loading

@bcmills bcmills changed the title cmd/go: go test fails if flag.Parse() called in init testing: go test fails if flag.Parse() called in init May 6, 2019
@bcmills
Copy link
Member

@bcmills bcmills commented May 6, 2019

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

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented May 6, 2019

Loading

@vearutop
Copy link
Contributor Author

@vearutop vearutop commented May 6, 2019

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.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented May 6, 2019

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

Loading

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 6, 2019

We knew that fbc6a97 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.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented May 6, 2019

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?

Loading

@cespare
Copy link
Contributor

@cespare cespare commented May 6, 2019

Fun. I'll take a look today.

Loading

@cespare
Copy link
Contributor

@cespare cespare commented May 7, 2019

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.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented May 9, 2019

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

Loading

@vearutop
Copy link
Contributor Author

@vearutop vearutop commented May 15, 2019

@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

Loading

@cespare
Copy link
Contributor

@cespare cespare commented May 15, 2019

@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 8515d9c, 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?

Loading

@cespare cespare reopened this May 15, 2019
@vearutop
Copy link
Contributor Author

@vearutop vearutop commented May 15, 2019

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.

Loading

@cespare
Copy link
Contributor

@cespare cespare commented May 15, 2019

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

Loading

@vearutop
Copy link
Contributor Author

@vearutop vearutop commented May 15, 2019

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.

Loading

@cespare
Copy link
Contributor

@cespare cespare commented May 15, 2019

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

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 15, 2019

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.

Loading

pgier added a commit to pgier/prometheus-operator that referenced this issue Apr 17, 2020
This allows tests to be run against functions in Main without
conflicting between test flags and application flags.

More info: golang/go#31859
hugelgupf added a commit to rminnich/u-root that referenced this issue May 2, 2020
When flag.Parse is in main, it is _only_ called in the command binary.

When flag.Parse is in init, it is _also_ called in the test binary. The
test binary, however, likes to register its own flags, which due to init
ordering usually happens after the command's inits.

In Go 1.13, flag registration was delayed to be later in the init
process, which is how this issue came up.

See also golang/go#31859

Signed-off-by: Chris Koch <chrisko@google.com>
hugelgupf added a commit to rminnich/u-root that referenced this issue May 2, 2020
When flag.Parse is in main, it is _only_ called in the command binary.

When flag.Parse is in init, it is _also_ called in the test binary. The
test binary, however, likes to register its own flags, which due to init
ordering usually happens after the command's inits.

In Go 1.13, flag registration was delayed to be later in the init
process, which is how this issue came up.

See also golang/go#31859

Signed-off-by: Chris Koch <chrisko@google.com>
hugelgupf added a commit to u-root/u-root that referenced this issue May 3, 2020
When flag.Parse is in main, it is _only_ called in the command binary.

When flag.Parse is in init, it is _also_ called in the test binary. The
test binary, however, likes to register its own flags, which due to init
ordering usually happens after the command's inits.

In Go 1.13, flag registration was delayed to be later in the init
process, which is how this issue came up.

See also golang/go#31859

Signed-off-by: Chris Koch <chrisko@google.com>
imartinezortiz added a commit to imartinezortiz/dns-proxy-server that referenced this issue May 4, 2020
dericmiller added a commit to TrueTickets/vaultlib that referenced this issue May 11, 2020
Go 1.13 introduces a Testing.Init function for handling test flags.

https://tip.golang.org/doc/go1.13#testing

This has the unfortunate consequence of panicking if flags are parsed
before testing.Init is called (for instance, in init). We therefore
update the code to parse flags in the Testing Main override such that
tests may be run in Go 1.11-1.14.

Related:
golang/go#31859

Co-authored-by: Andrew Pinkham <code@andrewsforge.com>

PR #3
[close ch1056]
@GreatSnoopy
Copy link

@GreatSnoopy GreatSnoopy commented May 14, 2020

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?

Loading

@mvdan
Copy link
Member

@mvdan mvdan commented May 14, 2020

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

Loading

@GreatSnoopy
Copy link

@GreatSnoopy GreatSnoopy commented May 15, 2020

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.

Loading

somombo added a commit to somombo/flatbuffers that referenced this issue May 23, 2020
Adds go.mod file to project root.

Fixes go_test's inoperability with go1.13+.

In go1.13+, invoking `flag.Parse()` in a test file's `init()`
does not work.

See https://golang.org/pkg/testing/#hdr-Main
and golang/go#31859
benjaminhuo pushed a commit to kubesphere/prometheus-operator that referenced this issue Jun 22, 2020
This allows tests to be run against functions in Main without
conflicting between test flags and application flags.

More info: golang/go#31859
benjaminhuo pushed a commit to benjaminhuo/prometheus-operator that referenced this issue Jun 26, 2020
This allows tests to be run against functions in Main without
conflicting between test flags and application flags.

More info: golang/go#31859
benjaminhuo pushed a commit to benjaminhuo/prometheus-operator that referenced this issue Jun 26, 2020
This allows tests to be run against functions in Main without
conflicting between test flags and application flags.

More info: golang/go#31859
benjaminhuo pushed a commit to kubesphere/prometheus-operator that referenced this issue Jun 28, 2020
This allows tests to be run against functions in Main without
conflicting between test flags and application flags.

More info: golang/go#31859
johanneswuerbach added a commit to johanneswuerbach/prometheus_varnish_exporter that referenced this issue Sep 30, 2020
Tests failed with "flag provided but not defined: -test.testlogfile"

See golang/go#31859
daveyarwood added a commit to alda-lang/alda that referenced this issue Oct 25, 2020
This hopefully fixes the issue we were seeing in CI, related to
golang/go#31859.
regenvanwalbeek-wf added a commit to Workiva/frugal that referenced this issue Dec 1, 2020
reltuk added a commit to reltuk/flatbuffers that referenced this issue Jan 7, 2021
Calling flags.Parse() within init() races with other packages which register
flags in their init(), and in particular with the testing package itself. It is
more reliable to call flags.Parse() from a TestMain implementation.

See golang/go#31859,
golang/go#33869.
aardappel pushed a commit to google/flatbuffers that referenced this issue Jan 7, 2021
#6388)

* tests/GoTest.sh: Fix flags.Parse location to work on new go SDKs.

Calling flags.Parse() within init() races with other packages which register
flags in their init(), and in particular with the testing package itself. It is
more reliable to call flags.Parse() from a TestMain implementation.

See golang/go#31859,
golang/go#33869.

* .github: Enable build-go action in build.yaml workflow.
@Wang-Kai
Copy link

@Wang-Kai Wang-Kai commented Feb 23, 2021

Calling flag.Parse as part of variable declaration is ill-advised

Why ?

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 24, 2021

@Wang-Kai This issue was closed a year and a half ago. Closed issues are not a good place for discussions. Thanks.

Loading

@golang golang locked as resolved and limited conversation to collaborators Feb 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet