CNTRLPLANE-3863: emit lifecycle-aware JUnit for informing e2e tests - #9168
CNTRLPLANE-3863: emit lifecycle-aware JUnit for informing e2e tests#9168ironcladlou wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ironcladlou: This pull request references CNTRLPLANE-3863 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. 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. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/testing
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds informing-failure detection and message extraction to the e2e v2 fail handler. Introduces JUnit XML models and Sequence Diagram(s)sequenceDiagram
participant GinkgoSuite
participant BuildLifecycleReport
participant XMLMarshaler
participant ArtifactDirectory
GinkgoSuite->>BuildLifecycleReport: provide suite and spec reports
BuildLifecycleReport-->>GinkgoSuite: return informing JUnit report
GinkgoSuite->>XMLMarshaler: marshal report
XMLMarshaler-->>GinkgoSuite: return XML
GinkgoSuite->>ArtifactDirectory: write lifecycle artifact
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (1)
test/e2e/v2/tests/lifecycle_validation_test.go (1)
15-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegister this feature through the v2 test entry point.
Export
RegisterLifecycleValidationTests(getTestCtx TestContextGetter)and wire it fromtest/e2e/v2/tests/suite_test.goinstead of registering theDescribethrough a package-level variable. As per coding guidelines, “Each test file must be feature-scoped and export aRegisterXxxTests(getTestCtx TestContextGetter)function that registers Ginkgo blocks.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/v2/tests/lifecycle_validation_test.go` around lines 15 - 20, Replace the package-level `Describe` registration in the lifecycle validation test with an exported `RegisterLifecycleValidationTests(getTestCtx TestContextGetter)` function that registers the same Ginkgo block, using the provided test context as required by the feature-scoped test pattern. Update `suite_test.go` to invoke this registration function through the v2 test entry point.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/v2/internal/junit_test.go`:
- Around line 28-179: Rename each affected table-test name in the test cases
around the existing specs to follow the required “When ... it should ...”
format, preserving the described behavior and all assertions. Update only the
name descriptions, including cases for informing failures, passes, skips,
excluded nodes, panics, mixed specs, and empty spec lists.
In `@test/e2e/v2/tests/suite_test.go`:
- Around line 72-80: Update the lifecycle artifact handling in the
ReportAfterSuite hook to fail the Ginkgo suite after logging either XML marshal
or file-write errors. Use the hook’s Ginkgo-supported failure mechanism after
the existing diagnostics, while preserving successful artifact generation and
the current error context.
---
Nitpick comments:
In `@test/e2e/v2/tests/lifecycle_validation_test.go`:
- Around line 15-20: Replace the package-level `Describe` registration in the
lifecycle validation test with an exported
`RegisterLifecycleValidationTests(getTestCtx TestContextGetter)` function that
registers the same Ginkgo block, using the provided test context as required by
the feature-scoped test pattern. Update `suite_test.go` to invoke this
registration function through the v2 test entry point.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 628241f6-b0fe-4a51-8f1e-63fb8bde8be6
📒 Files selected for processing (4)
test/e2e/v2/internal/junit.gotest/e2e/v2/internal/junit_test.gotest/e2e/v2/tests/lifecycle_validation_test.gotest/e2e/v2/tests/suite_test.go
31fdcdf to
3030e8e
Compare
|
/pipeline required |
|
Scheduling tests matching the |
35f7988 to
40187c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
test/e2e/v2/tests/suite_test.go (1)
72-80: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMarshal/write failures still don't fail the suite.
This still logs to stderr and returns silently on
xml.MarshalIndent/os.WriteFilefailure, so CI can pass green whilejunit_lifecycle_informing.xmlis never produced and downstream lifecycle ingestion silently loses data. This matches a prior review comment on this same range that does not appear to have been addressed.As per path instructions (test/e2e/v2/AGENTS.md), "Fail-loud philosophy: framework code generally panics on unrecoverable errors rather than silently continuing—if ReportAfterSuite can't produce output (missing/empty inputs), ensure it fails loudly enough or clearly logs and returns (avoid silent success)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/v2/tests/suite_test.go` around lines 72 - 80, Update the lifecycle JUnit output handling in ReportAfterSuite so xml.MarshalIndent and os.WriteFile failures fail loudly instead of logging to stderr and returning successfully. Replace the silent error paths with the suite’s established unrecoverable-error behavior, while preserving the existing output path and successful write flow.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/v2/internal/junit_test.go`:
- Around line 1-13: Move the plain testing-based coverage for
BuildLifecycleReport out of the internal package into the sanctioned tests/
location, preserving the existing e2ev2 build constraint and test behavior; only
retain it under internal/ if the exception is explicitly justified according to
the test/e2e/v2 convention.
- Around line 254-256: Update the validation around parsed.Suites before the
t.Fatalf call so the diagnostic never indexes parsed.Suites[0] unless at least
one suite exists. Preserve reporting of both suite and test-case counts when a
suite is present, while using a safe count or separate failure path when no
suites were parsed.
In `@test/e2e/v2/internal/junit.go`:
- Around line 84-103: Update the failure-state branch in the lifecycle spec
classification switch to use the Ginkgo report’s Failed() result, or otherwise
include SpecStateTimedout, SpecStateAborted, and SpecStateInterrupted alongside
SpecStateFailed and SpecStatePanicked. Preserve the existing JUnitFailureOutput
construction and suite.NumFailed increment so every failed lifecycle spec emits
a failure rather than falling through as a pass.
In `@test/e2e/v2/tests/lifecycle_validation_test.go`:
- Around line 15-20: Replace the package-level `var _ = Describe(...)`
registration with an exported `RegisterXxxTests(getTestCtx TestContextGetter)`
function in this lifecycle validation test file. Move the existing lifecycle
`Describe` and `It` blocks into that function, preserving their labels and
intentional failure behavior.
In `@test/e2e/v2/tests/suite_test.go`:
- Around line 64-67: Register ARTIFACT_DIR in the file’s environment-variable
catalog using RegisterEnvVar or RegisterEnvVarWithDefault alongside the existing
entries, then update the artifactDir lookup near the current os.Getenv call to
use internal.GetEnvVarValue. Preserve the existing empty-value early return and
ensure the registration occurs before ARTIFACT_DIR is read so PrintEnvVarHelp
includes it.
---
Duplicate comments:
In `@test/e2e/v2/tests/suite_test.go`:
- Around line 72-80: Update the lifecycle JUnit output handling in
ReportAfterSuite so xml.MarshalIndent and os.WriteFile failures fail loudly
instead of logging to stderr and returning successfully. Replace the silent
error paths with the suite’s established unrecoverable-error behavior, while
preserving the existing output path and successful write flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ddb52e8-b569-47d2-851e-fb76b7158383
📒 Files selected for processing (4)
test/e2e/v2/internal/junit.gotest/e2e/v2/internal/junit_test.gotest/e2e/v2/tests/lifecycle_validation_test.gotest/e2e/v2/tests/suite_test.go
a9527bf to
2aeed36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/e2e/v2/tests/lifecycle_validation_test.go (1)
15-20: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDoesn't follow the required
RegisterXxxTests(getTestCtx TestContextGetter)file pattern.As per path instructions,
test/e2e/v2/tests/*_test.go: "Each standard v2 test file must be feature-scoped, export aRegisterXxxTests(getTestCtx TestContextGetter)function, and define a top-level descriptiveDescribewith a filteringLabel." This file instead registers via a package-levelvar _ = Describe(...).♻️ Proposed refactor
+func RegisterLifecycleValidationTests(getTestCtx TestContextGetter) { // TODO(CNTRLPLANE-3863): Remove this file after validating lifecycle JUnit // output in CI. This is a temporary synthetic test that intentionally fails // with Label("Informing") to verify that: // 1. InformingAwareFailHandler converts the failure to a skip // 2. The ReportAfterSuite re-emits it as a failure with lifecycle="informing" // 3. The job still passes (exit 0) -var _ = Describe("[sig-hypershift][Jira:Hypershift][Feature:InformingLifecycle] Lifecycle Validation", - Label("informing-lifecycle-validation"), func() { - It("should demonstrate informing lifecycle JUnit output", Label("Informing"), func() { - Fail("This intentional failure validates that informing tests emit lifecycle metadata in JUnit") - }) - }) + Describe("[sig-hypershift][Jira:Hypershift][Feature:InformingLifecycle] Lifecycle Validation", + Label("informing-lifecycle-validation"), func() { + It("should demonstrate informing lifecycle JUnit output", Label("Informing"), func() { + Fail("This intentional failure validates that informing tests emit lifecycle metadata in JUnit") + }) + }) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/v2/tests/lifecycle_validation_test.go` around lines 15 - 20, Refactor the package-level Describe registration into the required exported RegisterXxxTests(getTestCtx TestContextGetter) function in the lifecycle validation test. Keep the feature-scoped descriptive Describe and filtering Label, and register the existing intentional failure through that function using the provided test-context getter.Source: Path instructions
test/e2e/v2/internal/junit_test.go (1)
276-288: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnconditional index still risks a panic on the very failure path it's meant to diagnose.
This is the same issue raised previously (marked "Addressed" in an earlier commit), but the current code still evaluates
parsed.Suites[0].TestCasesunconditionally inside thet.Fatalfcall. Iflen(parsed.Suites) != 1is 0 suites, this indexes an empty slice and panics instead of producing the diagnostic message.🐛 Proposed fix
- if len(parsed.Suites) != 1 || len(parsed.Suites[0].TestCases) != 1 { - t.Fatalf("round-trip produced %d suites / %d cases", len(parsed.Suites), len(parsed.Suites[0].TestCases)) - } + if len(parsed.Suites) != 1 { + t.Fatalf("round-trip produced %d suites, want 1", len(parsed.Suites)) + } + if len(parsed.Suites[0].TestCases) != 1 { + t.Fatalf("round-trip produced %d cases, want 1", len(parsed.Suites[0].TestCases)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/v2/internal/junit_test.go` around lines 276 - 288, Guard the round-trip validation in the JUnit test before indexing parsed.Suites[0]: handle an unexpected suite count and fail with a diagnostic that does not access any suite, then validate the test-case count only after confirming a suite exists. Preserve the existing lifecycle assertion for valid parsed data.
🧹 Nitpick comments (1)
test/e2e/v2/internal/junit.go (1)
71-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the "Informing" label into a shared constant.
The label string
"Informing"is hardcoded in two places here and duplicated again in test files (e.g.lifecycle_validation_test.go'sLabel("Informing")). A shared exported constant would prevent silent drift/typos breaking the filter.Also applies to: 120-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/v2/internal/junit.go` at line 71, Extract the repeated "Informing" label into a shared exported constant near the relevant label definitions, then update the checks in the JUnit logic and test usages such as lifecycle_validation_test.go to reference that constant instead of hardcoded strings. Preserve the existing label filtering behavior and use one canonical identifier everywhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/v2/internal/junit.go`:
- Around line 84-103: Update the lifecycle-result switch in BuildLifecycleReport
to add a types.SpecStatePending branch for informing specs, assigning the
appropriate skipped informative result (including its message) and incrementing
suite.NumSkipped. Keep existing failure and ordinary skipped-state handling
unchanged.
---
Duplicate comments:
In `@test/e2e/v2/internal/junit_test.go`:
- Around line 276-288: Guard the round-trip validation in the JUnit test before
indexing parsed.Suites[0]: handle an unexpected suite count and fail with a
diagnostic that does not access any suite, then validate the test-case count
only after confirming a suite exists. Preserve the existing lifecycle assertion
for valid parsed data.
In `@test/e2e/v2/tests/lifecycle_validation_test.go`:
- Around line 15-20: Refactor the package-level Describe registration into the
required exported RegisterXxxTests(getTestCtx TestContextGetter) function in the
lifecycle validation test. Keep the feature-scoped descriptive Describe and
filtering Label, and register the existing intentional failure through that
function using the provided test-context getter.
---
Nitpick comments:
In `@test/e2e/v2/internal/junit.go`:
- Line 71: Extract the repeated "Informing" label into a shared exported
constant near the relevant label definitions, then update the checks in the
JUnit logic and test usages such as lifecycle_validation_test.go to reference
that constant instead of hardcoded strings. Preserve the existing label
filtering behavior and use one canonical identifier everywhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 54283ad0-a09b-40c0-b6b0-848cd8ed9851
📒 Files selected for processing (5)
test/e2e/v2/AGENTS.mdtest/e2e/v2/internal/junit.gotest/e2e/v2/internal/junit_test.gotest/e2e/v2/tests/lifecycle_validation_test.gotest/e2e/v2/tests/suite_test.go
45d6bcd to
918e234
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9168 +/- ##
=======================================
Coverage 44.94% 44.94%
=======================================
Files 778 778
Lines 97427 97427
=======================================
Hits 43790 43790
Misses 50616 50616
Partials 3021 3021
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/retest |
|
Scheduling tests matching the |
|
rebased |
|
/test unit |
|
/retest |
|
/pipeline required |
|
Scheduling tests matching the |
openshift-ci-robot
added
the
verified
|
@ironcladlou: This PR has been marked as verified by 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. |
InformingAwareFailHandler converts informing test failures to Ginkgo skips so the job exits 0, but Sippy/CR drops skipped tests entirely, making informing failures invisible to regression tracking. Add a ReportAfterSuite that writes a supplemental JUnit file (junit_lifecycle_informing.xml) containing only informing tests with lifecycle="informing" on each <testcase>. ci-to-bigquery reads this attribute and populates the lifecycle column in BigQuery, matching the contract established by origin (TRT-2350) and ci-tools. This is a stopgap; the long-term fix is porting the test framework to OTE, which handles lifecycle JUnit emission natively.
076d104 to
fe4f240
Compare
openshift-ci-robot
removed
the
verified
|
/verified by e2e regression analysis and manual verification Example successful v2 gke output: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_hypershift/9168/pull-ci-openshift-hypershift-main-e2e-v2-gke/2082859653678501888 Passes and now shows 83 informing tests failed as expected, including the synthetic failure: The other 82 are existing informing failures that to date we have been skipping without appropriate metadata and thus are ignored in the downstream pipelines and invisible within component readiness and sippy test analysis (example with same failures prior to this patch). |
openshift-ci-robot
added
the
verified
|
@ironcladlou: This PR has been marked as verified by 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. |
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
1 similar comment
|
/retest |
|
/test e2e-aws-4-22 |
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/test e2e-aws-4-22 |
|
/test e2e-aws-4-22 |
|
/test e2e-aws-4-22 |
|
/override ci/prow/e2e-kubevirt-aws-ovn-reduced |
|
@csrwng: Overrode contexts on behalf of csrwng: ci/prow/e2e-kubevirt-aws-ovn-reduced 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 kubernetes-sigs/prow repository. |
|
/test e2e-aws-4-22 |
|
/uncc |
|
@ironcladlou: 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. |
8 participants
InformingAwareFailHandler converts informing test failures to Ginkgo skips so the job exits 0, but Sippy/CR drops skipped tests entirely, making informing failures invisible to regression tracking.
Add a ReportAfterSuite that writes a supplemental JUnit file (junit_lifecycle_informing.xml) containing only informing tests with lifecycle="informing" on each testcase. ci-to-bigquery reads this attribute and populates the lifecycle column in BigQuery, matching the contract established by origin (TRT-2350) and ci-tools.
This is a stopgap; the long-term fix is porting the test framework to OTE, which handles lifecycle JUnit emission natively.
A temporary synthetic informing test that intentionally fails to validate the output in CI was used to verify.
Summary by CodeRabbit