jobrunaggregator: treat informing tests as always passing #4902

stbenjam · 2026-01-15T13:36:02Z

Tests with lifecycle="informing" in the JUnit XML do not impact aggregation results. This change parses the lifecycle attribute and treats informing tests as always passing in the pass/fail calculation.

openshift-ci-robot · 2026-01-15T13:36:04Z

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

coderabbitai · 2026-01-15T13:36:18Z

Walkthrough

Introduces a new Lifecycle field on JUnit test case types and propagates it through aggregation and pass/fail logic; tests marked lifecycle="informing" are treated as non-impacting for aggregation.

Changes

Cohort / File(s)	Summary
Struct field additions `pkg/junit/types.go`, `pkg/jobrunaggregator/jobrunaggregatorlib/junit.go`	Added public `Lifecycle string` to `junit.TestCase` (XML attr `lifecycle,attr,omitempty`) and `jobrunaggregatorlib.TestCaseDetails` with comments indicating "blocking" vs "informing".
Aggregation logic `pkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.go`	In `aggregateTestCase`, copy incoming test case `Lifecycle` into aggregated `TestCaseDetails` when non-empty.
Pass/fail evaluation `pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go`	In `weeklyAverageFromTenDays.CheckFailed`, added early branch: if `testCaseDetails.Lifecycle == "informing"`, treat as passed and skip further failure checks.
Test data `pkg/junit/testdata/zz_fixture_TestCensorTestSuite.yaml`	Added empty `Lifecycle: ""` entries to fixture YAML mappings to reflect the new field in test data.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

✨ Finishing touches

📝 Generate docstrings

🧹 Recent nitpick comments

pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go (1)
552-556: LGTM - consider defining a constant for the lifecycle value.

The logic correctly treats informing tests as always passing. For improved maintainability, consider extracting "informing" to a named constant, especially if this value is used elsewhere or might need case-insensitive matching.
♻️ Optional: Extract to constant
+const lifecycleInforming = "informing"
+
 func (a *weeklyAverageFromTenDays) CheckFailed(ctx context.Context, jobName string, suiteNames []string, testCaseDetails *jobrunaggregatorlib.TestCaseDetails) (testCaseStatus, string, error) {
 	// Informing tests do not impact aggregation, so treat them as always passing
-	if testCaseDetails.Lifecycle == "informing" {
+	if testCaseDetails.Lifecycle == lifecycleInforming {
 		reason := fmt.Sprintf("always passing %q: informing test does not impact aggregation\n", testCaseDetails.Name)
 		return testCasePassed, reason, nil
 	}

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 061d456 and 665c8a2.

📒 Files selected for processing (5)

pkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.go
pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go
pkg/jobrunaggregator/jobrunaggregatorlib/junit.go
pkg/junit/testdata/zz_fixture_TestCensorTestSuite.yaml
pkg/junit/types.go

🚧 Files skipped from review as they are similar to previous changes (2)

pkg/junit/types.go
pkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go
pkg/junit/testdata/zz_fixture_TestCensorTestSuite.yaml
pkg/jobrunaggregator/jobrunaggregatorlib/junit.go

🔇 Additional comments (2)

pkg/junit/testdata/zz_fixture_TestCensorTestSuite.yaml (1)

29-29: LGTM!

The test fixture correctly includes the new Lifecycle field with empty default values across all TestCase entries, maintaining schema consistency with the struct changes.

Also applies to: 49-49, 89-89, 109-109, 149-149, 169-169

pkg/jobrunaggregator/jobrunaggregatorlib/junit.go (1)

11-13: LGTM!

The Lifecycle field is well-documented and appropriately placed in the struct. The comment clearly explains its purpose and impact on aggregation behavior.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

smg247 · 2026-01-15T13:38:21Z

/lgtm

openshift-ci · 2026-01-15T13:38:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smg247, stbenjam

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [smg247]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.go (1)
234-236: Pre-existing bug: error is silently ignored.

This is not introduced by your PR, but Line 235 returns nil instead of err, silently discarding the marshal error.
🐛 Suggested fix
 	detailsYaml, err := yaml.Marshal(currDetails)
 	if err != nil {
-		return nil
+		return err
 	}

🧹 Nitpick comments (1)

pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go (1)

552-556: LGTM! Consider extracting the "informing" literal to a constant.

The early return logic correctly bypasses aggregation evaluation for informing tests. The placement before testShouldAlwaysPass is appropriate.

Optional: The string "informing" is also used in the aggregation layer. Consider defining a package-level constant to avoid magic strings and ensure consistency.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 4175ad3 and 061d456.

📒 Files selected for processing (4)

pkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.go
pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go
pkg/jobrunaggregator/jobrunaggregatorlib/junit.go
pkg/junit/types.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

pkg/jobrunaggregator/jobrunaggregatorlib/junit.go
pkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.go
pkg/jobrunaggregator/jobrunaggregatoranalyzer/pass_fail.go
pkg/junit/types.go

🔇 Additional comments (3)

pkg/junit/types.go (1)

71-74: LGTM!

The Lifecycle field is well-documented and correctly annotated with the XML attribute tag. The omitempty ensures backward compatibility with JUnit XML that doesn't include this attribute.

pkg/jobrunaggregator/jobrunaggregatorlib/junit.go (1)

11-13: LGTM!

The Lifecycle field addition to TestCaseDetails mirrors the field in junit.TestCase and is correctly documented. This enables lifecycle data to propagate through the aggregation pipeline.

pkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.go (1)

196-200: LGTM!

The lifecycle propagation is correctly placed before the pass/fail/skip handling. The non-empty check ensures we don't overwrite existing lifecycle data with empty strings.

Note: If the same test appears in multiple job runs with different lifecycle values, later runs will overwrite earlier values. This should be fine in practice since lifecycle is expected to be consistent for a given test across runs.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

openshift-ci-robot · 2026-01-15T15:01:51Z

/retest-required

Remaining retests: 0 against base HEAD 4175ad3 and 2 for PR HEAD 061d456 in total

Tests with lifecycle="informing" in the JUnit XML do not impact aggregation results. This change parses the lifecycle attribute and treats informing tests as always passing in the pass/fail calculation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

openshift-ci · 2026-01-15T17:10:52Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2026-01-15T18:31:58Z

@stbenjam: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/breaking-changes	`665c8a2`	link	false	`/test breaking-changes`
ci/prow/images	`665c8a2`	link	true	`/test images`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

openshift-ci bot requested review from hector-vido and liangxia January 15, 2026 13:37

openshift-ci bot assigned smg247 Jan 15, 2026

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2026

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2026

coderabbitai bot reviewed Jan 15, 2026

View reviewed changes

stbenjam force-pushed the no-informing branch from 061d456 to 665c8a2 Compare January 15, 2026 17:10

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

jobrunaggregator: treat informing tests as always passing #4902

jobrunaggregator: treat informing tests as always passing #4902

stbenjam commented Jan 15, 2026

Uh oh!

openshift-ci-robot commented Jan 15, 2026

Uh oh!

coderabbitai bot commented Jan 15, 2026 •

edited

Loading

Uh oh!

smg247 commented Jan 15, 2026

Uh oh!

openshift-ci bot commented Jan 15, 2026

Uh oh!

coderabbitai bot left a comment

Uh oh!

openshift-ci-robot commented Jan 15, 2026

Uh oh!

openshift-ci bot commented Jan 15, 2026

Uh oh!

openshift-ci bot commented Jan 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jobrunaggregator: treat informing tests as always passing #4902

Are you sure you want to change the base?

jobrunaggregator: treat informing tests as always passing #4902

Conversation

stbenjam commented Jan 15, 2026

Uh oh!

openshift-ci-robot commented Jan 15, 2026

Uh oh!

coderabbitai bot commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

smg247 commented Jan 15, 2026

Uh oh!

openshift-ci bot commented Jan 15, 2026

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jan 15, 2026

Uh oh!

openshift-ci bot commented Jan 15, 2026

Uh oh!

openshift-ci bot commented Jan 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Jan 15, 2026 •

edited

Loading