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 <iant@golang.org>

diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go
index abd63087747df90318d4f36e1c180d799a1fe452..ab78fe9a88452fe275ab29b6f8474ff0f6d2dcab 100644 (file)
--- 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 (file)
index 0000000..ddf613d
--- /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 1f3e3d316af5a8794e8746e6617d9a943a5fc3a3..32a84b698f7cdc2e023d6111255bdd3dc1c2c157 100644 (file)
--- 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"