[tools][cts] Reduce diagnostic spam
We're getting an awful lot of "expectation is partly covered by previous expectations and the remaining tests all pass" comments sprayed over the roll CLs.
This can happen when a non-KEEP chunk has multiple expectations with the same query but with different tags. This is because the update process takes each expectation, strips the tags, then rebuilds the expectation, which can generate multiple new expectations with different tags. These new expectations can then overlap with the expectations that follow.
Don't produce these diagnostics if the expectation is covered by one in the same chunk.
Change-Id: I6c020c696b8ef66975be6ab5a60aa0c3313d9cc5
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/172760
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Auto-Submit: Ben Clayton <bclayton@google.com>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
diff --git a/tools/src/cts/expectations/update.go b/tools/src/cts/expectations/update.go
index 323f6d1..f20821c 100644
--- a/tools/src/cts/expectations/update.go
+++ b/tools/src/cts/expectations/update.go
@@ -427,11 +427,8 @@
}
}
- // Begin building the output chunk.
- // Copy over the chunk's comments.
- out := Chunk{Comments: in.Comments}
-
// Build the new chunk's expectations
+ newExpectations := container.NewMap[string, Expectation]()
for _, exIn := range in.Expectations {
if u.pb != nil {
u.pb.Update(progressbar.Status{Total: progress.totalExpectations, Segments: []progressbar.Segment{
@@ -440,20 +437,17 @@
progress.currentExpectation++
}
- exOut := u.expectation(exIn, isImmutable)
- out.Expectations = append(out.Expectations, exOut...)
+ u.addExpectations(newExpectations, exIn, isImmutable)
}
// Sort the expectations to keep things clean and tidy.
- out.Expectations.Sort()
- return out
+ return Chunk{Comments: in.Comments, Expectations: newExpectations.Values()}
}
// expectation returns a new list of Expectations, based on the Expectation 'in',
// using the new result data.
-func (u *updater) expectation(in Expectation, immutable bool) []Expectation {
- // noResults is a helper for returning when the expectation has no test
- // results.
+func (u *updater) addExpectations(out container.Map[string, Expectation], in Expectation, isImmutable bool) {
+ // noResults is a helper for returning when the expectation has no test results.
noResults := func() []Expectation {
if len(in.Tags) > 0 {
u.diag(Warning, in.Line, "no results found for '%v' with tags %v", in.Query, in.Tags)
@@ -472,12 +466,14 @@
// If we can't find any results for this query + tag combination, then bail.
switch {
case errors.As(err, &query.ErrNoDataForQuery{}):
- return noResults()
+ noResults()
+ return
case err != nil:
u.diag(Error, in.Line, "%v", err)
- return []Expectation{}
+ return
case len(results) == 0:
- return noResults()
+ noResults()
+ return
}
// Before returning, mark all the results as consumed.
@@ -486,21 +482,31 @@
// expectationsForRoot()
defer u.qt.markAsConsumed(q, in.Tags, in.Line)
- if immutable { // Expectation chunk was marked with 'KEEP'
+ // keyOf returns the map key for out
+ keyOf := func(e Expectation) string { return fmt.Sprint(e.Tags, e.Query, e.Status) }
+
+ if isImmutable { // Expectation chunk was marked with 'KEEP'
// Add a diagnostic if all tests of the expectation were 'Pass'
if s := results.Statuses(); len(s) == 1 && s.One() == result.Pass {
u.diagAllPass(in.Line, results)
}
- return []Expectation{in}
+ out.Add(keyOf(in), in)
+ return
}
// Rebuild the expectations for this query.
expectations, somePass, someConsumed := u.expectationsForRoot(q, in.Line, in.Bug, in.Comment)
+ // Add the new expectations to out
+ for _, expectation := range expectations {
+ out.Add(keyOf(expectation), expectation)
+ }
+
// Add a diagnostic if the expectation is filtered away
- if len(expectations) == 0 {
+ if !out.Contains(keyOf(in)) && len(expectations) == 0 {
switch {
case somePass && someConsumed:
+ fmt.Println("\n", strings.Join(out.Keys(), "\n"))
u.diag(Note, in.Line, "expectation is partly covered by previous expectations and the remaining tests all pass")
case someConsumed:
u.diag(Note, in.Line, "expectation is fully covered by previous expectations")
@@ -509,7 +515,6 @@
}
}
- return expectations
}
// addNewExpectations (potentially) appends to 'u.out' chunks for new flaky and
diff --git a/tools/src/cts/expectations/update_test.go b/tools/src/cts/expectations/update_test.go
index f4fa13e..67cf4cc 100644
--- a/tools/src/cts/expectations/update_test.go
+++ b/tools/src/cts/expectations/update_test.go
@@ -139,7 +139,7 @@
`,
results: result.List{
result.Result{
- Query: Q("a:b,c:d"),
+ Query: Q("a:b,c:d:e"),
Tags: result.NewTags("os-a", "os-c", "gpu-b"),
Status: result.Failure,
},
@@ -187,23 +187,23 @@
{ //////////////////////////////////////////////////////////////////////
name: "expectation case now passes",
expectations: `
-crbug.com/a/123 [ gpu-a os-a ] a:b,c:d [ Failure ]
-crbug.com/a/123 [ gpu-b os-b ] a:b,c:d [ Failure ]
+crbug.com/a/123 [ gpu-a os-a ] a:b,c:d:* [ Failure ]
+crbug.com/a/123 [ gpu-b os-b ] a:b,c:d:* [ Failure ]
`,
results: result.List{
result.Result{
- Query: Q("a:b,c:d"),
+ Query: Q("a:b,c:d:*"),
Tags: result.NewTags("os-a", "gpu-a"),
Status: result.Pass,
},
result.Result{
- Query: Q("a:b,c:d"),
+ Query: Q("a:b,c:d:*"),
Tags: result.NewTags("os-b", "gpu-b"),
Status: result.Abort,
},
},
updated: `
-crbug.com/a/123 [ os-b ] a:b,c:d: [ Failure ]
+crbug.com/a/123 [ os-b ] a:b,c:d:* [ Failure ]
`,
diagnostics: expectations.Diagnostics{
{
@@ -214,28 +214,71 @@
},
},
{ //////////////////////////////////////////////////////////////////////
- name: "expectation case now passes KEEP - single",
+ name: "first expectation expands to cover later expectations - no diagnostics",
expectations: `
-# KEEP
-crbug.com/a/123 [ gpu-a os-a ] a:b,c:d [ Failure ]
-crbug.com/a/123 [ gpu-b os-b ] a:b,c:d [ Failure ]
+crbug.com/a/123 [ gpu-a os-a ] a:b,c:d:* [ Failure ]
+crbug.com/a/123 [ gpu-c os-a ] a:b,c:d:* [ Failure ]
`,
results: result.List{
result.Result{
- Query: Q("a:b,c:d"),
+ Query: Q("a:b,c:d:e"),
+ Tags: result.NewTags("gpu-a", "os-a"),
+ Status: result.Failure,
+ },
+ result.Result{
+ Query: Q("a:b,c:d:e"),
+ Tags: result.NewTags("gpu-a", "os-b"),
+ Status: result.Pass,
+ },
+ result.Result{
+ Query: Q("a:b,c:d:e"),
+ Tags: result.NewTags("gpu-b", "os-a"),
+ Status: result.Pass,
+ },
+ result.Result{
+ Query: Q("a:b,c:d:e"),
+ Tags: result.NewTags("gpu-b", "os-b"),
+ Status: result.Pass,
+ },
+ result.Result{
+ Query: Q("a:b,c:d:e"),
+ Tags: result.NewTags("gpu-c", "os-a"),
+ Status: result.Failure,
+ },
+ result.Result{
+ Query: Q("a:b,c:d:e"),
+ Tags: result.NewTags("gpu-c", "os-b"),
+ Status: result.Pass,
+ },
+ },
+ updated: `
+crbug.com/a/123 [ gpu-a os-a ] a:b,c:d:* [ Failure ]
+crbug.com/a/123 [ gpu-c os-a ] a:b,c:d:* [ Failure ]
+`,
+ },
+ { //////////////////////////////////////////////////////////////////////
+ name: "expectation case now passes KEEP - single",
+ expectations: `
+# KEEP
+crbug.com/a/123 [ gpu-a os-a ] a:b,c:d:* [ Failure ]
+crbug.com/a/123 [ gpu-b os-b ] a:b,c:d:* [ Failure ]
+`,
+ results: result.List{
+ result.Result{
+ Query: Q("a:b,c:d:e"),
Tags: result.NewTags("os-a", "gpu-a"),
Status: result.Pass,
},
result.Result{
- Query: Q("a:b,c:d"),
+ Query: Q("a:b,c:d:e"),
Tags: result.NewTags("os-b", "gpu-b"),
Status: result.Abort,
},
},
updated: `
# KEEP
-crbug.com/a/123 [ gpu-a os-a ] a:b,c:d [ Failure ]
-crbug.com/a/123 [ gpu-b os-b ] a:b,c:d [ Failure ]
+crbug.com/a/123 [ gpu-a os-a ] a:b,c:d:* [ Failure ]
+crbug.com/a/123 [ gpu-b os-b ] a:b,c:d:* [ Failure ]
`,
diagnostics: expectations.Diagnostics{
{
