From 6acb4d944dafa13a6c80faffc4e7ecc47d2bcdbc Mon Sep 17 00:00:00 2001 From: Rob Pike Date: Tue, 22 Sep 2015 14:23:32 -0700 Subject: [PATCH] cmd/go: fix processing of flags for test binaries. The usage message says: test [-c] [-i] [build and test flags] [packages] [flags for test binary] but this was not what was implemented. Instead, after packages are named, flag processing continues, which makes it impossible, for example, to pass to the binary a flag with the same name as a test flag. This was triggered by the -v flag in glog. Consider this test: package pkg ... imports ... var v = flag.Int("v", 0, "v flag") func TestFoo(t *testing.T) { if *v != 7 { log.Fatal(*v) } } Attempting to run this test with go test pkg -v=7 would give a usage message. This change allows it. In fact it allows go test -v pkg -v=7 The solution is to implement the usage message. One compatibility issue is that flags after the package name are no longer processed as test flags, so this no longer works: go test foo -cover One must write go test -cover foo I do not think this is onerous but it must be called out in the release notes. Fixes #12177. Change-Id: Ib9267884b47a6b0c183efa888ec78333272113aa Reviewed-on: https://go-review.googlesource.com/14826 Reviewed-by: Ian Lance Taylor --- src/cmd/go/go_test.go | 12 ++++++-- src/cmd/go/testdata/flag_test.go | 16 ++++++++++ src/cmd/go/testflag.go | 52 +++++++++++++++----------------- 3 files changed, 50 insertions(+), 30 deletions(-) create mode 100644 src/cmd/go/testdata/flag_test.go diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go index abd6308774..ab78fe9a88 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -1698,7 +1698,7 @@ func TestCoverageUsesSetMode(t *testing.T) { tg := testgo(t) defer tg.cleanup() tg.creatingTemp("testdata/cover.out") - tg.run("test", "-short", "-cover", "encoding/binary", "-coverprofile=testdata/cover.out") + tg.run("test", "-short", "-coverprofile=testdata/cover.out", "encoding/binary") data := tg.getStdout() + tg.getStderr() if out, err := ioutil.ReadFile("testdata/cover.out"); err != nil { t.Error(err) @@ -1721,7 +1721,7 @@ func TestCoverageUsesAtomicModeForRace(t *testing.T) { tg := testgo(t) defer tg.cleanup() tg.creatingTemp("testdata/cover.out") - tg.run("test", "-short", "-race", "-cover", "encoding/binary", "-coverprofile=testdata/cover.out") + tg.run("test", "-short", "-race", "-coverprofile=testdata/cover.out", "encoding/binary") data := tg.getStdout() + tg.getStderr() if out, err := ioutil.ReadFile("testdata/cover.out"); err != nil { t.Error(err) @@ -1744,7 +1744,7 @@ func TestCoverageUsesActualSettingToOverrideEvenForRace(t *testing.T) { tg := testgo(t) defer tg.cleanup() tg.creatingTemp("testdata/cover.out") - tg.run("test", "-short", "-race", "-cover", "encoding/binary", "-covermode=count", "-coverprofile=testdata/cover.out") + tg.run("test", "-short", "-race", "-covermode=count", "-coverprofile=testdata/cover.out", "encoding/binary") data := tg.getStdout() + tg.getStderr() if out, err := ioutil.ReadFile("testdata/cover.out"); err != nil { t.Error(err) @@ -1940,6 +1940,12 @@ func TestGoTestFooTestWorks(t *testing.T) { tg.run("test", "testdata/standalone_test.go") } +func TestGoTestFlagsAfterPackage(t *testing.T) { + tg := testgo(t) + defer tg.cleanup() + tg.run("test", "-v", "testdata/flag_test.go", "-v=7") // Two distinct -v flags. +} + func TestGoTestXtestonlyWorks(t *testing.T) { tg := testgo(t) defer tg.cleanup() diff --git a/src/cmd/go/testdata/flag_test.go b/src/cmd/go/testdata/flag_test.go new file mode 100644 index 0000000000..ddf613d870 --- /dev/null +++ b/src/cmd/go/testdata/flag_test.go @@ -0,0 +1,16 @@ +package flag_test + +import ( + "flag" + "log" + "testing" +) + +var v = flag.Int("v", 0, "v flag") + +// Run this as go test pkg -v=7 +func TestVFlagIsSet(t *testing.T) { + if *v != 7 { + log.Fatal("v flag not set") + } +} diff --git a/src/cmd/go/testflag.go b/src/cmd/go/testflag.go index 1f3e3d316a..32a84b698f 100644 --- a/src/cmd/go/testflag.go +++ b/src/cmd/go/testflag.go @@ -80,40 +80,29 @@ func init() { // Unfortunately for us, we need to do our own flag processing because go test // grabs some flags but otherwise its command line is just a holding place for // pkg.test's arguments. -// We allow known flags both before and after the package name list, -// to allow both -// go test fmt -custom-flag-for-fmt-test -// go test -x math +// The usage is: +// go test [test flags] [packages] [flags for test binary] +// Thus we process test flags (adding -test. to each) until we find a non-flag, +// which introduces the optional list of packages. We collect the package paths +// until we find another -flag, and pass that and the rest of the command line +// to the test binary untouched. +// For backwards compatibility with a poor design, if while processing test +// flags we see an unrecognized flag, we accept it as an argument to the binary. +// For this to work in general, one must say -foo=xxx not -foo xxx or else +// xxx will be taken to be a package path. As said, the design is poor. func testFlags(args []string) (packageNames, passToTest []string) { - inPkg := false outputDir := "" - for i := 0; i < len(args); i++ { + // Flags. + var i int + for i = 0; i < len(args); i++ { if !strings.HasPrefix(args[i], "-") { - if !inPkg && packageNames == nil { - // First package name we've seen. - inPkg = true - } - if inPkg { - packageNames = append(packageNames, args[i]) - continue - } - } - - if inPkg { - // Found an argument beginning with "-"; end of package list. - inPkg = false + break // Start of packages. } f, value, extraWord := testFlag(args, i) if f == nil { - // This is a flag we do not know; we must assume - // that any args we see after this might be flag - // arguments, not package names. - inPkg = false - if packageNames == nil { - // make non-nil: we have seen the empty package list - packageNames = []string{} - } + // This is a flag we do not know. Pass it to the test but keep + // processing flags. passToTest = append(passToTest, args[i]) continue } @@ -174,6 +163,15 @@ func testFlags(args []string) (packageNames, passToTest []string) { passToTest = append(passToTest, "-test."+f.name+"="+value) } } + // Package names. + for ; i < len(args); i++ { + if strings.HasPrefix(args[i], "-") { + break // Start of trailing arguments. + } + packageNames = append(packageNames, args[i]) + } + // Trailing arguments. + passToTest = append(passToTest, args[i:]...) if testCoverMode == "" { testCoverMode = "set" -- 2.48.1