|
| 1 | +package extensions |
| 2 | + |
| 3 | +import ( |
| 4 | + "fmt" |
| 5 | + "sort" |
| 6 | + "strings" |
| 7 | + |
| 8 | + "github.com/openshift/origin/pkg/test/ginkgo/junitapi" |
| 9 | +) |
| 10 | + |
| 11 | +// binaryKey uniquely identifies a test binary by its image and path. |
| 12 | +type binaryKey struct { |
| 13 | + imageTag string |
| 14 | + binaryPath string |
| 15 | +} |
| 16 | + |
| 17 | +// CheckForGlobalNodes checks if any code locations appear in ALL tests for each binary, |
| 18 | +// which indicates that BeforeEach/AfterEach nodes were registered at the global level |
| 19 | +// (root of the Ginkgo tree). This is a serious bug because these hooks run for EVERY test, |
| 20 | +// wasting resources and time. In some cases, these global nodes can |
| 21 | +// interfere with test operations. |
| 22 | +// |
| 23 | +// Tests are grouped by their source image and binary path, and each group is checked separately. |
| 24 | +// |
| 25 | +// Returns JUnit test cases as flakes (failing + passing with same name) for each binary |
| 26 | +// that has global nodes detected. This allows the issue to be tracked in CI without |
| 27 | +// blocking the test run for now. |
| 28 | +func CheckForGlobalNodes(specs ExtensionTestSpecs) []*junitapi.JUnitTestCase { |
| 29 | + // Group tests by image tag and binary path |
| 30 | + specsByBinary := make(map[binaryKey]ExtensionTestSpecs) |
| 31 | + for _, spec := range specs { |
| 32 | + if spec.Binary == nil { |
| 33 | + continue |
| 34 | + } |
| 35 | + key := binaryKey{ |
| 36 | + imageTag: spec.Binary.imageTag, |
| 37 | + binaryPath: spec.Binary.binaryPath, |
| 38 | + } |
| 39 | + specsByBinary[key] = append(specsByBinary[key], spec) |
| 40 | + } |
| 41 | + |
| 42 | + // Check each binary for global nodes |
| 43 | + type binaryResult struct { |
| 44 | + imageTag string |
| 45 | + binaryPath string |
| 46 | + globalLocations []string |
| 47 | + totalTests int |
| 48 | + } |
| 49 | + var results []binaryResult |
| 50 | + |
| 51 | + for key, binarySpecs := range specsByBinary { |
| 52 | + // Skip binaries with fewer than 25 tests - can't detect global nodes meaningfully |
| 53 | + // with small test counts as it would generate false positives |
| 54 | + if len(binarySpecs) < 25 { |
| 55 | + continue |
| 56 | + } |
| 57 | + |
| 58 | + totalTests := len(binarySpecs) |
| 59 | + |
| 60 | + // Count how many unique tests contain each code location |
| 61 | + locationToTests := make(map[string]map[string]struct{}) |
| 62 | + for _, spec := range binarySpecs { |
| 63 | + for _, loc := range spec.CodeLocations { |
| 64 | + if locationToTests[loc] == nil { |
| 65 | + locationToTests[loc] = make(map[string]struct{}) |
| 66 | + } |
| 67 | + locationToTests[loc][spec.Name] = struct{}{} |
| 68 | + } |
| 69 | + } |
| 70 | + |
| 71 | + // Find code locations that appear in ALL tests for this binary |
| 72 | + var globalLocations []string |
| 73 | + for loc, tests := range locationToTests { |
| 74 | + if len(tests) != totalTests { |
| 75 | + continue |
| 76 | + } |
| 77 | + |
| 78 | + // Skip locations that are expected to be in all tests |
| 79 | + if isExpectedGlobalLocation(loc) { |
| 80 | + continue |
| 81 | + } |
| 82 | + |
| 83 | + globalLocations = append(globalLocations, loc) |
| 84 | + } |
| 85 | + |
| 86 | + if len(globalLocations) > 0 { |
| 87 | + sort.Strings(globalLocations) |
| 88 | + results = append(results, binaryResult{ |
| 89 | + imageTag: key.imageTag, |
| 90 | + binaryPath: key.binaryPath, |
| 91 | + globalLocations: globalLocations, |
| 92 | + totalTests: totalTests, |
| 93 | + }) |
| 94 | + } |
| 95 | + } |
| 96 | + |
| 97 | + if len(results) == 0 { |
| 98 | + return nil |
| 99 | + } |
| 100 | + |
| 101 | + // Sort results by imageTag then binaryPath for consistent output |
| 102 | + sort.Slice(results, func(i, j int) bool { |
| 103 | + if results[i].imageTag != results[j].imageTag { |
| 104 | + return results[i].imageTag < results[j].imageTag |
| 105 | + } |
| 106 | + return results[i].binaryPath < results[j].binaryPath |
| 107 | + }) |
| 108 | + |
| 109 | + // Create JUnit test cases as flakes (one failing, one passing per binary) |
| 110 | + var testCases []*junitapi.JUnitTestCase |
| 111 | + |
| 112 | + for _, result := range results { |
| 113 | + testName := fmt.Sprintf("[sig-ci] image %s binary %s should not have global BeforeEach/AfterEach nodes", result.imageTag, result.binaryPath) |
| 114 | + |
| 115 | + // Build detailed failure message |
| 116 | + failureOutput := buildGlobalNodeFailureMessage(result.imageTag, result.binaryPath, result.globalLocations, result.totalTests) |
| 117 | + |
| 118 | + // Create failing test case |
| 119 | + failingCase := &junitapi.JUnitTestCase{ |
| 120 | + Name: testName, |
| 121 | + FailureOutput: &junitapi.FailureOutput{ |
| 122 | + Message: fmt.Sprintf("Found %d global BeforeEach/AfterEach code locations in image %s binary %s", len(result.globalLocations), result.imageTag, result.binaryPath), |
| 123 | + Output: failureOutput, |
| 124 | + }, |
| 125 | + } |
| 126 | + testCases = append(testCases, failingCase) |
| 127 | + |
| 128 | + // Create passing test case (same name = flake) |
| 129 | + passingCase := &junitapi.JUnitTestCase{ |
| 130 | + Name: testName, |
| 131 | + } |
| 132 | + testCases = append(testCases, passingCase) |
| 133 | + } |
| 134 | + |
| 135 | + return testCases |
| 136 | +} |
| 137 | + |
| 138 | +// buildGlobalNodeFailureMessage creates a detailed message explaining the global node issue. |
| 139 | +func buildGlobalNodeFailureMessage(imageTag, binaryPath string, globalLocations []string, totalTests int) string { |
| 140 | + var sb strings.Builder |
| 141 | + |
| 142 | + sb.WriteString("\n") |
| 143 | + sb.WriteString("╔══════════════════════════════════════════════════════════════════════════════╗\n") |
| 144 | + sb.WriteString("║ GLOBAL BEFOREEACH/AFTEREACH DETECTED ║\n") |
| 145 | + sb.WriteString("╚══════════════════════════════════════════════════════════════════════════════╝\n") |
| 146 | + sb.WriteString("\n") |
| 147 | + sb.WriteString(fmt.Sprintf("IMAGE: %s\n", imageTag)) |
| 148 | + sb.WriteString(fmt.Sprintf("BINARY: %s\n", binaryPath)) |
| 149 | + sb.WriteString(fmt.Sprintf("TESTS: %d\n", totalTests)) |
| 150 | + sb.WriteString("\n") |
| 151 | + sb.WriteString("PROBLEM: The following code locations appear in ALL tests for this binary,\n") |
| 152 | + sb.WriteString("indicating that BeforeEach or AfterEach hooks were registered at the global level.\n") |
| 153 | + sb.WriteString("\n") |
| 154 | + sb.WriteString("This means these hooks run for EVERY SINGLE TEST even when not needed,\n") |
| 155 | + sb.WriteString("wasting CI resources and adding unnecessary test execution time.\n") |
| 156 | + sb.WriteString("\n") |
| 157 | + sb.WriteString("GLOBAL CODE LOCATIONS:\n") |
| 158 | + for _, loc := range globalLocations { |
| 159 | + sb.WriteString(fmt.Sprintf(" • %s\n", loc)) |
| 160 | + } |
| 161 | + sb.WriteString("\n") |
| 162 | + sb.WriteString("COMMON CAUSES:\n") |
| 163 | + sb.WriteString("\n") |
| 164 | + sb.WriteString("1. Package-level FixturePath() call:\n") |
| 165 | + sb.WriteString(" BAD: var myFixture = exutil.FixturePath(\"testdata\", \"file.yaml\")\n") |
| 166 | + sb.WriteString(" GOOD: func myFixture() string { return exutil.FixturePath(\"testdata\", \"file.yaml\") }\n") |
| 167 | + sb.WriteString("\n") |
| 168 | + sb.WriteString("2. Package-level NewCLI/NewCLIWithoutNamespace call:\n") |
| 169 | + sb.WriteString(" BAD: var oc = exutil.NewCLIWithoutNamespace(\"test\")\n") |
| 170 | + sb.WriteString(" GOOD: Inside g.Describe(): oc := exutil.NewCLIWithoutNamespace(\"test\")\n") |
| 171 | + sb.WriteString("\n") |
| 172 | + sb.WriteString("HOW TO FIX:\n") |
| 173 | + sb.WriteString("\n") |
| 174 | + sb.WriteString("1. Find the file and line number from the locations above\n") |
| 175 | + sb.WriteString("2. Look for package-level var declarations that call FixturePath() or NewCLI*()\n") |
| 176 | + sb.WriteString("3. Convert them to functions (for FixturePath) or move inside Describe blocks (for CLI)\n") |
| 177 | + sb.WriteString("\n") |
| 178 | + sb.WriteString("EXAMPLE FIX FOR FixturePath:\n") |
| 179 | + sb.WriteString("\n") |
| 180 | + sb.WriteString(" // BEFORE (causes global BeforeEach):\n") |
| 181 | + sb.WriteString(" var myFixture = exutil.FixturePath(\"testdata\", \"file.yaml\")\n") |
| 182 | + sb.WriteString(" func useFixture() { oc.Run(\"apply\").Args(\"-f\", myFixture) }\n") |
| 183 | + sb.WriteString("\n") |
| 184 | + sb.WriteString(" // AFTER (lazy evaluation, no global BeforeEach):\n") |
| 185 | + sb.WriteString(" func myFixture() string { return exutil.FixturePath(\"testdata\", \"file.yaml\") }\n") |
| 186 | + sb.WriteString(" func useFixture() { oc.Run(\"apply\").Args(\"-f\", myFixture()) }\n") |
| 187 | + sb.WriteString("\n") |
| 188 | + sb.WriteString("FOR AI ASSISTANTS:\n") |
| 189 | + sb.WriteString("If you're Claude or another AI helping fix this, search the codebase for the file\n") |
| 190 | + sb.WriteString("paths shown above. Look for package-level 'var' declarations that call\n") |
| 191 | + sb.WriteString("exutil.FixturePath() or exutil.NewCLI*(). Convert FixturePath vars to functions\n") |
| 192 | + sb.WriteString("that return the path. Move NewCLI* calls inside Ginkgo Describe/Context blocks.\n") |
| 193 | + sb.WriteString("\n") |
| 194 | + |
| 195 | + return sb.String() |
| 196 | +} |
| 197 | + |
| 198 | +// allowedGlobalLocations is an exception list of code locations that are known |
| 199 | +// to register global BeforeEach/AfterEach nodes. These should be fixed |
| 200 | +// unless it is intentional. |
| 201 | +var allowedGlobalLocations = map[string]string{ |
| 202 | + // Add exceptions here with an explanation, e.g.: |
| 203 | + // "github.com/openshift/origin/test/extended/networking/util.go:82": "FIXME: package-level FixturePath call", |
| 204 | +} |
| 205 | + |
| 206 | +// isExpectedGlobalLocation returns true for code locations that are expected |
| 207 | +// to appear in all tests and should not trigger the global node detection. |
| 208 | +func isExpectedGlobalLocation(loc string) bool { |
| 209 | + // Check exact match in allowlist |
| 210 | + if _, ok := allowedGlobalLocations[loc]; ok { |
| 211 | + return true |
| 212 | + } |
| 213 | + |
| 214 | + // Check pattern matches for framework infrastructure that's legitimately global |
| 215 | + expectedPatterns := []string{ |
| 216 | + // None currently - if we find legitimate cases, add them here with comments |
| 217 | + } |
| 218 | + |
| 219 | + for _, pattern := range expectedPatterns { |
| 220 | + if strings.Contains(loc, pattern) { |
| 221 | + return true |
| 222 | + } |
| 223 | + } |
| 224 | + |
| 225 | + return false |
| 226 | +} |
0 commit comments