go test -c in go 1.13 does not include -test.timeout flag in binary?

[Craig Rodrigues]

Hi,

I ran a quick experiment in my source tree, and saw a difference in

the behavior of "go test -c" between go 1.12 and go 1.13.

EXPERIMENT 1:  using docker image golang:1.12-alpine with go1.12.16 linux/amd64

----------------------------------------------------------------------------------------------------------------------

go test -o my_test -c my_test.go

my_test --help

I see this flag:
  -test.timeout d
        panic test binary after duration d (default 0, timeout disabled)

EXPERIMENT 2: using docker image golang:1.13-alpine with go1.13.7 linux/amd64
----------------------------------------------------------------------------------------------------------------------

go tet -o my_test -c my_test.go

my_test --help

The -test.timeout flag is not there.  In fact all the -test.timeout flags are gone.

Is there a way that I can get back these -test.XXX flags with go1.13 and higher?

I see here that this might be related: 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.

But I'm not sure how to move forward to get back the behavior from go 1.12.

Thanks for any help.

--

Craig

[Ian Lance Taylor]

That could well be the problem. If so, the solution is to not call
flag.Parse from an init function.

Ian

[Craig Rodrigues]

Ah OK!

So my_test.go , and every test file in my codebase has:

func init() {
        ParseFlags()
}

which calls flag.Parse for a bunch of framework-specific test flags.

How should I restructure this so that I get the "-test.XXX" flags generated in the binary,
like I did with go 1.12?

Thanks.

--

Craig

[Craig Rodrigues]

This change in go test seems due to this:

 https://github.com/golang/go/issues/21051
 https://go-review.googlesource.com/c/go/+/173722/


so for my function:

func init() {
    ParseFlags()
}

I went in the ParseFlags() function and removed a call to flag.Parse() as
suggested by Ian (thanks for that suggestion!).

That seemed to fix things for me.

Will this be guaranteed to work in future go versions, or could this behavior change if something
else changes in the internals of go test?

--

Craig

[Ian Lance Taylor]

This will work in future Go versions.

Calling flag.Parse in an init function never worked reliably, unless
you took special care. Flags are themselves often defined in init
functions, so calling flag.Parse in an init function will see the
flags that were defined before the point of the call but not the flags
defined after the point of the call. I guess it was working for you,
but small changes in your code could have caused it to stop working.
We didn't document that calling flags.Parse in an init function didn't
work reliably because we didn't realize that anybody would ever do it.
The documentation always just said "After all flags are defined, call
flag.Parse", and "Must be called after all flags are defined and
before flags are accessed by the program" without spelling out that if
you call it in an init function you won't necessarily be calling it
after all flags are defined.

The change to the testing package changed exactly when the testing
flags are defined with respect to init functions, and it caused many
cases of calling flag.Parse from an init function to break. But those
cases were already fragile and could have been broken in other ways
also.

If you don't call flag.Parse from an init function you will be
following the documentation, and that will continue to work.

Sorry for the trouble.

Ian

[Craig Rodrigues]

On Friday, January 31, 2020 at 7:23:32 AM UTC-8, Ian Lance Taylor wrote:

Calling flag.Parse in an init function never worked reliably, unless
you took special care.  Flags are themselves often defined in init
functions, so calling flag.Parse in an init function will see the
flags that were defined before the point of the call but not the flags
defined after the point of the call.  I guess it was working for you,
but small changes in your code could have caused it to stop working.
We didn't document that calling flags.Parse in an init function didn't
work reliably because we didn't realize that anybody would ever do it.
The documentation always just said "After all flags are defined, call
flag.Parse", and "Must be called after all flags are defined and
before flags are accessed by the program" without spelling out that if
you call it in an init function you won't necessarily be calling it
after all flags are defined.

The change to the testing package changed exactly when the testing
flags are defined with respect to init functions, and it caused many
cases of calling flag.Parse from an init function to break.  But those
cases were already fragile and could have been broken in other ways
also.

If you don't call flag.Parse from an init function you will be
following the documentation, and that will continue to work.

Sorry for the trouble.

Thanks for the response.  I have a few questions.

1.  In https://golang.org/pkg/flag/#Parse , would it be appropriate to add a warning to not call this function from inside init()?
     This was not obvious to me from reading the docs.

2.   To fix my problem, I removed flag.Parse() completely.  Is that the appropriate fix?  Where would it be appropriate for me
      to call flag.Parse()?  Or should I never bother calling it (inside test files)?

3.   Is calling flag.Parse() from inside https://golang.org/pkg/testing/#Init supposed to work?  I tried that and it also did not work.

Thanks again.
--

Craig

[Ian Lance Taylor]

Documentation should normally be precise and not overly long. I
personally don't feel that an explicit comment about calling
flag.Parse from an init function carries its weight. The
documentation is already clear that you must only call Parse after
defining all flags, and as noted previously you have limited control
over initialization order. I think that more useful would be to
implement https://golang.org/issue/33190 (contributions welcome).

> 2. To fix my problem, I removed flag.Parse() completely. Is that the appropriate fix? Where would it be appropriate for me
> to call flag.Parse()? Or should I never bother calling it (inside test files)?

It's not necessary to call flag.Parse in a test. The test harness
will call it for you. It's OK to call flag.Parse from a TestMain
function, and in fact that is required if TestMain is going to change
its behavior based on a flag (this is documented at
https://golang.org/pkg/testing/#hdr-Main). Broadly speaking, the only
other place that Go code should call flag.Parse is in the main
function, though of course there can be specific reasons to call it
from other places.

> 3. Is calling flag.Parse() from inside https://golang.org/pkg/testing/#Init supposed to work? I tried that and it also did not work.

That is a function in the standard library, so I'm not sure what you
are asking. Are you changing the standard library? There shouldn't
be a need to do that in order to parse flags.

Ian

[Craig Rodrigues]

Oh, I misread the documentation.  I thought that Init() was a method that I was supposed to implement in my _test.go
file, but I realize now that I made a mistake and that testing.Init() is a method inside the testing package.

Regarding the fact that I should not call flag.Parse() from inside an init() method in my_test.go file,
if a note for this is not added to the documentation at https://golang.org/pkg/flag/#Parse ,
how would an end user know about this?  This detail is buried deep, and the only way to
understand this problem was to read the release notes for golang 1.13 and to ask on this mailing list.

--

Craig

[Tamás Gulácsi]

You should do as few as possible in init() functions - use them only when all else fails!

They're are convenience hacks, but an "import with a side effect" goes against Go's philosophy of explicitness.

Be explicit and avoid magic - so put your flag.Parse() into your main() function!

Tamás Gulácsi

[Ian Lance Taylor]

On Mon, Feb 3, 2020 at 8:13 AM Craig Rodrigues <crod...@gmail.com> wrote:
>
> Regarding the fact that I should not call flag.Parse() from inside an init() method in my_test.go file,
> if a note for this is not added to the documentation at https://golang.org/pkg/flag/#Parse ,
> how would an end user know about this? This detail is buried deep, and the only way to
> understand this problem was to read the release notes for golang 1.13 and to ask on this mailing list.

The docs for flag.Parse already say "Must be called after all flags
are defined and before flags are accessed by the program." I think
that is precise and accurate. The testing package defines flags, so
you have to call flag.Parse after the testing package defines those
flags. When you call flag.Parse in an init function, you are making
an assumption about how the testing package defines the flags, an
assumption that happened to work before the 1.13 release and happened
to fail after the 1.13 release.

I personally don't think it is appropriate to add docs for flag.Parse
that say "Note that the testing package defines flags after init
functions are run, so in a test don't call flag.Parse in an init
function." That makes the docs longer and somewhat harder to
understand for an unusual situation that I hope is already somewhat
clear.

Ian

[Craig Rodrigues]

I did some more web searching and found that someone else ran across a similar issue:
https://www.bountysource.com/issues/79987300-flag-provided-but-not-defined-test-timeout-go-1-13

where antoineco recommended replacing:

func init() {
    flag.Parse()
}

TestXYZ(t *testing.T) {}

with

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

TestXYZ(t *testing.T) {}

I searched further and saw that the documentation for TestMain at https://golang.org/pkg/testing/#hdr-Main
explicitly mentions that calling flag.Parse() should be done in TestMain.  So I think that is a fair place to

document this.

I guess I was unlucky enough to inherit a codebase where calling flag.Parse() inside init() "just worked" before. :(

--

Craig