NO-JIRA: Update e2e diagnostics#488
Conversation
Don't double print diagnostics, bypassing GinkgoWriter entirely to avoid duplication. Relying on CapturedGinkgoWriterOutput was causing the dupes.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThe changes refactor diagnostic collection in e2e tests: instead of immediately dumping tracked resources to GinkgoWriter on failure, diagnostics are now collected into a per-spec map and injected into JUnit reports during post-suite processing, with stderr logging added for panic scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
openshift-ci-robot
added
the
jira/valid-reference
|
@theobarberbany: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/e2e_common.go`:
- Around line 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.
In `@e2e/e2e_test.go`:
- Around line 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33c5a4e8-e29b-454b-af86-9e5a52c242eb
📒 Files selected for processing (2)
e2e/e2e_common.goe2e/e2e_test.go
| // 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. |
There was a problem hiding this comment.
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.
| // 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.
| diag := collectTrackedResourceDiagnostics() | ||
| specDiagnostics[report.LeafNodeLocation.String()] = diag |
There was a problem hiding this comment.
🧩 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.goRepository: 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 2Repository: 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.modRepository: 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:
- 1: https://pkg.go.dev/github.com/onsi/ginkgo/v2/types
- 2: https://onsi.github.io/ginkgo/?utm_source=openai
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.
|
@theobarberbany: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Don't double print diagnostics, bypassing GinkgoWriter entirely to avoid duplication. Relying on CapturedGinkgoWriterOutput was causing the dupes.
Summary by CodeRabbit
Release Notes