Go: cmd/go: in 1.15: change in "go test" argument parsing

[ianlancetaylor]

Reported at https://groups.google.com/forum/?oldui=1#!topic/golang-dev/2O4EdN7XNZ0 :+1:

all_test.go:

        package foo
        import (
            "flag"
            "os"
            "testing"
            "time"
        )
        var oFoo = flag.Duration("foo", time.Second, "")
        func TestMain(m *testing.M) {
            flag.Parse()
            os.Exit(m.Run())
        }
        func Test(t *testing.T) {
            t.Log("ok")
        }

        jnml@3900x:~/src/tmp$ go test -v -foo 1s
        === RUN   Test
            all_test.go:18: ok
        --- PASS: Test (0.00s)
        PASS
        ok      tmp 0.001s
        jnml@3900x:~/src/tmp$ go test -v -timeout 1h
        === RUN   Test
            all_test.go:18: ok
        --- PASS: Test (0.00s)
        PASS
        ok      tmp 0.002s
        jnml@3900x:~/src/tmp$ go test -v -timeout 1h -foo 1s
        === RUN   Test
            all_test.go:18: ok
        --- PASS: Test (0.00s)
        PASS
        ok      tmp 0.002s
        jnml@3900x:~/src/tmp$ go test -v -foo 1s -timeout 1h
        flag provided but not defined: -timeout
        Usage of /tmp/go-build167035760/b001/tmp.test:
          -foo duration
                 (default 1s)
          -test.bench regexp
                run only benchmarks matching regexp
          -test.benchmem
                print memory allocations for benchmarks
          -test.benchtime d
                run each benchmark for duration d (default 1s)
          -test.blockprofile file
                write a goroutine blocking profile to file
          -test.blockprofilerate rate
                set blocking profile rate (see
runtime.SetBlockProfileRate) (default 1)
          -test.count n
                run tests and benchmarks n times (default 1)
          -test.coverprofile file
                write a coverage profile to file
          -test.cpu list
                comma-separated list of cpu counts to run each test with
          -test.cpuprofile file
                write a cpu profile to file
          -test.failfast
                do not start new tests after the first test failure
          -test.list regexp
                list tests, examples, and benchmarks matching regexp then exit
          -test.memprofile file
                write an allocation profile to file
          -test.memprofilerate rate
                set memory allocation profiling rate (see
runtime.MemProfileRate)
          -test.mutexprofile string
                write a mutex contention profile to the named file
after execution
          -test.mutexprofilefraction int
                if >= 0, calls runtime.SetMutexProfileFraction() (default 1)
          -test.outputdir dir
                write profiles to dir
          -test.parallel n
                run at most n tests in parallel (default 24)
          -test.run regexp
                run only tests and examples matching regexp
          -test.short
                run smaller test suite to save time
          -test.testlogfile file
                write test action log to file (for use only by cmd/go)
          -test.timeout d
                panic test binary after duration d (default 0, timeout disabled)
          -test.trace file
                write an execution trace to file
          -test.v
                verbose: print additional output
        exit status 2
        FAIL    tmp 0.002s

In Go 1.14, the last command does this:

> ~/go1.14/bin/go test -v -foo 1s -timeout 1h
=== RUN   Test
    foo_test.go:17: ok
--- PASS: Test (0.00s)
PASS
ok      command-line-arguments  0.010s

I see some comments in the release notes about flag parsing, but I don't see anything that says that this case that used to work no longer works. I'm not sure precisely what changed here, but this needs to either be fixed or be clearly documented in the release notes. (My personal preference would be to fix it to work as it did previously.)

CC @bcmills @jayconrod

[rsc]

I don't know precisely why this changed, but the old behavior was technically wrong.
The processing assumed that -timeout was a flag, even though strictly speaking it might not have been.
If -foo were a boolean flag, then at the end of flag.Parse
you'd expect flag.Args() = ["1s", "-timeout", "1h"],
and it would be weird to find ["1s", "-test.timeout", "1h"] instead,
which is what earlier versions of Go did.

I tried back to Go 1.11 and they all do this rewrite of -timeout to -test.timeout though.
As much as I pedantically disagree with it (see above), it's probably too late to break these scripts.
I'd like to know why it broke first, but assuming it was an unintentional change, I lean toward undoing it.

If we do undo it, it should be undone in Go 1.15.1, not Go 1.16.

[rsc]

For what it's worth, if the change was intentional, it was probably for a test binary with its own -v flag.
If you do go test -foo -v expecting to pass -v to the binary and it rewrites to -foo -test.v, that's annoying.

[bcmills]

https://golang.org/cl/14826 (in 2015) caused “test flags after the package name” to no longer be processed as test flags.

The change in 1.15 was that we no longer assumed that -foo is a non-boolean flag, and thus no longer assumed that -timeout was a flag rather than a positional argument. (If -foo is a boolean flag, then 1s should rightly be interpreted as either a package name or a positional argument to the test binary.)

I think we can restore the non-boolean assumption, but it will result in misparsed arguments if it turns out that one of the unknown flags really is a boolean after all.

[mvdan]

I think we can restore the non-boolean assumption, but it will result in misparsed arguments if it turns out that one of the unknown flags really is a boolean after all.

Personally, I think that's the right approach, as most flag "kinds" are non-boolean, and it was the previous behavior.

That said, I think we should actively encourage Go developers to not rely on ambiguous flag parsing for go test. I realise that ship has sailed in terms of design and backwards compatibility, but we should still show how to have good "test flag hygiene".

[bcmills]

To make @rsc's comment more explicit, consider this test program:

package foo

import (
    "flag"
    "strings"
    "testing"
    "time"
)

var _ = flag.Duration("foo", time.Second, "")
var _ = flag.Bool("bar", false, "")

func Test(t *testing.T) {
    t.Logf("args: %s", strings.Join(flag.Args(), " "))
    t.Logf("test.timeout: %v", flag.Lookup("test.timeout").Value)
}

If the flag is non-boolean, then go1.14.7 parses the arguments consistent with how the test binary parses them. However, if the flag is a boolean, then the go command interprets the -timeout flag as -test.timeout, but the test binary itself interprets it as a positional argument, resulting in the argument having the effect of both a flag and a (rewritten) positional argument:

example.com$ go1.14.7 test -v -foo 1s -timeout 25h
=== RUN   Test
    example_test.go:14: args:
    example_test.go:15: test.timeout: 25h0m0s
--- PASS: Test (0.00s)
PASS
ok      example.com     0.009s

example.com$ go1.14.7 test -v -bar 1s -timeout 25h
=== RUN   Test
    example_test.go:14: args: 1s -test.timeout=25h
    example_test.go:15: test.timeout: 25h0m0s
--- PASS: Test (0.00s)
PASS
ok      example.com     0.009s

[bcmills]

In go1.12.17 the type of the flag could actually change whether the test respected the -timeout flag at all:

example.com$ go1.12.17 test -v . -foo 1s -timeout 25h
=== RUN   Test
--- PASS: Test (0.00s)
    example_test.go:14: args:
    example_test.go:15: test.timeout: 25h0m0s
PASS
ok      example.com     0.008s

example.com$ go1.12.17 test -v . -bar 1s -timeout 25h
=== RUN   Test
--- PASS: Test (0.00s)
    example_test.go:14: args: 1s -test.timeout=25h
    example_test.go:15: test.timeout: 0s
PASS
ok      example.com     0.009s

That, at least, was fixed in 1.13:

example.com$ go1.13.14 test -v . -foo 1s -timeout 25h
=== RUN   Test
--- PASS: Test (0.00s)
    example_test.go:14: args:
    example_test.go:15: test.timeout: 25h0m0s
PASS
ok      example.com     0.011s

example.com$ go1.13.14 test -v . -bar 1s -timeout 25h
=== RUN   Test
--- PASS: Test (0.00s)
    example_test.go:14: args: 1s -test.timeout=25h
    example_test.go:15: test.timeout: 25h0m0s
PASS
ok      example.com     0.008s

[bcmills]

@gopherbot, please backport to 1.15

[gopherbot]

Backport issue(s) opened: #40802 (for 1.15).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

[gopherbot]

Change https://golang.org/cl/248618 mentions this issue: cmd/go/internal/test: keep looking for go command flags after ambiguous test flag

[gopherbot]

Change https://golang.org/cl/248726 mentions this issue: [release-branch.go1.15] cmd/go/internal/test: keep looking for go command flags after ambiguous test flag