[tools][cts] Fix duplicate expectations
Fixes an edge case where duplicate expectations could be created if two
unique results resulted in the same actual expectation once lower
priority tags were removed.
Change-Id: Ib99ee9caccb2a975c0b0a7b5a47ccd2aca7dff2d
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/204437
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Auto-Submit: Brian Sheedy <bsheedy@google.com>
diff --git a/tools/src/cts/expectations/expectations.go b/tools/src/cts/expectations/expectations.go
index 16a2aa9..9b8ac45 100644
--- a/tools/src/cts/expectations/expectations.go
+++ b/tools/src/cts/expectations/expectations.go
@@ -120,19 +120,7 @@
}
}
for _, expectation := range chunk.Expectations {
- parts := []string{}
- if expectation.Bug != "" {
- parts = append(parts, expectation.Bug)
- }
- if len(expectation.Tags) > 0 {
- parts = append(parts, fmt.Sprintf("[ %v ]", strings.Join(expectation.Tags.List(), " ")))
- }
- parts = append(parts, expectation.Query)
- parts = append(parts, fmt.Sprintf("[ %v ]", strings.Join(expectation.Status, " ")))
- if expectation.Comment != "" {
- parts = append(parts, expectation.Comment)
- }
- if _, err := fmt.Fprintln(w, strings.Join(parts, " ")); err != nil {
+ if _, err := fmt.Fprintln(w, expectation.AsExpectationFileString()); err != nil {
return err
}
}
@@ -172,6 +160,24 @@
return Chunk{comments, expectations}
}
+// AsExpectationFileString returns the human-readable form of the expectation
+// that matches the syntax of the expectation files.
+func (e Expectation) AsExpectationFileString() string {
+ parts := []string{}
+ if e.Bug != "" {
+ parts = append(parts, e.Bug)
+ }
+ if len(e.Tags) > 0 {
+ parts = append(parts, fmt.Sprintf("[ %v ]", strings.Join(e.Tags.List(), " ")))
+ }
+ parts = append(parts, e.Query)
+ parts = append(parts, fmt.Sprintf("[ %v ]", strings.Join(e.Status, " ")))
+ if e.Comment != "" {
+ parts = append(parts, e.Comment)
+ }
+ return strings.Join(parts, " ")
+}
+
// Clone makes a deep-copy of the Expectation.
func (e Expectation) Clone() Expectation {
out := Expectation{
diff --git a/tools/src/cts/expectations/expectations_test.go b/tools/src/cts/expectations/expectations_test.go
index 3790d38..5ee40cd 100644
--- a/tools/src/cts/expectations/expectations_test.go
+++ b/tools/src/cts/expectations/expectations_test.go
@@ -30,7 +30,10 @@
import (
"testing"
+ "dawn.googlesource.com/dawn/tools/src/cts/result"
+
"github.com/google/go-cmp/cmp"
+ "github.com/stretchr/testify/assert"
)
// Tests behavior of Content.Format()
@@ -123,3 +126,50 @@
t.Errorf("Format produced unexpected output: %v", diff)
}
}
+
+func TestExpectationAsExpectationFileString(t *testing.T) {
+ // Full expectation.
+ e := Expectation{
+ Bug: "crbug.com/1234",
+ Tags: result.NewTags("linux", "nvidia"),
+ Query: "query",
+ Status: []string{"Failure", "Slow"},
+ Comment: "# comment",
+ }
+ assert.Equal(t, e.AsExpectationFileString(), "crbug.com/1234 [ linux nvidia ] query [ Failure Slow ] # comment")
+
+ // No bug.
+ e = Expectation{
+ Tags: result.NewTags("linux", "nvidia"),
+ Query: "query",
+ Status: []string{"Failure", "Slow"},
+ Comment: "# comment",
+ }
+ assert.Equal(t, e.AsExpectationFileString(), "[ linux nvidia ] query [ Failure Slow ] # comment")
+
+ // No tags.
+ e = Expectation{
+ Bug: "crbug.com/1234",
+ Tags: result.NewTags(),
+ Query: "query",
+ Status: []string{"Failure", "Slow"},
+ Comment: "# comment",
+ }
+ assert.Equal(t, e.AsExpectationFileString(), "crbug.com/1234 query [ Failure Slow ] # comment")
+
+ // No comment.
+ e = Expectation{
+ Bug: "crbug.com/1234",
+ Tags: result.NewTags("linux", "nvidia"),
+ Query: "query",
+ Status: []string{"Failure", "Slow"},
+ }
+ assert.Equal(t, e.AsExpectationFileString(), "crbug.com/1234 [ linux nvidia ] query [ Failure Slow ]")
+
+ // Minimal expectation.
+ e = Expectation{
+ Query: "query",
+ Status: []string{"Failure", "Slow"},
+ }
+ assert.Equal(t, e.AsExpectationFileString(), "query [ Failure Slow ]")
+}
diff --git a/tools/src/cts/expectations/update.go b/tools/src/cts/expectations/update.go
index 43fe72f..0099751 100644
--- a/tools/src/cts/expectations/update.go
+++ b/tools/src/cts/expectations/update.go
@@ -708,20 +708,29 @@
func (u *updater) resultsToExpectations(results result.List, bug, comment string) []Expectation {
results.Sort()
- out := make([]Expectation, len(results))
- for i, r := range results {
+ out := make([]Expectation, 0, len(results))
+ addedExpectations := container.NewSet[string]()
+ for _, r := range results {
q := r.Query.String()
if r.Query.Target() == query.Tests && !r.Query.IsWildcard() {
// The expectation validator wants a trailing ':' for test queries
q += query.TargetDelimiter
}
- out[i] = Expectation{
+ e := Expectation{
Bug: bug,
Tags: u.in.Tags.RemoveLowerPriorityTags(r.Tags),
Query: q,
Status: []string{string(r.Status)},
Comment: comment,
}
+ key := e.AsExpectationFileString()
+ // We keep track of all expectations we've added so far to avoid cases where
+ // two distinct results create the same expectation due to
+ // RemoveLowerPriorityTags removing the distinguishing tags.
+ if !addedExpectations.Contains(key) {
+ out = append(out, e)
+ addedExpectations.Add(key)
+ }
}
return out
diff --git a/tools/src/cts/expectations/update_test.go b/tools/src/cts/expectations/update_test.go
index aba4925..3075b90 100644
--- a/tools/src/cts/expectations/update_test.go
+++ b/tools/src/cts/expectations/update_test.go
@@ -25,17 +25,17 @@
// 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
+package expectations
import (
"strings"
"testing"
"dawn.googlesource.com/dawn/tools/src/container"
- "dawn.googlesource.com/dawn/tools/src/cts/expectations"
"dawn.googlesource.com/dawn/tools/src/cts/query"
"dawn.googlesource.com/dawn/tools/src/cts/result"
"github.com/google/go-cmp/cmp"
+ "github.com/stretchr/testify/assert"
)
var Q = query.Parse
@@ -55,7 +55,7 @@
expectations string
results result.List
updated string
- diagnostics expectations.Diagnostics
+ diagnostics Diagnostics
err string
}
for _, test := range []Test{
@@ -90,14 +90,14 @@
crbug.com/a/123 a:missing,test,result:* [ Failure ]
crbug.com/a/123 [ tag ] another:missing,test,result:* [ Failure ]
`,
- diagnostics: expectations.Diagnostics{
+ diagnostics: Diagnostics{
{
- Severity: expectations.Note,
+ Severity: Note,
Line: headerLines + 3,
Message: "no results found for query 'a:missing,test,result:*'",
},
{
- Severity: expectations.Note,
+ Severity: Note,
Line: headerLines + 4,
Message: "no results found for query 'another:missing,test,result:*' with tags [tag]",
},
@@ -127,9 +127,9 @@
some:other,test:* [ Failure ]
`,
- diagnostics: expectations.Diagnostics{
+ diagnostics: Diagnostics{
{
- Severity: expectations.Note,
+ Severity: Note,
Line: headerLines + 2,
Message: "no results found for query 'a:missing,test,result:*'",
},
@@ -159,14 +159,14 @@
updated: `
some:other,test:* [ Failure ]
`,
- diagnostics: expectations.Diagnostics{
+ diagnostics: Diagnostics{
{
- Severity: expectations.Warning,
+ Severity: Warning,
Line: headerLines + 3,
Message: "no tests exist with query 'an:unknown,test:*' - removing",
},
{
- Severity: expectations.Warning,
+ Severity: Warning,
Line: headerLines + 4,
Message: "no tests exist with query 'another:unknown:test' - removing",
},
@@ -194,9 +194,9 @@
updated: `
some:other,test:* [ Failure ]
`,
- diagnostics: expectations.Diagnostics{
+ diagnostics: Diagnostics{
{
- Severity: expectations.Warning,
+ Severity: Warning,
Line: headerLines + 2,
Message: "no tests exist with query 'an:unknown,test:*' - removing",
},
@@ -220,9 +220,9 @@
# ##ROLLER_MUTABLE##
a:b,c:* [ Failure ]
`,
- diagnostics: expectations.Diagnostics{
+ diagnostics: Diagnostics{
{
- Severity: expectations.Note,
+ Severity: Note,
Line: headerLines + 4,
Message: "expectation is fully covered by previous expectations",
},
@@ -319,9 +319,9 @@
# ##ROLLER_MUTABLE##
crbug.com/a/123 [ os-b ] a:b,c:* [ Failure ]
`,
- diagnostics: expectations.Diagnostics{
+ diagnostics: Diagnostics{
{
- Severity: expectations.Note,
+ Severity: Note,
Line: headerLines + 4,
Message: "expectation is fully covered by previous expectations",
},
@@ -350,9 +350,9 @@
# ##ROLLER_MUTABLE##
crbug.com/a/123 [ os-b ] a:b,c:d:* [ Failure ]
`,
- diagnostics: expectations.Diagnostics{
+ diagnostics: Diagnostics{
{
- Severity: expectations.Note,
+ Severity: Note,
Line: headerLines + 4,
Message: "expectation is fully covered by previous expectations",
},
@@ -423,9 +423,9 @@
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{
+ diagnostics: Diagnostics{
{
- Severity: expectations.Note,
+ Severity: Note,
Line: headerLines + 2,
Message: "test now passes",
},
@@ -445,9 +445,9 @@
updated: `
crbug.com/a/123 a:b,c:d:* [ Failure ]
`,
- diagnostics: expectations.Diagnostics{
+ diagnostics: Diagnostics{
{
- Severity: expectations.Note,
+ Severity: Note,
Line: headerLines + 2,
Message: "all 4 tests now pass",
},
@@ -787,7 +787,7 @@
`,
},
} {
- ex, err := expectations.Parse("expectations.txt", header+test.expectations)
+ ex, err := Parse("expectations.txt", header+test.expectations)
if err != nil {
t.Fatalf("'%v': expectations.Parse():\n%v", test.name, err)
}
@@ -823,3 +823,134 @@
}
}
}
+
+func createGenericUpdater(t *testing.T) updater {
+ header := `
+# BEGIN TAG HEADER
+# OS
+# tags: [ linux win10 ]
+# GPU
+# tags: [ intel
+# nvidia nvidia-0x2184 ]
+# Driver
+# tags: [ nvidia_ge_31.0.15.4601 nvidia_lt_31.0.15.4601
+# nvidia_ge_535.183.01 nvidia_lt_535.183.01 ]
+# END TAG HEADER
+`
+ inContent, err := Parse("expectations.txt", header)
+ if err != nil {
+ t.Fatalf("Failed to parse expectations: %v", err)
+ }
+
+ u := updater{
+ in: inContent,
+ }
+ return u
+}
+
+// Tests basic result -> expectation conversion.
+func TestResultsToExpectationsBasic(t *testing.T) {
+ results := result.List{
+ {
+ Query: query.Parse("webgpu:shader,execution,memory_layout:read_layout:"),
+ Tags: result.NewTags("linux", "nvidia"),
+ Status: result.Failure,
+ },
+ {
+ Query: query.Parse("webgpu:shader,execution,memory_model,barrier:"),
+ Tags: result.NewTags("win10", "intel"),
+ Status: result.Failure,
+ },
+ }
+
+ expectedOutput := []Expectation{
+ {
+ Bug: "crbug.com/1234",
+ Tags: result.NewTags("linux", "nvidia"),
+ Query: "webgpu:shader,execution,memory_layout:read_layout:",
+ Status: []string{"Failure"},
+ Comment: "comment",
+ },
+ {
+ Bug: "crbug.com/1234",
+ Tags: result.NewTags("win10", "intel"),
+ Query: "webgpu:shader,execution,memory_model,barrier",
+ Status: []string{"Failure"},
+ Comment: "comment",
+ },
+ }
+
+ u := createGenericUpdater(t)
+ output := u.resultsToExpectations(results, "crbug.com/1234", "comment")
+ assert.Equal(t, output, expectedOutput)
+}
+
+// Tests behavior when two unique results end up creating the same expectation
+// due to lower priority tags being removed.
+func TestResultsToExpectationsOverlappingExpectations(t *testing.T) {
+ results := result.List{
+ {
+ Query: query.Parse("webgpu:shader,execution,memory_layout:read_layout:"),
+ Tags: result.NewTags("nvidia", "nvidia-0x2184", "nvidia_ge_31.0.15.4601", "nvidia_lt_535.183.01"),
+ Status: result.Failure,
+ },
+ {
+ Query: query.Parse("webgpu:shader,execution,memory_layout:read_layout:"),
+ Tags: result.NewTags("nvidia", "nvidia-0x2184", "nvidia_lt_31.0.15.4601", "nvidia_lt_535.183.01"),
+ Status: result.Failure,
+ },
+ }
+
+ expectedOutput := []Expectation{
+ {
+ Bug: "crbug.com/1234",
+ Tags: result.NewTags("nvidia-0x2184", "nvidia_lt_535.183.01"),
+ Query: "webgpu:shader,execution,memory_layout:read_layout:",
+ Status: []string{"Failure"},
+ Comment: "comment",
+ },
+ }
+
+ u := createGenericUpdater(t)
+ output := u.resultsToExpectations(results, "crbug.com/1234", "comment")
+ assert.Equal(t, output, expectedOutput)
+}
+
+// Tests behavior related to automatic inclusion of a trailing :.
+func TestResultsToExpectationsTrailingColon(t *testing.T) {
+ results := result.List{
+ // Should automatically have a : added since it's a test query.
+ {
+ Query: query.Parse("webgpu:shader,execution,memory_layout:read_layout"),
+ Tags: result.NewTags("linux", "nvidia"),
+ Status: result.Failure,
+ },
+ // Should not have a : added since it is a wildcard query.
+ {
+ Query: query.Parse("webgpu:shader,execution,*"),
+ Tags: result.NewTags("win10", "intel"),
+ Status: result.Failure,
+ },
+ }
+
+ expectedOutput := []Expectation{
+ {
+ Bug: "crbug.com/1234",
+ Tags: result.NewTags("win10", "intel"),
+ Query: "webgpu:shader,execution,*",
+ Status: []string{"Failure"},
+ Comment: "comment",
+ },
+ {
+ Bug: "crbug.com/1234",
+ Tags: result.NewTags("linux", "nvidia"),
+ Query: "webgpu:shader,execution,memory_layout:read_layout:",
+ Status: []string{"Failure"},
+ Comment: "comment",
+ },
+ }
+
+ u := createGenericUpdater(t)
+ output := u.resultsToExpectations(results, "crbug.com/1234", "comment")
+ assert.Equal(t, output, expectedOutput)
+}
