🤖 TRT-2622: test result aggregation: properties#5163
🤖 TRT-2622: test result aggregation: properties#5163sosiouxme wants to merge 3 commits intoopenshift:mainfrom
Conversation
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
|
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReworks jUnit types to introduce a unified ChangesjUnit Type Unification, Censoring, Propagation, and Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (13 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
🤖 Assisted by Claude Code
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. |
|
@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
Summary by CodeRabbit
New Features
Tests