From 78bb26e2b2b179b4bb53a823b894b9f1aa02420d Mon Sep 17 00:00:00 2001 From: Paul Jolly Date: Sun, 11 Feb 2024 08:07:41 +0000 Subject: [PATCH] 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 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 | 38 ++++++++++--- internal/cmd/preprocessor/cmd/page.go | 14 ++--- internal/cmd/preprocessor/cmd/rootfile.go | 16 +++++- internal/cmd/preprocessor/cmd/sanitisers.go | 60 +++++++++++++------- internal/cmd/preprocessor/cmd/schema.cue | 43 +++++++++++--- 5 files changed, 126 insertions(+), 45 deletions(-) diff --git a/internal/cmd/preprocessor/cmd/comparators.go b/internal/cmd/preprocessor/cmd/comparators.go index 1160843de7..52699dbf08 100644 --- a/internal/cmd/preprocessor/cmd/comparators.go +++ b/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 +} diff --git a/internal/cmd/preprocessor/cmd/page.go b/internal/cmd/preprocessor/cmd/page.go index fe00d6b5df..a479c9c472 100644 --- a/internal/cmd/preprocessor/cmd/page.go +++ b/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,7 +169,7 @@ 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 @@ -177,7 +177,7 @@ func (p *page) parseSanitiser(m json.RawMessage) (sanitiser, error) { 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,7 +205,7 @@ 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 @@ -213,7 +213,7 @@ func (p *page) parseComparator(m json.RawMessage) (comparator, error) { 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) } diff --git a/internal/cmd/preprocessor/cmd/rootfile.go b/internal/cmd/preprocessor/cmd/rootfile.go index 63963e4de5..8cc0191a7b 100644 --- a/internal/cmd/preprocessor/cmd/rootfile.go +++ b/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 diff --git a/internal/cmd/preprocessor/cmd/sanitisers.go b/internal/cmd/preprocessor/cmd/sanitisers.go index abed0312b4..7138dda65c 100644 --- a/internal/cmd/preprocessor/cmd/sanitisers.go +++ b/internal/cmd/preprocessor/cmd/sanitisers.go @@ -27,9 +27,19 @@ 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 @@ -37,11 +47,13 @@ type sanitiser interface { // // 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 } diff --git a/internal/cmd/preprocessor/cmd/schema.cue b/internal/cmd/preprocessor/cmd/schema.cue index 15a9dbbd5c..0eb7d40b4c 100644 --- a/internal/cmd/preprocessor/cmd/schema.cue +++ b/internal/cmd/preprocessor/cmd/schema.cue @@ -1,16 +1,18 @@ 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?: _|_ @@ -18,7 +20,11 @@ package preprocessor } } - #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,10 +45,19 @@ 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 @@ -50,11 +65,21 @@ package preprocessor 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