preprocessor: refactor sanitiser and comparator code sharing

In a later CL, we introduce script node per-statement sanitisers by
means of a tag directive (located in the doc comment for a statement).
This requires us to use a sanitiser for its sanitising capabilities, but
does not require us to use the "match against this command" capability.

As such, our reuse pattern of different capabilities is a bit upside
down, for both sanitiser and comparators.

Refactor to extract an explicit matcher interface, distinct from
sanitiser/comparator. This allows a sanitiser/comparator to only be
responsible for its principle task.

A patternSanitiser is just that: a sanitiser. The newly introduced
patternSanitiserMatcher is a pattern sanitiser that includes a
specification of which commands it should be run against.

The same refactor is applied for comparators.

Corresponding changes are made in the CUE schema for the preprocessor.
Some of the naming is likely a bit off, but I think I need to see/work
with it a bit to work out where it's good/bad and rework in a later CL.

Preprocessor-No-Write-Cache: true
Signed-off-by: Paul Jolly <[email protected]>
Change-Id: Id5dad562e992c1fdbb505b958597c896861845cc
Dispatch-Trailer: {"type":"trybot","CL":1176700,"patchset":7,"ref":"refs/changes/00/1176700/7","targetBranch":"alpha"}

internal/cmd/preprocessor/cmd/comparators.go

@@ -26,22 +26,41 @@ import (
 // such command outputs that differ only in known ways to be compared as
 // bytes-equal after normalization.
 type comparator interface {
+	init() error
 	normalize(cmd *commandStmt, output string, rand string) (transformedOutput string, err error)
 }
 
+type comparatorMatcher interface {
+	comparator
+	matcher
+}
+
 // A patternSanitiser replaces command output that matches
 // a regular expression pattern some some replacement text.
 type patternComparator struct {
-	patternKindAndCommand
+	patternProperties
 }
 
 func (p *patternComparator) normalize(cmd *commandStmt, output string, rand string) (string, error) {
-	if matched, err := p.matches(cmd); err != nil || !matched {
-		return "", err
-	}
 	return regexpReplaceRand(output, p.pattern, rand), nil
 }
 
+type patternComparatorMatcher struct {
+	kind
+	patternComparator
+	matchSpec
+}
+
+func (p *patternComparatorMatcher) init() error {
+	if err := p.patternComparator.init(); err != nil {
+		return err
+	}
+	if err := p.matchSpec.init(); err != nil {
+		return err
+	}
+	return nil
+}
+
 func regexpReplaceRand(input string, re *regexp.Regexp, rand string) string {
 	matches := re.FindAllStringSubmatchIndex(input, -1)
 
@@ -96,15 +115,16 @@ func regexpReplaceRand(input string, re *regexp.Regexp, rand string) string {
 // An unstableLineOrderComparator lexigraphically sorts the lines
 // a regular expression pattern some some replacement text.
 type unstableLineOrderComparator struct {
-	kindAndCommand
 }
 
 func (u *unstableLineOrderComparator) normalize(cmd *commandStmt, output string, rand string) (string, error) {
-	if matched, err := u.matches(cmd); err != nil || !matched {
-		return "", err
-	}
-
 	lines := strings.Split(output, "\n")
 	sort.Strings(lines)
 	return strings.Join(lines, "\n"), nil
 }
+
+type unstableLineOrderComparatorMatcher struct {
+	kind
+	unstableLineOrderComparator
+	matchSpec
+}

internal/cmd/preprocessor/cmd/page.go

@@ -69,8 +69,8 @@ type pageConfig struct {
 	LeftDelim  string `json:"leftDelim"`
 	RightDelim string `json:"rightDelim"`
 
-	Sanitisers  []sanitiser
-	Comparators []comparator
+	Sanitisers  []sanitiserMatcher
+	Comparators []comparatorMatcher
 }
 
 func (p *page) Format(state fmt.State, verb rune) {
@@ -169,15 +169,15 @@ func (p *page) loadConfig() {
 
 // parseSanitiser takes a CUE config value that represents a sanitiser
 // and returns a Go value that represents that sanitiser.
-func (p *page) parseSanitiser(m json.RawMessage) (sanitiser, error) {
+func (p *page) parseSanitiser(m json.RawMessage) (sanitiserMatcher, error) {
 	kind, err := parseKind(m)
 	if err != nil {
 		return nil, err
 	}
 
 	switch kind {
 	case "patternSanitiser":
-		var c patternSanitiser
+		var c patternSanitiserMatcher
 		if err := json.Unmarshal(m, &c); err != nil {
 			return nil, fmt.Errorf("failed to unmarshal %s: %v", kind, err)
 		}
@@ -205,15 +205,15 @@ func parseKind(m json.RawMessage) (string, error) {
 
 // parseComparator takes a CUE config value that represents a comparator
 // and returns a Go value that represents that comparator.
-func (p *page) parseComparator(m json.RawMessage) (comparator, error) {
+func (p *page) parseComparator(m json.RawMessage) (comparatorMatcher, error) {
 	kind, err := parseKind(m)
 	if err != nil {
 		return nil, err
 	}
 
 	switch kind {
 	case "patternComparator":
-		var p patternComparator
+		var p patternComparatorMatcher
 		if err := json.Unmarshal(m, &p); err != nil {
 			return nil, fmt.Errorf("failed to unmarshal %s: %v", kind, err)
 		}
@@ -222,7 +222,7 @@ func (p *page) parseComparator(m json.RawMessage) (comparator, error) {
 		}
 		return &p, nil
 	case "unstableLineOrderComparator":
-		var u unstableLineOrderComparator
+		var u unstableLineOrderComparatorMatcher
 		if err := json.Unmarshal(m, &u); err != nil {
 			return nil, fmt.Errorf("failed to unmarshal %s: %v", kind, err)
 		}

internal/cmd/preprocessor/cmd/rootfile.go

@@ -679,6 +679,13 @@ func (m *multiStepScript) run() (runerr error) {
 				}
 
 				for _, s := range m.page.config.Sanitisers {
+					matched, err := s.matches(stmt)
+					if err != nil {
+						m.fatalf("%v: failed to determine if sanitiser should apply for %q: %v", m, stmt.Cmd, err)
+					}
+					if !matched {
+						continue
+					}
 					if err := s.sanitise(stmt); err != nil {
 						m.fatalf("%v: failed to sanitise output for %q: %v", m, stmt.Cmd, err)
 					}
@@ -706,8 +713,15 @@ func (m *multiStepScript) run() (runerr error) {
 					actualAccum := stmt.Output
 					cachedAccum := cstmt.Output
 					for _, cmp := range m.page.config.Comparators {
-						f := m.getFence()
 						var err error
+						matched, err := cmp.matches(stmt)
+						if err != nil {
+							m.fatalf("%v: failed to determine if comparator should apply for %q: %v", m, stmt.Cmd, err)
+						}
+						if !matched {
+							continue
+						}
+						f := m.getFence()
 						actualAccum, err = cmp.normalize(stmt, actualAccum, f)
 						if err != nil {
 							return err

internal/cmd/preprocessor/cmd/sanitisers.go

@@ -27,21 +27,33 @@ import (
 // means that contributors produce the same output regardless of platform, OS
 // or other systems specifics.
 type sanitiser interface {
+	init() error
 	sanitise(*commandStmt) error
 }
 
+type sanitiserMatcher interface {
+	sanitiser
+	matcher
+}
+
+type matcher interface {
+	matches(cmd *commandStmt) (bool, error)
+}
+
 // kindAndCommand is the structure common to various types that are tied to
 // commands or steps. It defines a unique kind, and a series of fields that are
 // part of a one-of that identify when a value of the containing type should
 // apply.
 //
 // The kindAndCommand is generally embedded in other types, e.g.
 // patternSanitiser.
-type kindAndCommand struct {
+type kind struct {
 	// Kind is a discriminator field for identifying the containing type in
 	// config
 	Kind string `json:"kind"`
+}
 
+type matchSpec struct {
 	// Command, CommandPrefix and Step are a one-of. They define the coniditions
 	// under which values of the containing type should apply. We refer to such
 	// values as X below.
@@ -58,10 +70,12 @@ type kindAndCommand struct {
 	stmt *syntax.Stmt
 }
 
-func (k *kindAndCommand) init() error {
-	cmd := k.Command
-	if k.CommandPrefix != "" {
-		cmd = k.CommandPrefix
+func (m *matchSpec) init() error {
+	// We rely on the CUE validation here to know this is a one-of.
+
+	cmd := m.Command
+	if m.CommandPrefix != "" {
+		cmd = m.CommandPrefix
 	}
 	file, err := syntax.NewParser(syntax.KeepComments(true)).Parse(strings.NewReader(cmd), " ")
 	if err != nil {
@@ -71,13 +85,13 @@ func (k *kindAndCommand) init() error {
 	if l := len(file.Stmts); l != 1 {
 		return fmt.Errorf("expected 1 statement; found %d", l)
 	}
-	k.stmt = file.Stmts[0]
+	m.stmt = file.Stmts[0]
 	return nil
 }
 
-func (k *kindAndCommand) matches(cmd *commandStmt) (bool, error) {
+func (m *matchSpec) matches(cmd *commandStmt) (bool, error) {
 	got := cmd.stmt
-	want := k.stmt
+	want := m.stmt
 
 	// Negated state must match
 	if got.Negated != want.Negated {
@@ -90,7 +104,7 @@ func (k *kindAndCommand) matches(cmd *commandStmt) (bool, error) {
 		if !ok {
 			return false, nil
 		}
-		if k.CommandPrefix != "" {
+		if m.CommandPrefix != "" {
 			if len(got.Args) < len(want.Args) {
 				return false, nil
 			}
@@ -120,9 +134,7 @@ func (k *kindAndCommand) matches(cmd *commandStmt) (bool, error) {
 
 // patternKindAndCommand defines a common type to be embedded in types that use
 // pattern-based matching.
-type patternKindAndCommand struct {
-	kindAndCommand
-
+type patternProperties struct {
 	// Pattern defines the regexp that defines a match
 	Pattern pattern `json:"pattern"`
 
@@ -131,10 +143,7 @@ type patternKindAndCommand struct {
 	pattern *regexp.Regexp
 }
 
-func (p *patternKindAndCommand) init() (err error) {
-	if err := p.kindAndCommand.init(); err != nil {
-		return err
-	}
+func (p *patternProperties) init() (err error) {
 	p.pattern, err = regexp.Compile(p.Pattern.Expr)
 	if err != nil {
 		return err
@@ -148,7 +157,7 @@ func (p *patternKindAndCommand) init() (err error) {
 // A patternSanitiser replaces command output that matches
 // a regular expression pattern some some replacement text.
 type patternSanitiser struct {
-	patternKindAndCommand
+	patternProperties
 
 	Replacement string `json:"replacement"`
 }
@@ -159,9 +168,22 @@ type pattern struct {
 }
 
 func (p *patternSanitiser) sanitise(cmd *commandStmt) error {
-	if matched, err := p.matches(cmd); err != nil || !matched {
+	cmd.Output = p.pattern.ReplaceAllString(cmd.Output, p.Replacement)
+	return nil
+}
+
+type patternSanitiserMatcher struct {
+	kind
+	patternSanitiser
+	matchSpec
+}
+
+func (p *patternSanitiserMatcher) init() error {
+	if err := p.patternSanitiser.init(); err != nil {
+		return err
+	}
+	if err := p.matchSpec.init(); err != nil {
 		return err
 	}
-	cmd.Output = p.pattern.ReplaceAllString(cmd.Output, p.Replacement)
 	return nil
 }

internal/cmd/preprocessor/cmd/schema.cue

@@ -1,24 +1,30 @@
 package preprocessor
 
 #site: {
-	let _kindAndCommand = {
+	_kind: {
 		kind!: string // the discriminator field
+	}
 
+	_matcher: {
 		// command or commandPrefix are ways of indicating to which commands
 		// the sanitiser should apply.
 		//
 		// Implement a oneOf. Here both command and commandPrefix are strings
 		// because we perform a comparison at the AST level.
 		{
-			command!:       string
+			command!:       string & !=""
 			commandPrefix?: _|_
 		} | {
 			command?:       _|_
 			commandPrefix!: string
 		}
 	}
 
-	#sanitiser: _kindAndCommand & #patternSanitiser // build up a disjunction here if required
+	#sanitiser: _kind & #patternSanitiser // build up a disjunction of all sanitisers
+	#matchingSanitiser: {
+		_matcher
+		#sanitiser
+	}
 
 	#pattern: {
 		expr!:    string
@@ -28,7 +34,7 @@ package preprocessor
 	// Instances of #patternSanitiser define replacements that should be applied
 	// to commands that match.
 	#patternSanitiser: {
-		_kindAndCommand
+		_kind
 		kind: "patternSanitiser"
 
 		// pattern defines the text without the output of the matched command
@@ -39,22 +45,41 @@ package preprocessor
 		replacement?: string
 	}
 
-	#comparator: _kindAndCommand & (#patternComparator | #unstableLineOrderComparator)
+	#matchingPatternSanitiser: {
+		_matcher
+		#patternSanitiser
+	}
+
+	#comparator: _kind & (#patternComparator | #unstableLineOrderComparator)
+	#matchingComparator: {
+		_matcher
+		#comparator
+	}
 
 	#patternComparator: {
-		_kindAndCommand
+		_kind
 		kind: "patternComparator"
 
 		// pattern defines the text without the output of the matched command for
 		// which we loosen comparison.
 		pattern?: #pattern
 	}
 
+	#matchingPatternComparator: {
+		_matcher
+		#patternComparator
+	}
+
 	#unstableLineOrderComparator: {
-		_kindAndCommand
+		_kind
 		kind: "unstableLineOrderComparator"
 	}
 
+	#matchingUnstableLineOrderComparator: {
+		_matcher
+		#unstableLineOrderComparator
+	}
+
 	#page: {
 		// It's questionable whether these leftDelim and rightDelim should be
 		// required or not. We can always relax this later.
@@ -73,13 +98,13 @@ package preprocessor
 		// a multi-step script is re-run on a machine which has a different OS or
 		// architecture. e.g. the output of go version. All sanitisers that match
 		// are applied in order.
-		sanitisers?: [...#sanitiser]
+		sanitisers?: [...#matchingSanitiser]
 
 		// comparators allow for some variance in output, variance that is
 		// generally random. For example, a command where the order of lines can
 		// vary, or test run time. All comparators that match for a given command
 		// are applied, in order.
-		comparators?: [...#comparator]
+		comparators?: [...#matchingComparator]
 
 		cache?: {
 			upload?: [string]: string