Skip to content

Commit

Permalink
UPSTREAM: <carry>: Ensure balanced brackets in annotated test names
Browse files Browse the repository at this point in the history
We recently started marking tests with apigroups, and in one case we
missed the closing bracket on the annotation resulting in the test being
erroneously skipped.

This adds a check in the annotation generation, and errors when brackets
are unbalanced.

```
Example:
$ ./hack/verify-generated.sh
FAILURE after 12.870s: hack/verify-generated.sh:13: executing '/home/stbenjam/go/src/github.com/openshift/origin/hack/update-generated.sh' expecting success: the command returned the wrong error code
Standard output from the command:
Nov  4 14:11:25.026: INFO: Enabling in-tree volume drivers
Nov  4 14:11:25.026: INFO: Warning: deprecated ENABLE_STORAGE_GCE_PD_DRIVER used. This will be removed in a future release. Use --enabled-volume-drivers=gcepd instead
Nov  4 14:11:25.026: INFO: Enabled gcepd and windows-gcepd in-tree volume drivers

Standard error from the command:
failed: unbalanced brackets in test name:
[Top Level] [sig-scheduling][Early] The openshift-console console pods [apigroup:console.openshift.io should be scheduled on different nodes
                                                                       ^
```
  • Loading branch information
stbenjam committed Nov 8, 2022
1 parent 9f4dd65 commit 2256387
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 3 deletions.
50 changes: 47 additions & 3 deletions openshift-hack/e2e/annotate/annotate.go
Expand Up @@ -17,6 +17,8 @@ var reHasSig = regexp.MustCompile(`\[sig-[\w-]+\]`)

// Run generates tests annotations for the targeted package.
func Run() {
var errors []string

if len(os.Args) != 2 && len(os.Args) != 3 {
fmt.Fprintf(os.Stderr, "error: requires exactly one argument\n")
os.Exit(1)
Expand All @@ -26,6 +28,9 @@ func Run() {
generator := newGenerator()
ginkgo.GetSuite().BuildTree()
ginkgo.GetSuite().WalkTests(generator.generateRename)
if len(generator.errors) > 0 {
errors = append(errors, generator.errors...)
}

renamer := newRenamerFromGenerated(generator.output)
// generated file has a map[string]string in the following format:
Expand All @@ -49,7 +54,6 @@ func Run() {
// Upstream sigs map to teams (if you have representation on that sig, you
// own those tests in origin)
// Downstream sigs: sig-imageregistry, sig-builds, sig-devex
var errors []string
for from, to := range generator.output {
if !reHasSig.MatchString(from) && !reHasSig.MatchString(to) {
errors = append(errors, fmt.Sprintf("all tests must define a [sig-XXXX] tag or have a rule %q", from))
Expand Down Expand Up @@ -131,8 +135,7 @@ func newGenerator() *ginkgoTestRenamer {
stringMatches: stringMatches,
matches: matches,
excludedTestsFilter: excludedTestsFilter,

output: make(map[string]string),
output: make(map[string]string),
}
}

Expand All @@ -158,6 +161,8 @@ type ginkgoTestRenamer struct {
output map[string]string
// map of unmatched test names
missing map[string]struct{}
// a list of errors to display
errors []string
}

func (r *ginkgoTestRenamer) updateNodeText(name string, node types.TestSpec) {
Expand Down Expand Up @@ -226,6 +231,9 @@ func (r *ginkgoTestRenamer) generateRename(name string, node types.TestSpec) {
newLabels += " [Suite:k8s]"
}

if err := checkBalancedBrackets(newName); err != nil {
r.errors = append(r.errors, err.Error())
}
r.output[name] = newLabels
}

Expand All @@ -239,3 +247,39 @@ func (r *ginkgoTestRenamer) generateRename(name string, node types.TestSpec) {
func isGoModulePath(packagePath, module, modulePath string) bool {
return regexp.MustCompile(fmt.Sprintf(`\b%s(@[^/]*|)/%s\b`, regexp.QuoteMeta(module), regexp.QuoteMeta(modulePath))).MatchString(packagePath)
}

// checkBalancedBrackets ensures that square brackets are balanced in generated test
// names. If they are not, it returns an error with the name of the test and a guess
// where the unmatched bracket(s) are.
func checkBalancedBrackets(testName string) error {
stack := make([]int, 0, len(testName))
for idx, c := range testName {
if c == '[' {
stack = append(stack, idx)
} else if c == ']' {
// case when we start off with a ]
if len(stack) == 0 {
stack = append(stack, idx)
} else {
stack = stack[:len(stack)-1]
}
}
}

if len(stack) > 0 {
msg := testName + "\n"
outerLoop:
for i := 0; i < len(testName); i++ {
for _, loc := range stack {
if i == loc {
msg += "^"
continue outerLoop
}
}
msg += " "
}
return fmt.Errorf("unbalanced brackets in test name:\n%s\n", msg)
}

return nil
}
55 changes: 55 additions & 0 deletions openshift-hack/e2e/annotate/annotate_test.go
@@ -0,0 +1,55 @@
package annotate

import (
"fmt"
"os"
"testing"
)

func Test_checkBalancedBrackets(t *testing.T) {
tests := []struct {
testCase string
testName string
wantErr bool
}{
{
testCase: "balanced brackets succeeds",
testName: "[sig-storage] Test that storage [apigroup:storage.openshift.io] actually works [Driver:azure][Serial][Late]",
wantErr: false,
},
{
testCase: "unbalanced brackets errors",
testName: "[sig-storage] Test that storage [apigroup:storage.openshift.io actually works [Driver:azure][Serial][Late]",
wantErr: true,
},
{
testCase: "start with close bracket errors",
testName: "[sig-storage] test with a random bracket ]",
wantErr: true,
},
{
testCase: "multiple unbalanced brackets errors",
testName: "[sig-storage Test that storage [apigroup:storage.openshift.io actually works [Driver:azure]",
wantErr: true,
},
{
testCase: "balanced deeply nested brackets succeeds",
testName: "[[[[[[some weird test with deeply nested brackets]]]]]]",
wantErr: false,
},
{
testCase: "unbalanced deeply nested brackets errors",
testName: "[[[[[[some weird test with deeply nested brackets]]]]]",
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.testCase, func(t *testing.T) {
if err := checkBalancedBrackets(tt.testName); (err != nil) != tt.wantErr {
t.Errorf("checkBalancedBrackets() error = %v, wantErr %v", err, tt.wantErr)
} else if err != nil {
fmt.Fprintf(os.Stderr, "checkBalancedBrackets() success, found expected err = \n%s\n", err.Error())
}
})
}
}

0 comments on commit 2256387

Please sign in to comment.