[gold] Fix flaky commenter test
This was occasionally caught by the Race bot.
Change-Id: I4e67d1327db179fda0a880cd38aa31a92c35d143
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/264560
Commit-Queue: Kevin Lubick <kjlubick@google.com>
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
diff --git a/go/util/util.go b/go/util/util.go
index 54b2afc..fc89ea2 100644
--- a/go/util/util.go
+++ b/go/util/util.go
@@ -594,6 +594,17 @@
}
// ChunkIterParallel is similar to ChunkIter but it uses an errgroup to run the chunks in parallel.
+// To avoid costly execution from happening after the error context is cancelled, it is recommended
+// to include a context short-circuit inside of the loop processing the subslice:
+// var xs []string
+// util.ChunkIterParallel(ctx, len(xs), 10, func(ctx context.Context, start, stop int) error {
+// for _, tr := range xs[start:stop] {
+// if err := ctx.Err(); err != nil {
+// return err
+// }
+// // Do work here.
+// }
+// }
func ChunkIterParallel(ctx context.Context, length, chunkSize int, fn func(ctx context.Context, startIdx int, endIdx int) error) error {
if chunkSize < 1 {
return fmt.Errorf("Chunk size may not be less than 1 (saw %d).", chunkSize)
diff --git a/golden/go/code_review/commenter/commenter.go b/golden/go/code_review/commenter/commenter.go
index 2a68a62..3dbf4a0 100644
--- a/golden/go/code_review/commenter/commenter.go
+++ b/golden/go/code_review/commenter/commenter.go
@@ -79,6 +79,9 @@
beforeCount := len(stillOpen)
err := util.ChunkIterParallel(ctx, len(xcl), chunks, func(ctx context.Context, startIdx int, endIdx int) error {
for _, cl := range xcl[startIdx:endIdx] {
+ if err := ctx.Err(); err != nil {
+ return skerr.Wrap(err)
+ }
open, err := i.updateCLInStoreIfAbandoned(ctx, cl)
if err != nil {
return skerr.Wrap(err)
diff --git a/golden/go/code_review/commenter/commenter_test.go b/golden/go/code_review/commenter/commenter_test.go
index 0d7a059..e7fc42c 100644
--- a/golden/go/code_review/commenter/commenter_test.go
+++ b/golden/go/code_review/commenter/commenter_test.go
@@ -5,6 +5,7 @@
"errors"
"fmt"
"strconv"
+ "strings"
"testing"
"time"
@@ -160,9 +161,7 @@
c := New(mcr, mcs, instanceURL, false)
err := c.CommentOnChangeListsWithUntriagedDigests(context.Background())
- require.Error(t, err)
- assert.Contains(t, err.Error(), "down")
- assert.Contains(t, err.Error(), "github")
+ assertErrorWasCanceledOrContains(t, err, "down", "github")
}
// TestUpdateNotOpenBotsCLStoreError tests that we bail out if writing to the clstore fails.
@@ -191,8 +190,7 @@
c := New(mcr, mcs, instanceURL, false)
err := c.CommentOnChangeListsWithUntriagedDigests(context.Background())
- require.Error(t, err)
- assert.Contains(t, err.Error(), "firestore broke")
+ assertErrorWasCanceledOrContains(t, err, "firestore broke")
}
// TestCommentOnCLsSunnyDay tests a typical case where two of the open ChangeLists have patchsets
@@ -365,8 +363,7 @@
c := New(mcr, mcs, instanceURL, false)
err := c.CommentOnChangeListsWithUntriagedDigests(context.Background())
- require.Error(t, err)
- assert.Contains(t, err.Error(), "internet down")
+ assertErrorWasCanceledOrContains(t, err, "internet down")
}
func makeChangeLists(n int) []code_review.ChangeList {
@@ -402,4 +399,18 @@
return xps
}
+// assertErrorWasCanceledOrContains helps with the cases where the error that is returned is
+// non-deterministic, for example, when using an errgroup. It checks that the error message matches
+// a context being canceled or contains the given submessages.
+func assertErrorWasCanceledOrContains(t *testing.T, err error, submessages ...string) {
+ require.Error(t, err)
+ e := err.Error()
+ if strings.Contains(e, "canceled") {
+ return
+ }
+ for _, m := range submessages {
+ assert.Contains(t, err.Error(), m)
+ }
+}
+
const instanceURL = "gold.skia.org"
