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/14826Reviewed-by: Ian Lance Taylor <iant@golang.org>

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()

src/cmd/go/testdata/flag_test.go

+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")
+	}
+}

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"