🤖 TRT-2622: test result aggregation: properties#5163
🤖 TRT-2622: test result aggregation: properties#5163sosiouxme wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@sosiouxme: This pull request references TRT-2622 which is a valid 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: sosiouxme 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 |
📝 WalkthroughWalkthroughIntroduces a unified ChangesjUnit Type Unification, Censoring, Propagation, and Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Pull request overview
This PR extends the JUnit aggregation flow so aggregated TestCases can carry forward <properties> from the source testcases (using the first encountered testcase instance as the representative), and consolidates suite/testcase property modeling in pkg/junit.
Changes:
- Unify test suite properties under a shared
junit.Propertytype and addTestCase.Propertiessupport in the JUnit types. - Propagate
TestCase.Propertiesduring aggregation (first testcase instance wins). - Add/update fixtures and tests to reflect the new struct fields and aggregation behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/junit/types.go | Unifies property modeling and adds TestCase.Properties to the JUnit data model. |
| pkg/junit/censor_test.go | Updates censor test to use the unified Property type. |
| pkg/junit/testdata/zz_fixture_TestCensorTestSuite.yaml | Updates golden fixture for the new TestCase.Properties field presence. |
| pkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.go | Copies testcase properties into the aggregated testcase (first source wins). |
| pkg/jobrunaggregator/jobrunaggregatoranalyzer/analyzer_test.go | Adds coverage for properties propagation behavior in aggregation. |
Comments suppressed due to low confidence (1)
pkg/junit/types.go:56
- Renaming/removing the exported
TestSuitePropertytype is a breaking API change for any downstream code importingpkg/junit. Consider keeping backwards compatibility by adding a type alias (e.g.,type TestSuiteProperty = Property) or retaining the old name as a deprecated wrapper type.
// Properties holds other properties of the test suite as a mapping of name to value
Properties []*Property `xml:"properties>property,omitempty"`
// TestCases are the test cases contained in the test suite
TestCases []*TestCase `xml:"testcase"`
// Children holds nested test suites
Children []*TestSuite `xml:"testsuite"`
}
// Property contains a mapping of a property name to a value
type Property struct {
XMLName xml.Name `xml:"property"`
Name string `xml:"name,attr"`
Value string `xml:"value,attr"`
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/junit/types.go (1)
41-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep a compatibility alias for
TestSuiteProperty.
pkg/junitis already consumed fromopenshift/release, and those callers still construct[]*junit.TestSuiteProperty. Replacing the exported type name here turns this into a cross-repo compile break even though the XML shape is effectively the same. A temporary alias keeps this PR safe while downstream code migrates.Suggested compatibility shim
// Property contains a mapping of a property name to a value type Property struct { XMLName xml.Name `xml:"property"` Name string `xml:"name,attr"` Value string `xml:"value,attr"` } + +// TestSuiteProperty is kept as an alias while downstream consumers migrate. +type TestSuiteProperty = Property🤖 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 `@pkg/junit/types.go` around lines 41 - 55, Add a backwards-compatible exported alias named TestSuiteProperty that points to the existing Property struct so callers constructing []*junit.TestSuiteProperty continue to compile; specifically, add a type alias declaration like "type TestSuiteProperty = Property" near the Property type (with a short comment explaining it’s a compatibility shim) and keep the XML tags and original Property definition unchanged.
🤖 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 `@pkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.go`:
- Around line 238-247: The current logic uses len(combined.Properties) to decide
whether to copy properties from toAdd, which can copy properties from a later
testcase if the first aggregated testcase had no properties; instead, introduce
or use a boolean sentinel that marks whether the first testcase has already been
aggregated (e.g., a field or local flag like firstAggregated) and change the
condition to check that flag when deciding to copy properties from toAdd to
combined; update the aggregation site where combined and toAdd are handled so
that when you process the very first testcase you set firstAggregated=true and
only allow properties to be copied when firstAggregated is false (use the
symbols combined, toAdd, combined.Properties, and the new firstAggregated flag
in your changes).
In `@pkg/junit/censor_test.go`:
- Around line 16-18: CensorTestSuite currently redacts suite-level Properties
but omits testcase-level Properties on TestSuite.TestCases; update the
CensorTestSuite function to iterate over each TestCase.Properties and call
censored(censor, ...) for both Name and Value (use the same pattern as suite
Properties), ensuring you reference TestSuite.TestCases[i].Properties and the
censored(censor, ...) helper; then update pkg/junit/censor_test.go to add
Properties with secret values to the testcase fixtures (the cases around the
existing lines where Properties are asserted) and extend the tests to verify
those testcase-level property names and values are redacted.
---
Outside diff comments:
In `@pkg/junit/types.go`:
- Around line 41-55: Add a backwards-compatible exported alias named
TestSuiteProperty that points to the existing Property struct so callers
constructing []*junit.TestSuiteProperty continue to compile; specifically, add a
type alias declaration like "type TestSuiteProperty = Property" near the
Property type (with a short comment explaining it’s a compatibility shim) and
keep the XML tags and original Property definition unchanged.
🪄 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: Enterprise
Run ID: 227929de-8311-4572-89c9-b07a79b2ee76
📒 Files selected for processing (5)
pkg/jobrunaggregator/jobrunaggregatoranalyzer/analyzer_test.gopkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.gopkg/junit/censor_test.gopkg/junit/testdata/zz_fixture_TestCensorTestSuite.yamlpkg/junit/types.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/junit/censor.go`:
- Around line 17-19: In the loop that iterates testSuite.TestCases[i].Properties
(in pkg/junit/censor.go) you must guard against nil property entries and censor
both the Value and Name fields; add a nil-check for
testSuite.TestCases[i].Properties[j] before dereferencing, and call
censored(censor, ...) on both Properties[j].Value and Properties[j].Name so
user-provided names cannot leak secrets (keep using the existing censored
function).
🪄 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: Enterprise
Run ID: b333610c-0470-4031-9164-61ea21a03a9f
📒 Files selected for processing (3)
pkg/junit/censor.gopkg/junit/censor_test.gopkg/junit/testdata/zz_fixture_TestCensorTestSuite.yaml
✅ Files skipped from review due to trivial changes (1)
- pkg/junit/censor_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/junit/testdata/zz_fixture_TestCensorTestSuite.yaml
|
/override ci/prow/images it's not me breaking it |
|
Scheduling tests matching the |
|
@sosiouxme: Overrode contexts on behalf of sosiouxme: ci/prow/images 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 |
|
/cc |
|
/test e2e |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/test e2e |
|
@sosiouxme: The following tests failed, say
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. |
The aggregator now propagates properties of the testcases aggregated, choosing the properties of the first testcase instance as representative in the aggregated testcase.
🤖 Assisted by Claude Code
sosiouxme
force-pushed
the
20260506-TRT-2622-aggregator-property
branch
from
1f431f2 to
991c87d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/junit/types.go (1)
41-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the old exported property type during rollout.
This renames a public model type from
TestSuitePropertytoProperty, and linked-repo findings already showopenshift/releaseconsumers intools/junitreportandtools/gotest2junitstill usingTestSuiteProperty. Please keep a temporary compatibility alias here or land the companion updates in lockstep, otherwise this API change is likely to leave those consumers out of sync.🤖 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 `@pkg/junit/types.go` around lines 41 - 55, You renamed the exported type TestSuiteProperty to Property which breaks downstream consumers; add a temporary exported compatibility alias by declaring "type TestSuiteProperty = Property" alongside the new Property type (keep the existing Property struct unchanged and ensure the alias is exported and documented as temporary) so code referencing TestSuiteProperty continues to compile while consumers are migrated.
🧹 Nitpick comments (1)
pkg/jobrunaggregator/jobrunaggregatoranalyzer/analyzer_test.go (1)
483-515: ⚡ Quick winAssert that properties are copied, not aliased.
These assertions still pass if
aggregateTestCasekeepssource.Propertiesby reference. A post-aggregation mutation onsourcewould catch shared-slice/shared-element bugs and lock in the “representative properties come from the first testcase” behavior.Suggested test tightening
err := aggregateTestCase("suite", combined, "logs/job", "run1", source) assert.NoError(t, err) assert.Equal(t, 2, len(combined.Properties)) assert.Equal(t, "lifecycle", combined.Properties[0].Name) assert.Equal(t, "informing", combined.Properties[0].Value) assert.Equal(t, "owner", combined.Properties[1].Name) assert.Equal(t, "team-platform", combined.Properties[1].Value) + + source.Properties[0].Value = "mutated-after-aggregation" + assert.Equal(t, "informing", combined.Properties[0].Value)🤖 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 `@pkg/jobrunaggregator/jobrunaggregatoranalyzer/analyzer_test.go` around lines 483 - 515, The test currently only checks values but not that Properties were deep-copied; after calling aggregateTestCase with source (and source2), mutate source.Properties and/or the Property elements (e.g., change Values or replace the slice) and then assert combined.Properties still retain the original values; update TestAggregateTestCasePropagatesProperties to perform this post-aggregation mutation and re-check combined to ensure aggregateTestCase (the function under test) copies properties rather than aliasing source.Properties or its elements.
🤖 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.
Outside diff comments:
In `@pkg/junit/types.go`:
- Around line 41-55: You renamed the exported type TestSuiteProperty to Property
which breaks downstream consumers; add a temporary exported compatibility alias
by declaring "type TestSuiteProperty = Property" alongside the new Property type
(keep the existing Property struct unchanged and ensure the alias is exported
and documented as temporary) so code referencing TestSuiteProperty continues to
compile while consumers are migrated.
---
Nitpick comments:
In `@pkg/jobrunaggregator/jobrunaggregatoranalyzer/analyzer_test.go`:
- Around line 483-515: The test currently only checks values but not that
Properties were deep-copied; after calling aggregateTestCase with source (and
source2), mutate source.Properties and/or the Property elements (e.g., change
Values or replace the slice) and then assert combined.Properties still retain
the original values; update TestAggregateTestCasePropagatesProperties to perform
this post-aggregation mutation and re-check combined to ensure aggregateTestCase
(the function under test) copies properties rather than aliasing
source.Properties or its elements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9cc2a237-8975-4c16-abfa-6087b527e3a1
📒 Files selected for processing (9)
pkg/jobrunaggregator/jobrunaggregatoranalyzer/analyzer_test.gopkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.gopkg/junit/censor.gopkg/junit/censor_test.gopkg/junit/testdata/zz_fixture_TestCensorTestSuite.yamlpkg/junit/types.gotest/e2e/observer/artifacts/multi-observers-junit_operator.xmltest/e2e/observer/artifacts/simple-observer-junit_operator.xmltest/e2e/simple/artifacts/junit_operator.xml
✅ Files skipped from review due to trivial changes (3)
- test/e2e/simple/artifacts/junit_operator.xml
- test/e2e/observer/artifacts/multi-observers-junit_operator.xml
- test/e2e/observer/artifacts/simple-observer-junit_operator.xml
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.go
- pkg/junit/censor_test.go
- pkg/junit/testdata/zz_fixture_TestCensorTestSuite.yaml
- pkg/junit/censor.go
|
/test e2e |
The aggregator now propagates properties of the testcases aggregated, choosing the properties of the first testcase instance as representative in the aggregated testcase.
🤖 Assisted by Claude Code
What changed (practical effect): The test-result aggregator now carries test-case metadata (lifecycle and properties) from source testcases into aggregated testcases. When multiple instances of the same testcase are combined the aggregator will adopt the lifecycle and the Properties list from the first observed testcase instance and will not overwrite them on subsequent aggregations. Censoring was extended so property names and values are redacted alongside suite/case names, messages and stdout/stderr.
Component(s) affected: jobrunaggregator (jobrunaggregatoranalyzer) and the junit package used to parse/manipulate JUnit XML.
Behavioral impact for CI users/operators:
Key implementation notes (what changed in code/tests):
Operator considerations: downstream tooling that consumes aggregated JUnit should expect testcase-level property elements and lifecycle attributes to appear. If tooling relies on merging or preferring newer property values, it may need to be updated because the aggregator picks the first instance as authoritative.