Skip to content

NO-JIRA: Update e2e diagnostics #488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
theobarberbany wants to merge 1 commit into
base: main
Choose a base branch
from
Open

NO-JIRA: Update e2e diagnostics #488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions e2e/e2e_common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package e2e
import (
"context"
"fmt"
"os"
"reflect"
"strings"
"time"
Expand Down Expand Up @@ -66,6 +67,11 @@ var (
// ReportAfterEach dumps detailed state for each tracked resource then clears
// the list.
resourcesUnderTest []client.Object

// specDiagnostics stores per-spec diagnostic output keyed by spec text.
// ReportAfterEach populates it on failure; ReportAfterSuite reads it to
// append diagnostics to the JUnit failure message.
Comment on lines +71 to +73
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Comment/key mismatch for specDiagnostics can mislead future changes.

Line 71 says the map is keyed by “spec text”, but current usage keys by LeafNodeLocation.String(). Please align the comment to the implemented key to avoid maintenance mistakes.

✏️ Suggested comment fix 
-// specDiagnostics stores per-spec diagnostic output keyed by spec text.
+// specDiagnostics stores per-spec diagnostic output keyed by leaf node location.
 // ReportAfterEach populates it on failure; ReportAfterSuite reads it to
 // append diagnostics to the JUnit failure message.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// specDiagnostics stores per-spec diagnostic output keyed by spec text.
// ReportAfterEach populates it on failure; ReportAfterSuite reads it to
// append diagnostics to the JUnit failure message.
// specDiagnostics stores per-spec diagnostic output keyed by leaf node location.
// ReportAfterEach populates it on failure; ReportAfterSuite reads it to
// append diagnostics to the JUnit failure message.
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@e2e/e2e_common.go` around lines 71 - 73, The comment for specDiagnostics is
inaccurate: it says the map is keyed by “spec text” but the code keys entries by
LeafNodeLocation.String(); update the comment for specDiagnostics to state that
entries are keyed by the spec's LeafNodeLocation.String() (or simply “leaf node
location string”) and mention that ReportAfterEach populates it and
ReportAfterSuite reads it, so future maintainers won't assume spec text is the
key.

specDiagnostics = map[string]string{}
)

func init() {
Expand Down Expand Up @@ -113,16 +119,16 @@ func trackResource(obj client.Object) {
resourcesUnderTest = append(resourcesUnderTest, obj)
}

// dumpTrackedResources writes detailed diagnostics for each tracked resource
// to GinkgoWriter. For each resource it fetches current state, marshals it as
// collectTrackedResourceDiagnostics builds a diagnostics string for each
// tracked resource. For each resource it fetches current state, marshals it as
// YAML, and lists events specific to that object. It also dumps all
// AWSMachineTemplates (on AWS) and all events in both namespaces.
// Best-effort: panics are recovered and individual errors are logged without
// aborting the dump.
func dumpTrackedResources() {
func collectTrackedResourceDiagnostics() string {
defer func() {
if r := recover(); r != nil {
GinkgoWriter.Printf("WARNING: dumpTrackedResources panicked: %v\n", r)
fmt.Fprintf(os.Stderr, "WARNING: collectTrackedResourceDiagnostics panicked: %v\n", r)
}
}()

Expand All @@ -141,7 +147,7 @@ func dumpTrackedResources() {

buf.WriteString("\n=== End Test Failure Diagnostics ===\n")

GinkgoWriter.Print(buf.String())
return buf.String()
}

func dumpSingleResource(buf *strings.Builder, obj client.Object) {
Expand Down
12 changes: 4 additions & 8 deletions e2e/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ var _ = BeforeSuite(func() {

var _ = ReportAfterEach(func(report SpecReport) {
if report.Failed() {
dumpTrackedResources()
diag := collectTrackedResourceDiagnostics()
specDiagnostics[report.LeafNodeLocation.String()] = diag
Comment on lines +39 to +40
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check Go version in go.mod
head -5 go.mod | grep -i "^go "

Repository: openshift/cluster-capi-operator

Length of output: 84

🏁 Script executed:

# Read the relevant sections of e2e_test.go to understand the context
wc -l e2e/e2e_test.go && echo "---" && sed -n '30,50p' e2e/e2e_test.go && echo "---" && sed -n '60,75p' e2e/e2e_test.go

Repository: openshift/cluster-capi-operator

Length of output: 1209

🏁 Script executed:

# Check if FullText() method is available in the code
rg "FullText" e2e/e2e_test.go -A 2 -B 2

Repository: openshift/cluster-capi-operator

Length of output: 57

🏁 Script executed:

# Find the import of Ginkgo to determine exact version
rg "github.com/onsi/ginkgo" go.mod

Repository: openshift/cluster-capi-operator

Length of output: 143

🌐 Web query:

Ginkgo v2.2.0 SpecReport LeafNodeLocation uniqueness guarantee table-driven tests

💡 Result:

In Ginkgo v2.2.0, types.SpecReport.LeafNodeLocation is just a types.CodeLocation (i.e., FileName, LineNumber, optional stack trace / custom message) pointing at where Ginkgo believes the leaf node (typically an It, or the generated leaf node for a table Entry) was declared—it is not documented as a uniqueness guarantee. So you should expect duplicates any time multiple specs resolve to the same callsite/line (common with helpers/wrappers, loops that generate nodes, or table entries that end up sharing a single declaration location). [1][2]

For table-driven tests specifically: each Entry(...) becomes its own spec, but LeafNodeLocation is still only a source location, not an ID, so it’s not safe to use it as a unique key for entries/specs. [1][2]

If you need stable uniqueness, prefer deriving a key from the spec’s text (e.g., report.FullText() / hierarchy + LeafNodeText), or explicitly set a distinct location/message via the types.CodeLocation decorator (e.g. types.NewCustomCodeLocation("...")) when constructing nodes. [2]

Sources:
[1] Ginkgo types.SpecReport / LeafNodeLocation field docs (pkg.go.dev) (pkg.go.dev)
[2] Ginkgo docs: CodeLocation decorator / types.NewCustomCodeLocation (onsi.github.io)

Citations:

Use a stronger diagnostics key to prevent collisions across failed specs.

Using only LeafNodeLocation.String() as a map key risks collisions when multiple specs share the same source location—this occurs with table-driven tests (multiple Entry calls at one line) or specs generated by helpers. This will cause one spec's diagnostics to overwrite another's, attaching the wrong diagnostic text to failures in the JUnit output at lines 39-40 and 68-69.

A composite key combining LeafNodeLocation.String() with FullText() (or equivalent) ensures uniqueness per spec.

🔧 Suggested fix 
 var _ = ReportAfterEach(func(report SpecReport) {
 	if report.Failed() {
 		diag := collectTrackedResourceDiagnostics()
-		specDiagnostics[report.LeafNodeLocation.String()] = diag
+		specDiagnostics[diagnosticKey(report)] = diag
 	}

 	resourcesUnderTest = nil
 })
+
+func diagnosticKey(sr SpecReport) string {
+	return fmt.Sprintf("%s|%s", sr.LeafNodeLocation.String(), sr.FullText())
+}
@@
-		if diag, ok := specDiagnostics[sr.LeafNodeLocation.String()]; ok {
+		if diag, ok := specDiagnostics[diagnosticKey(*sr)]; ok {
 			sr.Failure.Message += "\n\n" + diag
 		}

Also applies to: 68-69

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@e2e/e2e_test.go` around lines 39 - 40, Replace the weak map key using only
report.LeafNodeLocation.String() when populating specDiagnostics (where diag :=
collectTrackedResourceDiagnostics() and
specDiagnostics[report.LeafNodeLocation.String()] = diag) with a composite key
that includes the leaf location plus the spec's unique text (e.g., combine
report.LeafNodeLocation.String() and report.FullText() or equivalent) so each
spec instance is unique; apply the same change to the other occurrence around
the specDiagnostics assignment at the later location (the lines referenced as
68-69) to prevent overwriting diagnostics for table-driven or helper-generated
specs.

}

resourcesUnderTest = nil
Expand All @@ -58,20 +59,15 @@ var _ = ReportAfterSuite("junit with diagnostics", func(report Report) {
return
}

// Append GinkgoWriter output (which contains our tracked resource dump)
// to the failure description so it appears in the <failure> element of
// the JUnit XML, which is what Spyglass renders by default.
for i := range report.SpecReports {
sr := &report.SpecReports[i]
if !sr.Failed() {
continue
}

if sr.CapturedGinkgoWriterOutput == "" {
continue
if diag, ok := specDiagnostics[sr.LeafNodeLocation.String()]; ok {
sr.Failure.Message += "\n\n" + diag
}

sr.Failure.Message += "\n\n" + sr.CapturedGinkgoWriterOutput
}

dst := filepath.Join(artifactDir, "junit_cluster_capi_operator.xml")
Expand Down