From e822f5e01666300d7d36948a47ec7d29a4ddf6b3 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Fri, 29 Apr 2022 19:09:17 +0000 Subject: [PATCH] tools: Add 'Duration' field to cts.Result. Will help identify slow tests. Bug: dawn:1342 Change-Id: I422d345361785addcc2faa6281e0608de02629b7 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/88316 Kokoro: Kokoro Reviewed-by: Corentin Wallez --- tools/src/cts/result/result.go | 143 ++++++++++++++++++---------- tools/src/cts/result/result_test.go | 58 ++++++----- tools/src/resultsdb/resultsdb.go | 2 +- 3 files changed, 126 insertions(+), 77 deletions(-) diff --git a/tools/src/cts/result/result.go b/tools/src/cts/result/result.go index e234c42df3..623a90611b 100644 --- a/tools/src/cts/result/result.go +++ b/tools/src/cts/result/result.go @@ -23,6 +23,7 @@ import ( "path/filepath" "sort" "strings" + "time" "dawn.googlesource.com/dawn/tools/src/container" "dawn.googlesource.com/dawn/tools/src/cts/query" @@ -30,9 +31,10 @@ import ( // Result holds the result of a CTS test type Result struct { - Query query.Query - Tags Tags - Status Status + Query query.Query + Tags Tags + Status Status + Duration time.Duration } // Format writes the Result to the fmt.State @@ -41,9 +43,9 @@ type Result struct { // This matches the order in which results are sorted. func (r Result) Format(f fmt.State, verb rune) { if len(r.Tags) > 0 { - fmt.Fprintf(f, "%v %v %v", r.Query, TagsToString(r.Tags), r.Status) + fmt.Fprintf(f, "%v %v %v %v", r.Query, TagsToString(r.Tags), r.Status, r.Duration) } else { - fmt.Fprintf(f, "%v %v", r.Query, r.Status) + fmt.Fprintf(f, "%v %v %v", r.Query, r.Status, r.Duration) } } @@ -54,6 +56,34 @@ func (r Result) String() string { return sb.String() } +// Compare compares the relative order of r and o, returning: +// -1 if r should come before o +// 1 if r should come after o +// 0 if r and o are identical +// Note: Result.Duration is not considered in comparison. +func (r Result) Compare(o Result) int { + a, b := r, o + switch a.Query.Compare(b.Query) { + case -1: + return -1 + case 1: + return 1 + } + ta := strings.Join(a.Tags.List(), TagDelimiter) + tb := strings.Join(b.Tags.List(), TagDelimiter) + switch { + case ta < tb: + return -1 + case ta > tb: + return 1 + case a.Status < b.Status: + return -1 + case a.Status > b.Status: + return 1 + } + return 0 +} + // Parse parses the result from a string of the form: // // may be omitted if there were no tags. @@ -81,17 +111,29 @@ func Parse(in string) (Result, error) { a := token() b := token() c := token() - if a == "" || b == "" || token() != "" { + d := token() + if a == "" || b == "" || c == "" || token() != "" { return Result{}, fmt.Errorf("unable to parse result '%v'", in) } - q := query.Parse(a) - if c == "" { + + query := query.Parse(a) + + if d == "" { status := Status(b) - return Result{q, nil, status}, nil + duration, err := time.ParseDuration(c) + if err != nil { + return Result{}, fmt.Errorf("unable to parse result '%v': %w", in, err) + } + return Result{query, nil, status, duration}, nil + } else { + tags := StringToTags(b) + status := Status(c) + duration, err := time.ParseDuration(d) + if err != nil { + return Result{}, fmt.Errorf("unable to parse result '%v': %w", in, err) + } + return Result{query, tags, status, duration}, nil } - tags := StringToTags(b) - status := Status(c) - return Result{q, tags, status}, nil } // List is a list of results @@ -123,9 +165,10 @@ func (l List) TransformTags(f func(Tags) Tags) List { cache[key] = tags } out = append(out, Result{ - Query: r.Query, - Tags: tags, - Status: r.Status, + Query: r.Query, + Tags: tags, + Status: r.Status, + Duration: r.Duration, }) } return out @@ -134,39 +177,54 @@ func (l List) TransformTags(f func(Tags) Tags) List { // ReplaceDuplicates returns a new list with duplicate test results replaced. // When a duplicate is found, the function f is called with the duplicate // results. The returned status will be used as the replaced result. +// Merged results will use the average (mean) duration of the duplicates. func (l List) ReplaceDuplicates(f func(Statuses) Status) List { type key struct { query query.Query tags string } // Collect all duplicates - duplicates := map[key]Statuses{} - for _, r := range l { + keyToIndices := map[key][]int{} // key to index + for i, r := range l { k := key{r.Query, TagsToString(r.Tags)} - if s, ok := duplicates[k]; ok { - s.Add(r.Status) - } else { - duplicates[k] = NewStatuses(r.Status) - } + keyToIndices[k] = append(keyToIndices[k], i) } // Resolve duplicates - merged := map[key]Status{} - for key, statuses := range duplicates { - if len(statuses) > 1 { - merged[key] = f(statuses) - } else { - merged[key] = statuses.One() // Only one status + type StatusAndDuration struct { + Status Status + Duration time.Duration + } + merged := map[key]StatusAndDuration{} + for key, indices := range keyToIndices { + statuses := NewStatuses() + duration := time.Duration(0) + for _, i := range indices { + r := l[i] + statuses.Add(r.Status) + duration += r.Duration + } + status := func() Status { + if len(statuses) > 1 { + return f(statuses) + } + return statuses.One() + }() + duration = duration / time.Duration(len(indices)) + merged[key] = StatusAndDuration{ + Status: status, + Duration: duration, } } // Rebuild list - out := make(List, 0, len(duplicates)) + out := make(List, 0, len(keyToIndices)) for _, r := range l { k := key{r.Query, TagsToString(r.Tags)} - if status, ok := merged[k]; ok { + if sd, ok := merged[k]; ok { out = append(out, Result{ - Query: r.Query, - Tags: r.Tags, - Status: status, + Query: r.Query, + Tags: r.Tags, + Status: sd.Status, + Duration: sd.Duration, }) delete(merged, k) // Remove from map to prevent duplicates } @@ -176,24 +234,7 @@ func (l List) ReplaceDuplicates(f func(Statuses) Status) List { // Sort sorts the list func (l List) Sort() { - sort.Slice(l, func(i, j int) bool { - a, b := l[i], l[j] - switch a.Query.Compare(b.Query) { - case -1: - return true - case 1: - return false - } - ta := strings.Join(a.Tags.List(), TagDelimiter) - tb := strings.Join(b.Tags.List(), TagDelimiter) - switch { - case ta < tb: - return true - case ta > tb: - return false - } - return a.Status < b.Status - }) + sort.Slice(l, func(i, j int) bool { return l[i].Compare(l[j]) < 0 }) } // Filter returns the results that match the given predicate diff --git a/tools/src/cts/result/result_test.go b/tools/src/cts/result/result_test.go index bd1eb27f29..946dbbbff9 100644 --- a/tools/src/cts/result/result_test.go +++ b/tools/src/cts/result/result_test.go @@ -16,8 +16,8 @@ package result_test import ( "bytes" - "fmt" "testing" + "time" "dawn.googlesource.com/dawn/tools/src/container" "dawn.googlesource.com/dawn/tools/src/cts/query" @@ -40,25 +40,28 @@ func TestStringAndParse(t *testing.T) { for _, test := range []Test{ { result.Result{ - Query: Q(`a`), - Status: result.Failure, + Query: Q(`a`), + Status: result.Failure, + Duration: time.Second * 42, }, - `a Failure`, + `a Failure 42s`, }, { result.Result{ - Query: Q(`a:b,c,*`), - Tags: T("x"), - Status: result.Pass, + Query: Q(`a:b,c,*`), + Tags: T("x"), + Status: result.Pass, + Duration: time.Second * 42, }, - `a:b,c,* x Pass`, + `a:b,c,* x Pass 42s`, }, { result.Result{ - Query: Q(`a:b,c:d,*`), - Tags: T("zzz", "x", "yy"), - Status: result.Failure, + Query: Q(`a:b,c:d,*`), + Tags: T("zzz", "x", "yy"), + Status: result.Failure, + Duration: time.Second * 42, }, - `a:b,c:d,* x,yy,zzz Failure`, + `a:b,c:d,* x,yy,zzz Failure 42s`, }, } { if diff := cmp.Diff(test.result.String(), test.expect); diff != "" { @@ -77,15 +80,20 @@ func TestStringAndParse(t *testing.T) { } func TestParseError(t *testing.T) { - for _, test := range []string{ - ``, - `a`, - `a b c d`, + for _, test := range []struct { + in, expect string + }{ + {``, `unable to parse result ''`}, + {`a`, `unable to parse result 'a'`}, + {`a b c d`, `unable to parse result 'a b c d': time: invalid duration "d"`}, } { - _, err := result.Parse(test) - expect := fmt.Sprintf(`unable to parse result '%v'`, test) - if err == nil || err.Error() != expect { - t.Errorf("Parse('%v') returned '%v'", test, err) + _, err := result.Parse(test.in) + got := "" + if err != nil { + got = err.Error() + } + if diff := cmp.Diff(got, test.expect); diff != "" { + t.Errorf("Parse('%v'): %v", test, diff) continue } } @@ -315,26 +323,26 @@ func TestReplaceDuplicates(t *testing.T) { { ////////////////////////////////////////////////////////////////////// location: utils.ThisLine(), results: result.List{ - result.Result{Query: Q(`a`), Status: result.Pass}, + result.Result{Query: Q(`a`), Status: result.Pass, Duration: 1}, }, fn: func(result.Statuses) result.Status { return result.Abort }, expect: result.List{ - result.Result{Query: Q(`a`), Status: result.Pass}, + result.Result{Query: Q(`a`), Status: result.Pass, Duration: 1}, }, }, { ////////////////////////////////////////////////////////////////////// location: utils.ThisLine(), results: result.List{ - result.Result{Query: Q(`a`), Status: result.Pass}, - result.Result{Query: Q(`a`), Status: result.Pass}, + result.Result{Query: Q(`a`), Status: result.Pass, Duration: 1}, + result.Result{Query: Q(`a`), Status: result.Pass, Duration: 3}, }, fn: func(result.Statuses) result.Status { return result.Abort }, expect: result.List{ - result.Result{Query: Q(`a`), Status: result.Pass}, + result.Result{Query: Q(`a`), Status: result.Pass, Duration: 2}, }, }, { ////////////////////////////////////////////////////////////////////// diff --git a/tools/src/resultsdb/resultsdb.go b/tools/src/resultsdb/resultsdb.go index a5ad765406..b484f52a3c 100644 --- a/tools/src/resultsdb/resultsdb.go +++ b/tools/src/resultsdb/resultsdb.go @@ -72,7 +72,7 @@ func (r *ResultsDB) QueryTestResults( TestIdRegexp: filterRegex, }, ReadMask: &fieldmaskpb.FieldMask{Paths: []string{ - "test_id", "status", "tags", "failure_reason", + "test_id", "status", "tags", "failure_reason", "duration", }}, PageSize: 1000, // Maximum page size. PageToken: pageToken,