dashboard: use a more consistent definition for longtest builders

Previously, it was possible to define a builder whose IsLongTest method
would report positively, such that it'd test golang.org/x repos in long
test mode, but the main Go repository in short test mode. This would be
the case for any builder with "-longtest" suffix if it did not manually
include GO_TEST_SHORT=0 in its environment configuration.

It's unlikely we would want to do that intentionally, so this refactor
makes such misconfiguration less likely by inserting the GO_TEST_SHORT
environment variable assignment into the output from Env automatically.

Now, a longtest builder is defined in one consistent way: it must have
a "-longtest" suffix, so that the IsLongTest method reports positively.

For golang/go#39054.
For golang/go#29252.
For golang/go#12508.

Change-Id: Ic24df7b3b7de7db728bba6dc6ad4dd38a2e61e82
Reviewed-on: https://go-review.googlesource.com/c/build/+/233901
Reviewed-by: Carlos Amedee <[email protected]>
Reviewed-by: Alexander Rakoczy <[email protected]>

dashboard/builders.go

@@ -888,6 +888,11 @@ func (c *BuildConfig) Env() []string {
 	if c.FlakyNet {
 		env = append(env, "GO_BUILDER_FLAKY_NET=1")
 	}
+	if c.IsLongTest() {
+		// Set a private hook in cmd/dist to run main Go repository tests
+		// without the default -short flag. See golang.org/issue/12508.
+		env = append(env, "GO_TEST_SHORT=0")
+	}
 	env = append(env, c.HostConfig().env...)
 	return append(env, c.env...)
 }
@@ -1038,6 +1043,11 @@ func (c *BuildConfig) IsRace() bool {
 	return strings.HasSuffix(c.Name, "-race")
 }
 
+// IsLongTest reports whether this is a longtest builder.
+// A longtest builder runs tests without the -short flag.
+//
+// A builder is considered to be a longtest builder
+// if and only if its name ends with "-longtest".
 func (c *BuildConfig) IsLongTest() bool {
 	return strings.HasSuffix(c.Name, "-longtest")
 }
@@ -1754,7 +1764,6 @@ func init() {
 		},
 		needsGoProxy: true, // for cmd/go module tests
 		env: []string{
-			"GO_TEST_SHORT=0",
 			"GO_TEST_TIMEOUT_SCALE=5", // give them lots of time
 		},
 	})
@@ -1770,7 +1779,6 @@ func init() {
 		},
 		needsGoProxy: true, // for cmd/go module tests
 		env: []string{
-			"GO_TEST_SHORT=0",
 			"GO_TEST_TIMEOUT_SCALE=5", // give them lots of time
 		},
 	})
@@ -2072,7 +2080,6 @@ func init() {
 		},
 		needsGoProxy: true, // for cmd/go module tests
 		env: []string{
-			"GO_TEST_SHORT=0",
 			"GO_TEST_TIMEOUT_SCALE=5", // give them lots of time
 		},
 	})

dashboard/builders_test.go

@@ -849,3 +849,24 @@ func TestExpectedMacstadiumVMCount(t *testing.T) {
 		t.Fatalf("macstadium host count: got %d; want 20", got)
 	}
 }
+
+// Test that we have a longtest builder and
+// that its environment configuration is okay.
+func TestLongTestBuilder(t *testing.T) {
+	long, ok := Builders["linux-amd64-longtest"]
+	if !ok {
+		t.Fatal("we don't have a linux-amd64-longtest builder anymore, is that intentional?")
+	}
+	if !long.IsLongTest() {
+		t.Error("the linux-amd64-longtest builder isn't a longtest builder, is that intentional?")
+	}
+	var shortDisabled bool
+	for _, e := range long.Env() {
+		if e == "GO_TEST_SHORT=0" {
+			shortDisabled = true
+		}
+	}
+	if !shortDisabled {
+		t.Error("the linux-amd64-longtest builder doesn't set GO_TEST_SHORT=0, is that intentional?")
+	}
+}