cmd/coordinator, maintner/maintnerd/maintapi: use latest TRY= of patchset
Previously, for each patch set, only the first TRY=foo SlowBot request
was considered, and all follow up requests were being dropped. This is
a bad experience for users who sometimes want to correct their initial
SlowBot request on the same patch set.
Fixing bugs or making changes to the logic of finding TryBot work and
selecting SlowBots will be easier when it's factored into coordinator.
Fixes golang/go#42084.
Change-Id: Id33c415a26680b75cd393cc62f885964bbb67f6e
Reviewed-on: https://go-review.googlesource.com/c/build/+/325229
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
diff --git a/cmd/coordinator/coordinator.go b/cmd/coordinator/coordinator.go
index 8edb6cf..fd250ec 100644
--- a/cmd/coordinator/coordinator.go
+++ b/cmd/coordinator/coordinator.go
@@ -1299,6 +1299,9 @@
linuxBuilder := dashboard.Builders["linux-amd64"]
addXrepo := func(project string) *buildStatus {
+ if testingKnobSkipBuilds {
+ return nil
+ }
if !linuxBuilder.BuildsRepoTryBot(project, branch, branch) {
return nil
}
diff --git a/cmd/coordinator/coordinator_test.go b/cmd/coordinator/coordinator_test.go
index 4910850..a7c6c2c 100644
--- a/cmd/coordinator/coordinator_test.go
+++ b/cmd/coordinator/coordinator_test.go
@@ -233,6 +233,57 @@
}
}
+// Test that when there are multiple SlowBot requests on the same patch set,
+// the latest request is used. See golang.org/issue/42084.
+func TestIssue42084(t *testing.T) {
+ testingKnobSkipBuilds = true
+
+ work := &apipb.GerritTryWorkItem{ // Based on what maintapi's GoFindTryWork does for CL 324763. TryMessage is set later.
+ Project: "go",
+ Branch: "master",
+ ChangeId: "I023d5208374f867552ba68b45011f7990159868f",
+ Commit: "dd38fd80c3667f891dbe06bd1d8ed153c2e208da",
+ Version: 1,
+ GoCommit: []string{"9995c6b50aa55c1cc1236d1d688929df512dad53"},
+ GoBranch: []string{"master"},
+ GoVersion: []*apipb.MajorMinor{{1, 17}},
+ }
+
+ // First, determine builds without try messages. Our target SlowBot shouldn't be included.
+ ts := newTrySet(work)
+ hasWindowsARM64Builder := false
+ for _, bs := range ts.builds {
+ v := bs.NameAndBranch()
+ if v == "windows-arm64-aws" {
+ hasWindowsARM64Builder = true
+ }
+ }
+ if hasWindowsARM64Builder {
+ // This test relies on windows-arm64-aws builder not being default TryBot
+ // to provide coverage for issue 42084. If the build policy changes, need
+ // to pick another builder to use in this test.
+ t.Fatal("windows-arm64-aws builder was included even without TRY= message")
+ }
+
+ // Next, add try messages, and check that the SlowBot is now included.
+ work.TryMessage = []*apipb.TryVoteMessage{
+ {Message: "TRY=windows-arm64,windows-amd64", AuthorId: 1234, Version: 1},
+ {Message: "TRY=windows-arm64-aws", AuthorId: 1234, Version: 1},
+ }
+ ts = newTrySet(work)
+ hasWindowsARM64Builder = false
+ for i, bs := range ts.builds {
+ v := bs.NameAndBranch()
+ t.Logf("build[%d]: %s", i, v)
+ if v == "windows-arm64-aws" {
+ hasWindowsARM64Builder = true
+ }
+ }
+ if !hasWindowsARM64Builder {
+ t.Error("windows-arm64-aws SlowBot was not included")
+ }
+}
+
func TestFindWork(t *testing.T) {
if testing.Short() {
t.Skip("skipping in short mode")
diff --git a/maintner/maintnerd/maintapi/api.go b/maintner/maintnerd/maintapi/api.go
index 036db04..68e46d3 100644
--- a/maintner/maintnerd/maintapi/api.go
+++ b/maintner/maintnerd/maintapi/api.go
@@ -147,7 +147,8 @@
if !c.Updated.Equal(m.Time) || c.Author.NumericID != m.Author.NumericID || c.PatchSet != m.RevisionNumber {
continue
}
- if len(w.TryMessage) > 0 && c.PatchSet <= int(w.TryMessage[len(w.TryMessage)-1].Version) {
+ if len(w.TryMessage) > 0 && c.PatchSet < int(w.TryMessage[len(w.TryMessage)-1].Version) {
+ // Don't include try messages older than the latest we've seen. They're obsolete.
continue
}
tm := tryCommentRx.FindStringSubmatch(c.Message)
diff --git a/maintner/maintnerd/maintapi/api_test.go b/maintner/maintnerd/maintapi/api_test.go
index 174ea16..2a92710 100644
--- a/maintner/maintnerd/maintapi/api_test.go
+++ b/maintner/maintnerd/maintapi/api_test.go
@@ -232,6 +232,51 @@
},
want: `project:"go" branch:"master" change_id:"I358eb7b11768df8c80fb7e805abd4cd01d52bb9b" commit:"f99d33e72efdea68fce39765bc94479b5ebed0a9" version:88 go_version:<major:1 minor:17 > try_message:<message:"foo" author_id:1234 version:1 > try_message:<message:"bar, baz" author_id:5678 version:2 > `,
},
+
+ // Test that followup TRY= requests on the same patch set are included. See issue 42084.
+ {
+ proj: "go",
+ clnum: 324763,
+ ci: &gerrit.ChangeInfo{
+ CurrentRevision: "dd38fd80c3667f891dbe06bd1d8ed153c2e208da",
+ Revisions: map[string]gerrit.RevisionInfo{
+ "dd38fd80c3667f891dbe06bd1d8ed153c2e208da": {PatchSetNumber: 1},
+ },
+ Messages: []gerrit.ChangeMessageInfo{
+ {
+ Author: &gerrit.AccountInfo{NumericID: 1234},
+ Message: "Patch Set 1: Run-TryBot+1 Trust+1\n\n(1 comment)",
+ Time: gerrit.TimeStamp(time.Date(2021, 6, 3, 18, 58, 0, 0, time.UTC)),
+ RevisionNumber: 1,
+ },
+ {
+ Author: &gerrit.AccountInfo{NumericID: 1234},
+ Message: "Patch Set 1: Run-TryBot+1\n\n(1 comment)",
+ Time: gerrit.TimeStamp(time.Date(2021, 6, 3, 19, 16, 26, 0, time.UTC)),
+ RevisionNumber: 1,
+ },
+ },
+ },
+ comments: map[string][]gerrit.CommentInfo{
+ "/PATCHSET_LEVEL": {
+ {
+ PatchSet: 1,
+ Message: "TRY=windows-arm64,windows-amd64",
+ Updated: gerrit.TimeStamp(time.Date(2021, 6, 3, 18, 58, 0, 0, time.UTC)),
+ Author: &gerrit.AccountInfo{NumericID: 1234},
+ },
+ {
+ PatchSet: 1,
+ Message: "TRY=windows-arm64-aws",
+ Updated: gerrit.TimeStamp(time.Date(2021, 6, 3, 19, 16, 26, 0, time.UTC)),
+ Author: &gerrit.AccountInfo{NumericID: 1234},
+ },
+ },
+ },
+ want: `project:"go" branch:"master" change_id:"I023d5208374f867552ba68b45011f7990159868f" commit:"dd38fd80c3667f891dbe06bd1d8ed153c2e208da" version:1 go_version:<major:1 minor:17 > ` +
+ `try_message:<message:"windows-arm64,windows-amd64" author_id:1234 version:1 > ` +
+ `try_message:<message:"windows-arm64-aws" author_id:1234 version:1 > `,
+ },
}
for _, tt := range tests {
cl := c.Gerrit().Project("go.googlesource.com", tt.proj).CL(tt.clnum)
