[tools][cts] Substantial roller changes

Avoid collisions by processing 'KEEP' blocks before non-keep. The former cannot be changed by the tool, so if expectations are added before the KEEP block is seen, then we'd end up with a collision.

Simplify the expectation parser. Removes the concept of an 'empty' expectation chunk. This results in the expectation file with double-newlines being folded to a single newline, but simplifies the code a bunch.

Remove tag validation of `expectations.txt`. This had false-positives, as you simply cannot statically know whether two tag sets collide, as proper collision detection requires knowledge of the variants being run. We now just rely on typ's collision detection.

Remove emission of 'SLOW' tests. These are now in a separate expectation file, and will fail validation if emitted into `expectations.txt`.

Change the pattern matching of 'untriaged failures' to be more lenient. This is required to match the new style in the expectations file. Also wrap the comment in a horizontal `###` line.

More aggressively collapse nodes with partially failing children to reduce expectation spam.

Add a bunch more logging and add progress bars for the expensive operations.

Change-Id: Ie14cb89790ed32f0e3f3721be5b573113611edc0
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/151921
Reviewed-by: Antonio Maiorano <[email protected]>
Auto-Submit: Ben Clayton <[email protected]>
Kokoro: Kokoro <[email protected]>
Commit-Queue: Ben Clayton <[email protected]>

Author: ben-clayton

tools/src/cmd/cts/common/config.go

@@ -19,7 +19,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
-	"time"
 
 	"dawn.googlesource.com/dawn/tools/src/buildbucket"
 	"github.com/tidwall/jsonc"
@@ -33,8 +32,6 @@ type Config struct {
 	Test struct {
 		// The ResultDB string prefix for CTS tests.
 		Prefixes []string
-		// The time threshold used to classify tests as slow.
-		SlowThreshold time.Duration
 	}
 	// Gerrit holds configuration for Dawn's Gerrit server.
 	Gerrit struct {

tools/src/cmd/cts/common/results.go

@@ -126,7 +126,6 @@ func (r *ResultSource) GetResults(ctx context.Context, cfg Config, auth auth.Opt
 
 	// Obtain the patchset's results, kicking a build if there are no results
 	// already available.
-	log.Printf("fetching results from cl %v ps %v...", ps.Change, ps.Patchset)
 	builds, err := GetOrStartBuildsAndWait(ctx, cfg, *ps, bb, "", false)
 	if err != nil {
 		return nil, err
@@ -157,11 +156,13 @@ func CacheResults(
 		dir := fileutils.ExpandHome(cacheDir)
 		path := filepath.Join(dir, strconv.Itoa(ps.Change), fmt.Sprintf("ps-%v.txt", ps.Patchset))
 		if _, err := os.Stat(path); err == nil {
+			log.Printf("loading cached results from cl %v ps %v...", ps.Change, ps.Patchset)
 			return result.Load(path)
 		}
 		cachePath = path
 	}
 
+	log.Printf("fetching results from cl %v ps %v...", ps.Change, ps.Patchset)
 	results, err := GetResults(ctx, cfg, rdb, builds)
 	if err != nil {
 		return nil, err
@@ -241,10 +242,6 @@ func GetResults(
 				}
 			}
 
-			if status == result.Pass && duration > cfg.Test.SlowThreshold {
-				status = result.Slow
-			}
-
 			results = append(results, result.Result{
 				Query:        query.Parse(testName),
 				Status:       status,

tools/src/cmd/cts/roll/roll.go

@@ -77,7 +77,8 @@ type rollerFlags struct {
 	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
-	parentSwarmingRunId string
+	parentSwarmingRunID string
+	maxAttempts         int
 }
 
 type cmd struct {
@@ -105,7 +106,8 @@ func (c *cmd) RegisterFlags(ctx context.Context, cfg common.Config) ([]string, e
 	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.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.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
 }
 
@@ -169,7 +171,7 @@ func (c *cmd) Run(ctx context.Context, cfg common.Config) error {
 		flags:               c.flags,
 		auth:                auth,
 		bb:                  bb,
-		parentSwarmingRunId: c.flags.parentSwarmingRunId,
+		parentSwarmingRunID: c.flags.parentSwarmingRunID,
 		rdb:                 rdb,
 		git:                 git,
 		gerrit:              gerrit,
@@ -185,7 +187,7 @@ type roller struct {
 	flags               rollerFlags
 	auth                auth.Options
 	bb                  *buildbucket.Buildbucket
-	parentSwarmingRunId string
+	parentSwarmingRunID string
 	rdb                 *resultsdb.ResultsDB
 	git                 *git.Git
 	gerrit              *gerrit.Gerrit
@@ -237,16 +239,13 @@ func (r *roller) roll(ctx context.Context) error {
 		return fmt.Errorf("failed to load expectations: %v", err)
 	}
 
-	// If the user requested a full rebuild of the expecations, strip out
+	// If the user requested a full rebuild of the expectations, strip out
 	// everything but comment chunks.
 	if r.flags.rebuild {
 		rebuilt := ex.Clone()
 		rebuilt.Chunks = rebuilt.Chunks[:0]
 		for _, c := range ex.Chunks {
-			switch {
-			case c.IsBlankLine():
-				rebuilt.MaybeAddBlankLine()
-			case c.IsCommentOnly():
+			if c.IsCommentOnly() {
 				rebuilt.Chunks = append(rebuilt.Chunks, c)
 			}
 		}
@@ -333,12 +332,11 @@ func (r *roller) roll(ctx context.Context) error {
 	}
 
 	// Begin main roll loop
-	const maxAttempts = 3
 	results := result.List{}
 	for attempt := 0; ; attempt++ {
 		// Kick builds
 		log.Printf("building (attempt %v)...\n", attempt)
-		builds, err := common.GetOrStartBuildsAndWait(ctx, r.cfg, ps, r.bb, r.parentSwarmingRunId, false)
+		builds, err := common.GetOrStartBuildsAndWait(ctx, r.cfg, ps, r.bb, r.parentSwarmingRunID, false)
 		if err != nil {
 			return err
 		}
@@ -397,7 +395,7 @@ func (r *roller) roll(ctx context.Context) error {
 			return fmt.Errorf("failed to update change '%v': %v", changeID, err)
 		}
 
-		if attempt >= maxAttempts {
+		if attempt >= r.flags.maxAttempts {
 			err := fmt.Errorf("CTS failed after %v attempts.\nGiving up", attempt)
 			r.gerrit.Comment(ps, err.Error(), nil)
 			return err
@@ -417,10 +415,10 @@ func (r *roller) roll(ctx context.Context) error {
 			return err
 		}
 
-		type StructuredJsonResponse struct {
+		type StructuredJSONResponse struct {
 			Emails []string
 		}
-		var jsonRes StructuredJsonResponse
+		var jsonRes StructuredJSONResponse
 		if err := json.Unmarshal(jsonResponse, &jsonRes); err != nil {
 			return err
 		}

tools/src/cmd/cts/update/update.go

@@ -19,6 +19,7 @@ import (
 	"flag"
 	"fmt"
 	"io/ioutil"
+	"log"
 	"os"
 	"strings"
 
@@ -79,31 +80,37 @@ func (c *cmd) Run(ctx context.Context, cfg common.Config) error {
 	}
 
 	// Fetch the results
+	log.Println("fetching results...")
 	results, err := c.flags.results.GetResults(ctx, cfg, auth)
 	if err != nil {
 		return err
 	}
 
 	// Merge to remove duplicates
+	log.Println("removing duplicate results...")
 	results = result.Merge(results)
 
 	// Load the expectations file
+	log.Println("loading expectations...")
 	ex, err := expectations.Load(c.flags.expectations)
 	if err != nil {
 		return err
 	}
 
+	log.Println("loading test list...")
 	testlist, err := loadTestList(common.DefaultTestListPath())
 	if err != nil {
 		return err
 	}
 
+	log.Println("validating...")
 	if diag := ex.Validate(); diag.NumErrors() > 0 {
 		diag.Print(os.Stdout, c.flags.expectations)
 		return fmt.Errorf("validation failed")
 	}
 
 	// Update the expectations file with the results
+	log.Println("updating expectations...")
 	diag, err := ex.Update(results, testlist)
 	if err != nil {
 		return err

tools/src/cts/expectations/expectations.go

@@ -21,7 +21,6 @@ package expectations
 import (
 	"fmt"
 	"io"
-	"io/ioutil"
 	"os"
 	"sort"
 	"strings"
@@ -86,7 +85,7 @@ type Expectations []Expectation
 
 // Load loads the expectation file at 'path', returning a Content.
 func Load(path string) (Content, error) {
-	content, err := ioutil.ReadFile(path)
+	content, err := os.ReadFile(path)
 	if err != nil {
 		return Content{}, err
 	}
@@ -122,27 +121,13 @@ func (c Content) Empty() bool {
 	return len(c.Chunks) == 0
 }
 
-// EndsInBlankLine returns true if the Content ends with a blank line
-func (c Content) EndsInBlankLine() bool {
-	return !c.Empty() && c.Chunks[len(c.Chunks)-1].IsBlankLine()
-}
-
-// MaybeAddBlankLine appends a new blank line to the content, if the content
-// does not already end in a blank line.
-func (c *Content) MaybeAddBlankLine() {
-	if !c.Empty() && !c.EndsInBlankLine() {
-		c.Chunks = append(c.Chunks, Chunk{})
-	}
-}
-
 // Write writes the Content, in textual form, to the writer w.
 func (c Content) Write(w io.Writer) error {
-	for _, chunk := range c.Chunks {
-		if len(chunk.Comments) == 0 && len(chunk.Expectations) == 0 {
+	for i, chunk := range c.Chunks {
+		if i > 0 {
 			if _, err := fmt.Fprintln(w); err != nil {
 				return err
 			}
-			continue
 		}
 		for _, comment := range chunk.Comments {
 			if _, err := fmt.Fprintln(w, comment); err != nil {
@@ -182,11 +167,6 @@ func (c Chunk) IsCommentOnly() bool {
 	return len(c.Comments) > 0 && len(c.Expectations) == 0
 }
 
-// IsBlankLine returns true if the Chunk has no comments or expectations.
-func (c Chunk) IsBlankLine() bool {
-	return len(c.Comments) == 0 && len(c.Expectations) == 0
-}
-
 // Clone returns a deep-copy of the Chunk
 func (c Chunk) Clone() Chunk {
 	comments := make([]string, len(c.Comments))

tools/src/cts/expectations/parse.go

@@ -73,7 +73,6 @@ func Parse(path, body string) (Content, error) {
 		if i > 0 {
 			switch {
 			case
-				lastLineType == blank && lineType != blank,             // blank -> !blank
 				lastLineType != blank && lineType == blank,             // !blank -> blank
 				lastLineType == expectation && lineType != expectation: // expectation -> comment
 				flush()

tools/src/cts/expectations/parse_test.go

@@ -60,7 +60,6 @@ func TestParse(t *testing.T) {
 # a comment`,
 			expect: expectations.Content{
 				Chunks: []expectations.Chunk{
-					{},
 					{Comments: []string{`# a comment`}},
 				},
 			},
@@ -70,7 +69,6 @@ func TestParse(t *testing.T) {
 			in:   "\r\n# a comment",
 			expect: expectations.Content{
 				Chunks: []expectations.Chunk{
-					{},
 					{Comments: []string{`# a comment`}},
 				},
 			},
@@ -98,7 +96,6 @@ func TestParse(t *testing.T) {
 			expect: expectations.Content{
 				Chunks: []expectations.Chunk{
 					{Comments: []string{`# comment 1`}},
-					{},
 					{Comments: []string{`# comment 2`}},
 				},
 			},
@@ -113,7 +110,6 @@ func TestParse(t *testing.T) {
 			expect: expectations.Content{
 				Chunks: []expectations.Chunk{
 					{Comments: []string{`# comment 1`}},
-					{},
 					{Comments: []string{`# comment 2`}},
 				},
 			},
@@ -334,7 +330,6 @@ crbug.com/123 abc_def [ Skip ]
 							},
 						},
 					},
-					{},
 					{Comments: []string{`### comment 2`}},
 				},
 			},
@@ -358,9 +353,7 @@ crbug.com/456 [ Mac ] ghi_jkl [ PASS ]
 			expect: expectations.Content{
 				Chunks: []expectations.Chunk{
 					{Comments: []string{`# comment 1`}},
-					{},
 					{Comments: []string{`# comment 2`, `# comment 3`}},
-					{},
 					{
 						Expectations: []expectations.Expectation{
 							{
@@ -372,7 +365,6 @@ crbug.com/456 [ Mac ] ghi_jkl [ PASS ]
 							},
 						},
 					},
-					{},
 					{
 						Comments: []string{`# comment 4`, `# comment 5`},
 						Expectations: []expectations.Expectation{
@@ -386,7 +378,6 @@ crbug.com/456 [ Mac ] ghi_jkl [ PASS ]
 						},
 					},
 					{Comments: []string{`# comment 6`}},
-					{},
 					{Comments: []string{`# comment 7`}},
 				},
 			},
@@ -409,7 +400,6 @@ crbug.com/456 [ Mac ] ghi_jkl [ PASS ]
 `,
 			expect: expectations.Content{
 				Chunks: []expectations.Chunk{
-					{},
 					{Comments: []string{
 						`# BEGIN TAG HEADER (autogenerated, see validate_tag_consistency.py)`,
 						`# Devices`,