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 <carlos@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
diff --git a/dashboard/builders.go b/dashboard/builders.go
index 2580a91..039967c 100644
--- a/dashboard/builders.go
+++ b/dashboard/builders.go
@@ -888,6 +888,11 @@
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 @@
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 @@
},
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 @@
},
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 @@
},
needsGoProxy: true, // for cmd/go module tests
env: []string{
- "GO_TEST_SHORT=0",
"GO_TEST_TIMEOUT_SCALE=5", // give them lots of time
},
})
diff --git a/dashboard/builders_test.go b/dashboard/builders_test.go
index 542ce69..dfef255 100644
--- a/dashboard/builders_test.go
+++ b/dashboard/builders_test.go
@@ -849,3 +849,24 @@
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?")
+ }
+}
