[tools][cts roll] Rework tag reduction
The following changes attempt to fix issues where result tags can be a subset of another. This is a long-standing issue that causes incorrect expectation updates.
Split cleanupTags() into removeUnknownTags() and RemoveLowerPriorityTags().
removeUnknownTags() is run before tree folding and redundant tag set removal.
RemoveLowerPriorityTags() is now done after tree folding and redundant tag set removal.
This is has to be done later to avoid ambiguities with tag sets which are not mutually exclusive.
More details of the issues being addressed can be found in https://crbug.com/1498681.
An example of the new tag output can be seen in https://dawn-review.googlesource.com/c/dawn/+/159202
Bug: dawn:2198
Bug: dawn:1401
Bug: chromium:1498681
Change-Id: Ife95840dd7a902a48236c1e7d28eab51213612dd
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/158980
Auto-Submit: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Ben Clayton <bclayton@google.com>
diff --git a/tools/src/cmd/cts/roll/roll.go b/tools/src/cmd/cts/roll/roll.go
index c0b1850..14628bd 100644
--- a/tools/src/cmd/cts/roll/roll.go
+++ b/tools/src/cmd/cts/roll/roll.go
@@ -91,6 +91,7 @@
rebuild bool // Rebuild the expectations file from scratch
preserve bool // If false, abandon past roll changes
sendToGardener bool // If true, automatically send to the gardener for review
+ verbose bool
parentSwarmingRunID string
maxAttempts int
}
@@ -119,6 +120,7 @@
flag.BoolVar(&c.flags.rebuild, "rebuild", false, "rebuild the expectation file from scratch")
flag.BoolVar(&c.flags.preserve, "preserve", false, "do not abandon existing rolls")
flag.BoolVar(&c.flags.sendToGardener, "send-to-gardener", false, "send the CL to the WebGPU gardener for review")
+ flag.BoolVar(&c.flags.verbose, "verbose", false, "emit additional logging")
flag.StringVar(&c.flags.parentSwarmingRunID, "parent-swarming-run-id", "", "parent swarming run id. All triggered tasks will be children of this task and will be canceled if the parent is canceled.")
flag.IntVar(&c.flags.maxAttempts, "max-attempts", 3, "number of update attempts before giving up")
return nil, nil
@@ -374,7 +376,11 @@
// Begin main roll loop
for attempt := 0; ; attempt++ {
// Kick builds
- log.Printf("building (attempt %v)...\n", attempt)
+ if attempt == 0 {
+ log.Println("building...")
+ } else {
+ log.Printf("building (retry %v)...\n", attempt)
+ }
builds, err := common.GetOrStartBuildsAndWait(ctx, r.cfg, ps, r.bb, r.parentSwarmingRunID, false)
if err != nil {
return err
@@ -409,7 +415,7 @@
exInfo.results = result.Merge(exInfo.results, psResultsByExecutionMode[exInfo.executionMode])
exInfo.newExpectations = exInfo.expectations.Clone()
- diags, err := exInfo.newExpectations.Update(exInfo.results, testlist)
+ diags, err := exInfo.newExpectations.Update(exInfo.results, testlist, r.flags.verbose)
if err != nil {
return err
}
@@ -440,7 +446,7 @@
}
if attempt >= r.flags.maxAttempts {
- err := fmt.Errorf("CTS failed after %v attempts.\nGiving up", attempt)
+ err := fmt.Errorf("CTS failed after %v retries.\nGiving up", attempt)
r.gerrit.Comment(ps, err.Error(), nil)
return err
}
diff --git a/tools/src/cmd/cts/update/update.go b/tools/src/cmd/cts/update/update.go
index b550e37..e2fd1521c 100644
--- a/tools/src/cmd/cts/update/update.go
+++ b/tools/src/cmd/cts/update/update.go
@@ -63,6 +63,7 @@
results common.ResultSource
expectations arrayFlags
auth authcli.Flags
+ verbose bool
}
}
@@ -77,6 +78,7 @@
func (c *cmd) RegisterFlags(ctx context.Context, cfg common.Config) ([]string, error) {
c.flags.results.RegisterFlags(cfg)
c.flags.auth.Register(flag.CommandLine, auth.DefaultAuthOptions( /* needsCloudScopes */ false))
+ flag.BoolVar(&c.flags.verbose, "verbose", false, "emit additional logging")
flag.Var(&c.flags.expectations, "expectations", "path to CTS expectations file(s) to update")
return nil, nil
}
@@ -145,7 +147,7 @@
if strings.Contains(expectationsFilename, "compat") {
name = "compat"
}
- diag, err := ex.Update(resultsByExecutionMode[name], testlist)
+ diag, err := ex.Update(resultsByExecutionMode[name], testlist, c.flags.verbose)
if err != nil {
return err
}
diff --git a/tools/src/cts/expectations/expectations.go b/tools/src/cts/expectations/expectations.go
index d85ebc5..3b39a3a 100644
--- a/tools/src/cts/expectations/expectations.go
+++ b/tools/src/cts/expectations/expectations.go
@@ -55,34 +55,6 @@
Expectations Expectations // Expectations for the chunk
}
-// Tags holds the tag information parsed in the comments between the
-// 'BEGIN TAG HEADER' and 'END TAG HEADER' markers.
-// Tags are grouped in tag-sets.
-type Tags struct {
- // Map of tag-set name to tags
- Sets []TagSet
- // Map of tag name to tag-set and priority
- ByName map[string]TagSetAndPriority
-}
-
-// TagSet is a named collection of tags, parsed from the 'TAG HEADER'
-type TagSet struct {
- Name string // Name of the tag-set
- Tags result.Tags // Tags belonging to the tag-set
-}
-
-// TagSetAndPriority is used by the Tags.ByName map to identify which tag-set
-// a tag belongs to.
-type TagSetAndPriority struct {
- // The tag-set that the tag belongs to.
- Set string
- // The declared order of tag in the set.
- // An expectation may only list a single tag from any set. This priority
- // is used to decide which tag(s) should be dropped when multiple tags are
- // found in the same set.
- Priority int
-}
-
// Expectation holds a single expectation line
type Expectation struct {
Line int // The 1-based line number of the expectation
@@ -193,22 +165,6 @@
return Chunk{comments, expectations}
}
-// Clone returns a deep-copy of the Tags
-func (t Tags) Clone() Tags {
- out := Tags{}
- if t.ByName != nil {
- out.ByName = make(map[string]TagSetAndPriority, len(t.ByName))
- for n, t := range t.ByName {
- out.ByName[n] = t
- }
- }
- if t.Sets != nil {
- out.Sets = make([]TagSet, len(t.Sets))
- copy(out.Sets, t.Sets)
- }
- return out
-}
-
// Clone makes a deep-copy of the Expectation.
func (e Expectation) Clone() Expectation {
out := Expectation{
diff --git a/tools/src/cts/expectations/tags.go b/tools/src/cts/expectations/tags.go
new file mode 100644
index 0000000..2c934af
--- /dev/null
+++ b/tools/src/cts/expectations/tags.go
@@ -0,0 +1,102 @@
+// Copyright 2023 The Dawn & Tint Authors
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are met:
+//
+// 1. Redistributions of source code must retain the above copyright notice, this
+// list of conditions and the following disclaimer.
+//
+// 2. Redistributions in binary form must reproduce the above copyright notice,
+// this list of conditions and the following disclaimer in the documentation
+// and/or other materials provided with the distribution.
+//
+// 3. Neither the name of the copyright holder nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+// AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+// DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
+// FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+// DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+// SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+// CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+// OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+package expectations
+
+import (
+ "dawn.googlesource.com/dawn/tools/src/container"
+ "dawn.googlesource.com/dawn/tools/src/cts/result"
+)
+
+// Tags holds the tag information parsed in the comments between the
+// 'BEGIN TAG HEADER' and 'END TAG HEADER' markers.
+// Tags are grouped in tag-sets.
+type Tags struct {
+ // Map of tag-set name to tags
+ Sets []TagSet
+ // Map of tag name to tag-set and priority
+ ByName map[string]TagSetAndPriority
+}
+
+// TagSet is a named collection of tags, parsed from the 'TAG HEADER'
+type TagSet struct {
+ Name string // Name of the tag-set
+ Tags result.Tags // Tags belonging to the tag-set
+}
+
+// TagSetAndPriority is used by the Tags.ByName map to identify which tag-set
+// a tag belongs to.
+type TagSetAndPriority struct {
+ // The tag-set that the tag belongs to.
+ Set string
+ // The declared order of tag in the set.
+ // An expectation may only list a single tag from any set. This priority
+ // is used to decide which tag(s) should be dropped when multiple tags are
+ // found in the same set.
+ Priority int
+}
+
+// Clone returns a deep-copy of the Tags
+func (t Tags) Clone() Tags {
+ out := Tags{}
+ if t.ByName != nil {
+ out.ByName = make(map[string]TagSetAndPriority, len(t.ByName))
+ for n, t := range t.ByName {
+ out.ByName[n] = t
+ }
+ }
+ if t.Sets != nil {
+ out.Sets = make([]TagSet, len(t.Sets))
+ copy(out.Sets, t.Sets)
+ }
+ return out
+}
+
+// RemoveLowerPriorityTags returns a copy of the provided tags with only the
+// highest priority tag of each tag set.
+func (t Tags) RemoveLowerPriorityTags(tags container.Set[string]) container.Set[string] {
+ type TagPriority struct {
+ tag string
+ priority int
+ }
+ highestPriorty := container.NewMap[string, *TagPriority]()
+ for tag := range tags {
+ info := t.ByName[tag]
+ tp := highestPriorty[info.Set]
+ if tp == nil {
+ tp = &TagPriority{tag: tag, priority: info.Priority}
+ highestPriorty[info.Set] = tp
+ } else if tp.priority < info.Priority {
+ tp.tag, tp.priority = tag, info.Priority
+ }
+ }
+ out := container.Set[string]{}
+ for _, tp := range highestPriorty {
+ out.Add(tp.tag)
+ }
+ return out
+}
diff --git a/tools/src/cts/expectations/tags_test.go b/tools/src/cts/expectations/tags_test.go
new file mode 100644
index 0000000..abea991
--- /dev/null
+++ b/tools/src/cts/expectations/tags_test.go
@@ -0,0 +1,86 @@
+// Copyright 2023 The Dawn & Tint Authors
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are met:
+//
+// 1. Redistributions of source code must retain the above copyright notice, this
+// list of conditions and the following disclaimer.
+//
+// 2. Redistributions in binary form must reproduce the above copyright notice,
+// this list of conditions and the following disclaimer in the documentation
+// and/or other materials provided with the distribution.
+//
+// 3. Neither the name of the copyright holder nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+// AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+// DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
+// FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+// DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+// SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+// CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+// OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+package expectations_test
+
+import (
+ "testing"
+
+ "dawn.googlesource.com/dawn/tools/src/container"
+ "dawn.googlesource.com/dawn/tools/src/cts/expectations"
+ "github.com/google/go-cmp/cmp"
+)
+
+func TestRemoveLowerPriorityTags(t *testing.T) {
+ tags := expectations.Tags{
+ Sets: []expectations.TagSet{
+ {
+ Name: "OS",
+ Tags: container.NewSet(
+ "os_a",
+ "os_b",
+ "os_c",
+ ),
+ },
+ {
+ Name: "GPU",
+ Tags: container.NewSet(
+ "gpu_a",
+ "gpu_b",
+ "gpu_c",
+ ),
+ },
+ },
+ ByName: map[string]expectations.TagSetAndPriority{
+ "os_a": {Set: "OS", Priority: 0},
+ "os_b": {Set: "OS", Priority: 1},
+ "os_c": {Set: "OS", Priority: 2},
+ "gpu_a": {Set: "GPU", Priority: 0},
+ "gpu_b": {Set: "GPU", Priority: 1},
+ "gpu_c": {Set: "GPU", Priority: 2},
+ },
+ }
+ type Test struct {
+ in []string
+ expected []string
+ }
+ for _, test := range []Test{
+ {in: []string{"os_a"}, expected: []string{"os_a"}},
+ {in: []string{"gpu_b"}, expected: []string{"gpu_b"}},
+ {in: []string{"gpu_b", "os_a"}, expected: []string{"gpu_b", "os_a"}},
+ {in: []string{"gpu_a", "gpu_b"}, expected: []string{"gpu_b"}},
+ {in: []string{"gpu_b", "gpu_c"}, expected: []string{"gpu_c"}},
+ {in: []string{"os_a", "os_b"}, expected: []string{"os_b"}},
+ {in: []string{"os_b", "os_c"}, expected: []string{"os_c"}},
+ {in: []string{"gpu_a", "gpu_c", "os_b", "os_c"}, expected: []string{"gpu_c", "os_c"}},
+ } {
+ got := tags.RemoveLowerPriorityTags(container.NewSet(test.in...)).List()
+ if diff := cmp.Diff(got, test.expected); diff != "" {
+ t.Errorf("TestRemoveLowerPriorityTags(%v) returned %v:\n%v", test.in, got, diff)
+ }
+ }
+}
diff --git a/tools/src/cts/expectations/update.go b/tools/src/cts/expectations/update.go
index 2e9fa8c..53d4ed7 100644
--- a/tools/src/cts/expectations/update.go
+++ b/tools/src/cts/expectations/update.go
@@ -57,7 +57,7 @@
// Note: Validate() should be called before attempting to update the
// expectations. If Validate() returns errors, then Update() behaviour is
// undefined.
-func (c *Content) Update(results result.List, testlist []query.Query) (Diagnostics, error) {
+func (c *Content) Update(results result.List, testlist []query.Query, verbose bool) (Diagnostics, error) {
// Make a copy of the results. This code mutates the list.
results = append(result.List{}, results...)
@@ -77,6 +77,13 @@
// (unique tag combinations).
variants := results.Variants()
+ if verbose {
+ fmt.Println("result variants:")
+ for i, tags := range variants {
+ fmt.Printf(" (%.2d) %v\n", i, tags.List())
+ }
+ }
+
// Add 'consumed' results for tests that were skipped.
// This ensures that skipped results are not included in reduced trees.
results = c.appendConsumedResultsForSkippedTests(results, testlist, variants)
@@ -514,7 +521,8 @@
}
// Build a tree from the results matching the given variant.
- tree, err := u.qt.results.FilterByVariant(variant).StatusTree()
+ filtered := u.qt.results.FilterByVariant(variant)
+ tree, err := filtered.StatusTree()
if err != nil {
return fmt.Errorf("while building tree for tags '%v': %w", variant, err)
}
@@ -522,8 +530,10 @@
tree.Reduce(treeReducer)
// Add all the reduced leaf nodes to 'roots'.
for _, qd := range tree.List() {
- // Use Split() to ensure that only the leaves have data (true) in the tree
- roots.Split(qd.Query, true)
+ if qd.Data != result.Pass {
+ // Use Split() to ensure that only the leaves have data (true) in the tree
+ roots.Split(qd.Query, true)
+ }
}
}
@@ -597,7 +607,7 @@
// Using the full list of unfiltered tests, generate the minimal set of
// variants (tags) that uniquely classify the results with differing status.
minimalVariants := u.
- cleanupTags(results).
+ removeUnknownTags(results).
MinimalVariantTags(u.tagSets)
// For each minimized variant...
@@ -659,7 +669,7 @@
}
out[i] = Expectation{
Bug: bug,
- Tags: r.Tags,
+ Tags: u.in.Tags.RemoveLowerPriorityTags(r.Tags),
Query: q,
Status: []string{string(r.Status)},
Comment: comment,
@@ -669,32 +679,17 @@
return out
}
-// cleanupTags returns a copy of the provided results with:
-// - All tags not found in the expectations list removed
-// - All but the highest priority tag for any tag-set.
-// The tag sets are defined by the `BEGIN TAG HEADER` / `END TAG HEADER`
-// section at the top of the expectations file.
-func (u *updater) cleanupTags(results result.List) result.List {
+// removeUnknownTags returns a copy of the provided results with all tags not
+// found in the expectations list removed
+func (u *updater) removeUnknownTags(results result.List) result.List {
return results.TransformTags(func(t result.Tags) result.Tags {
- type HighestPrioritySetTag struct {
- tag string
- priority int
- }
- // Set name to highest priority tag for that set
- best := map[string]HighestPrioritySetTag{}
+ filtered := result.NewTags()
for tag := range t {
- sp, ok := u.in.Tags.ByName[tag]
- if ok {
- if set := best[sp.Set]; sp.Priority >= set.priority {
- best[sp.Set] = HighestPrioritySetTag{tag, sp.Priority}
- }
+ if _, ok := u.in.Tags.ByName[tag]; ok {
+ filtered.Add(tag)
}
}
- t = result.NewTags()
- for _, ts := range best {
- t.Add(ts.tag)
- }
- return t
+ return filtered
})
}
diff --git a/tools/src/cts/expectations/update_test.go b/tools/src/cts/expectations/update_test.go
index a4d3cb7..efd2f50 100644
--- a/tools/src/cts/expectations/update_test.go
+++ b/tools/src/cts/expectations/update_test.go
@@ -378,8 +378,8 @@
################################################################################
# New failures. Please triage:
################################################################################
-crbug.com/dawn/0000 [ gpu-b os-c ] a:* [ Failure ]
crbug.com/dawn/0000 [ gpu-c os-b ] a:* [ Failure ]
+crbug.com/dawn/0000 [ gpu-b os-c ] a:* [ Failure ]
`,
},
{ //////////////////////////////////////////////////////////////////////
@@ -535,7 +535,7 @@
}
errMsg := ""
- diagnostics, err := ex.Update(test.results, testList.Values())
+ diagnostics, err := ex.Update(test.results, testList.Values() /* verbose */, false)
if err != nil {
errMsg = err.Error()
}
