Skip to content

🤖 TRT-2622: test result aggregation: properties #5163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 3 commits into from
May 15, 2026
Merged

🤖 TRT-2622: test result aggregation: properties #5163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
95f768e
🤖 TRT-2622: test result aggregation: properties
sosiouxme May 6, 2026
991c87d
🤖 junit/censor.go: refactor, censor property values
sosiouxme May 6, 2026
1ec51b3
fix TestAggregateTestCasePropagatesProperties
sosiouxme May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions pkg/jobrunaggregator/jobrunaggregatoranalyzer/analyzer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,39 @@ func TestAggregateTestCasePropagatesLifecycle(t *testing.T) {
assert.Equal(t, "informing", combined.Lifecycle)
}

func TestAggregateTestCasePropagatesProperties(t *testing.T) {
combined := &junit.TestCase{Name: "test-with-properties"}

source := &junit.TestCase{
Name: "test-with-properties",
Properties: []*junit.Property{
{Name: "lifecycle", Value: "informing"},
{Name: "owner", Value: "team-platform"},
},
}

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)

// A different second aggregation should not overwrite the properties
// (Usually the important properties would be the same anyway, but where properties differ, just one gets picked)
source2 := &junit.TestCase{
Name: "test-with-properties",
Properties: []*junit.Property{
{Name: "lifecycle", Value: "blocking"},
},
}
err = aggregateTestCase("suite", combined, "logs/job", "run2", source2)
assert.NoError(t, err)
// Properties should remain unchanged from first aggregation
assert.Equal(t, 2, len(combined.Properties))
assert.Equal(t, "lifecycle", combined.Properties[0].Name)
assert.Equal(t, "informing", combined.Properties[0].Value)
Comment on lines +510 to +513
Copy link
Copy Markdown
Member

@smg247 smg247 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we not checking the owner still? I also don't see how we verified this didn't overwrite since lifecycle is set to informing in both cases.

Copy link
Copy Markdown
Member Author

@sosiouxme sosiouxme May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, you're right. It's not testing what it's supposed to.
TBH I let Claude write the test cases and didn't much examine them.
Also TBH this part of the unit test seems like overkill. Claude seems to have gotten overly focused on a minor detail. I don't really care which representative test case supplies the properties, they should all be the same anyway.

Copy link
Copy Markdown
Member

@smg247 smg247 May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to just remove this one then

}

func TestInformingTestFailureMessage(t *testing.T) {
suite := &junit.TestSuites{
Suites: []*junit.TestSuite{
Expand Down
11 changes: 11 additions & 0 deletions pkg/jobrunaggregator/jobrunaggregatoranalyzer/junit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,5 +235,16 @@ func aggregateTestCase(testSuiteName string, combined *junit.TestCase, jobGCSBuc
combined.Lifecycle = toAdd.Lifecycle
}

// Propagate properties from source test case to the combined one
if len(combined.Properties) == 0 && len(toAdd.Properties) > 0 {
combined.Properties = make([]*junit.Property, len(toAdd.Properties))
for i, prop := range toAdd.Properties {
combined.Properties[i] = &junit.Property{
Name: prop.Name,
Value: prop.Value,
}
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

return nil
}
39 changes: 22 additions & 17 deletions pkg/junit/censor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,35 @@ func CensorTestSuite(censor secretutil.Censorer, testSuite *TestSuite) {
if testSuite == nil {
return
}
testSuite.Name = censored(censor, testSuite.Name)
for i := range testSuite.Properties {
testSuite.Properties[i].Name = censored(censor, testSuite.Properties[i].Name)
testSuite.Properties[i].Value = censored(censor, testSuite.Properties[i].Value)
censorStr(censor, &testSuite.Name)
for i, prop := range testSuite.Properties {
censorStr(censor, &prop.Name, &prop.Value)
testSuite.Properties[i] = prop
}
for i := range testSuite.TestCases {
testSuite.TestCases[i].Name = censored(censor, testSuite.TestCases[i].Name)
if testSuite.TestCases[i].SkipMessage != nil {
testSuite.TestCases[i].SkipMessage.Message = censored(censor, testSuite.TestCases[i].SkipMessage.Message)
for i, testCase := range testSuite.TestCases {
censorStr(censor, &testCase.Name)
for j, prop := range testCase.Properties {
censorStr(censor, &prop.Name, &prop.Value)
testCase.Properties[j] = prop
}
if testSuite.TestCases[i].FailureOutput != nil {
testSuite.TestCases[i].FailureOutput.Output = censored(censor, testSuite.TestCases[i].FailureOutput.Output)
testSuite.TestCases[i].FailureOutput.Message = censored(censor, testSuite.TestCases[i].FailureOutput.Message)
if testCase.SkipMessage != nil {
censorStr(censor, &testCase.SkipMessage.Message)
}
testSuite.TestCases[i].SystemOut = censored(censor, testSuite.TestCases[i].SystemOut)
testSuite.TestCases[i].SystemErr = censored(censor, testSuite.TestCases[i].SystemErr)
if testCase.FailureOutput != nil {
censorStr(censor, &testCase.FailureOutput.Output, &testCase.FailureOutput.Message)
}
censorStr(censor, &testCase.SystemOut, &testCase.SystemErr)
testSuite.TestCases[i] = testCase
}
for i := range testSuite.Children {
CensorTestSuite(censor, testSuite.Children[i])
}
}

func censored(censor secretutil.Censorer, value string) string {
raw := []byte(value)
censor.Censor(&raw)
return string(raw)
func censorStr(censor secretutil.Censorer, values ...*string) {
for _, val := range values {
raw := []byte(*val)
censor.Censor(&raw)
*val = string(raw)
}
}
36 changes: 27 additions & 9 deletions pkg/junit/censor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,16 @@ func TestCensorTestSuite(t *testing.T) {
censorer.Refresh("secret")
input := TestSuite{
Name: "some secret",
Properties: []*TestSuiteProperty{
Properties: []*Property{
{Name: "secret things", Value: "secret values"},
{Name: "also secret things", Value: "really secret values"},
Comment thread
coderabbitai[bot] marked this conversation as resolved.
},
TestCases: []*TestCase{
{
Name: "somehow secret",
Name: "somehow secret",
Properties: []*Property{
{Name: "lifecycle", Value: "secret value"},
},
SkipMessage: &SkipMessage{Message: "skipped due to secret"},
FailureOutput: &FailureOutput{
Message: "failed due to secret",
Expand All @@ -29,7 +32,10 @@ func TestCensorTestSuite(t *testing.T) {
SystemErr: "error containing secret",
},
{
Name: "somehow also secret",
Name: "somehow also secret",
Properties: []*Property{
{Name: "owner", Value: "also secret value"},
},
SkipMessage: &SkipMessage{Message: "also skipped due to secret"},
FailureOutput: &FailureOutput{
Message: "also failed due to secret",
Expand All @@ -42,13 +48,16 @@ func TestCensorTestSuite(t *testing.T) {
Children: []*TestSuite{
{
Name: "some nested secret",
Properties: []*TestSuiteProperty{
Properties: []*Property{
{Name: "nested secret things", Value: "nested secret values"},
{Name: "also nested secret things", Value: "really nested secret values"},
},
TestCases: []*TestCase{
{
Name: "somehow nested secret",
Name: "somehow nested secret",
Properties: []*Property{
{Name: "lifecycle", Value: "nested secret value"},
},
SkipMessage: &SkipMessage{Message: "skipped due to nested secret"},
FailureOutput: &FailureOutput{
Message: "failed due to nested secret",
Expand All @@ -58,7 +67,10 @@ func TestCensorTestSuite(t *testing.T) {
SystemErr: "error containing nested secret",
},
{
Name: "somehow also nested secret",
Name: "somehow also nested secret",
Properties: []*Property{
{Name: "owner", Value: "also nested secret value"},
},
SkipMessage: &SkipMessage{Message: "also skipped due to nested secret"},
FailureOutput: &FailureOutput{
Message: "also failed due to nested secret",
Expand All @@ -71,13 +83,16 @@ func TestCensorTestSuite(t *testing.T) {
Children: []*TestSuite{
{
Name: "some very nested secret",
Properties: []*TestSuiteProperty{
Properties: []*Property{
{Name: "very nested secret things", Value: "very nested secret values"},
{Name: "also very nested secret things", Value: "really very nested secret values"},
},
TestCases: []*TestCase{
{
Name: "somehow very nested secret",
Name: "somehow very nested secret",
Properties: []*Property{
{Name: "lifecycle", Value: "very nested secret value"},
},
SkipMessage: &SkipMessage{Message: "skipped due to very nested secret"},
FailureOutput: &FailureOutput{
Message: "failed due to very nested secret",
Expand All @@ -87,7 +102,10 @@ func TestCensorTestSuite(t *testing.T) {
SystemErr: "error containing very nested secret",
},
{
Name: "somehow also very nested secret",
Name: "somehow also very nested secret",
Properties: []*Property{
{Name: "owner", Value: "also very nested secret value"},
},
SkipMessage: &SkipMessage{Message: "also skipped due to very nested secret"},
FailureOutput: &FailureOutput{
Message: "also failed due to very nested secret",
Expand Down
36 changes: 36 additions & 0 deletions pkg/junit/testdata/zz_fixture_TestCensorTestSuite.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ Children:
Space: ""
Lifecycle: ""
Name: somehow very nested XXXXXX
Properties:
- Name: lifecycle
Value: very nested XXXXXX value
XMLName:
Local: ""
Space: ""
SkipMessage:
Message: skipped due to very nested XXXXXX
XMLName:
Expand All @@ -48,6 +54,12 @@ Children:
Space: ""
Lifecycle: ""
Name: somehow also very nested XXXXXX
Properties:
- Name: owner
Value: also very nested XXXXXX value
XMLName:
Local: ""
Space: ""
SkipMessage:
Message: also skipped due to very nested XXXXXX
XMLName:
Expand Down Expand Up @@ -88,6 +100,12 @@ Children:
Space: ""
Lifecycle: ""
Name: somehow nested XXXXXX
Properties:
- Name: lifecycle
Value: nested XXXXXX value
XMLName:
Local: ""
Space: ""
SkipMessage:
Message: skipped due to nested XXXXXX
XMLName:
Expand All @@ -108,6 +126,12 @@ Children:
Space: ""
Lifecycle: ""
Name: somehow also nested XXXXXX
Properties:
- Name: owner
Value: also nested XXXXXX value
XMLName:
Local: ""
Space: ""
SkipMessage:
Message: also skipped due to nested XXXXXX
XMLName:
Expand Down Expand Up @@ -148,6 +172,12 @@ TestCases:
Space: ""
Lifecycle: ""
Name: somehow XXXXXX
Properties:
- Name: lifecycle
Value: XXXXXX value
XMLName:
Local: ""
Space: ""
SkipMessage:
Message: skipped due to XXXXXX
XMLName:
Expand All @@ -168,6 +198,12 @@ TestCases:
Space: ""
Lifecycle: ""
Name: somehow also XXXXXX
Properties:
- Name: owner
Value: also XXXXXX value
XMLName:
Local: ""
Space: ""
SkipMessage:
Message: also skipped due to XXXXXX
XMLName:
Expand Down
9 changes: 6 additions & 3 deletions pkg/junit/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ type TestSuite struct {
Duration float64 `xml:"time,attr"`

// Properties holds other properties of the test suite as a mapping of name to value
Properties []*TestSuiteProperty `xml:"properties>property,omitempty"`
Properties []*Property `xml:"properties>property,omitempty"`

// TestCases are the test cases contained in the test suite
TestCases []*TestCase `xml:"testcase"`
Expand All @@ -47,8 +47,8 @@ type TestSuite struct {
Children []*TestSuite `xml:"testsuite"`
}

// TestSuiteProperty contains a mapping of a property name to a value
type TestSuiteProperty struct {
// Property contains a mapping of a property name to a value
type Property struct {
XMLName xml.Name `xml:"property"`

Name string `xml:"name,attr"`
Expand All @@ -71,6 +71,9 @@ type TestCase struct {
// Lifecycle indicates the test lifecycle phase (e.g. "informing" or "blocking")
Lifecycle string `xml:"lifecycle,attr,omitempty"`

// Properties holds other properties of the test case as a mapping of name to value
Properties []*Property `xml:"properties>property,omitempty"`

Comment thread
sosiouxme marked this conversation as resolved.
// SkipMessage holds the reason why the test was skipped
SkipMessage *SkipMessage `xml:"skipped"`

Expand Down
24 changes: 19 additions & 5 deletions test/e2e/observer/artifacts/multi-observers-junit_operator.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,35 @@
<testsuites>
<testsuite name="step graph" tests="9" skipped="0" failures="1" time="whatever">
<properties></properties>
<testcase name="Find the input image os and tag it into the pipeline" time="whatever"></testcase>
<testcase name="Run multi-stage test multi-observers - multi-observers-check-shared-dir container test" time="whatever"></testcase>
<testcase name="Run multi-stage test multi-observers - multi-observers-create-kubeconfig container test" time="whatever"></testcase>
<testcase name="Find the input image os and tag it into the pipeline" time="whatever">
<properties></properties>
</testcase>
<testcase name="Run multi-stage test multi-observers - multi-observers-check-shared-dir container test" time="whatever">
<properties></properties>
</testcase>
<testcase name="Run multi-stage test multi-observers - multi-observers-create-kubeconfig container test" time="whatever">
<properties></properties>
</testcase>
<testcase name="Run multi-stage test multi-observers - multi-observers-failing-observer container test" time="whatever">
<properties></properties>
<failure message="">+ echo &#39;this is going to fail&#39;&#xA;this is going to fail&#xA;+ exit 1&#xA;{&#34;component&#34;:&#34;entrypoint&#34;,&#34;error&#34;:&#34;wrapped process failed: exit status 1&#34;,&#34;file&#34;:&#34;sigs.k8s.io/prow/pkg/entrypoint/run.go&#34;,&#34;func&#34;:&#34;sigs.k8s.io/prow/pkg/entrypoint.Options.internalRun&#34;,&#34;level&#34;:&#34;error&#34;,&#34;msg&#34;:&#34;Error executing test process&#34;,&#34;severity&#34;:&#34;error&#34;,&#34;time&#34;:&#34;whatever&#34;}&#xA;error: failed to execute wrapped command: exit status 1&#xA;</failure>
</testcase>
<testcase name="Run multi-stage test multi-observers - multi-observers-inject-observer container test" time="whatever"></testcase>
<testcase name="Run multi-stage test multi-observers - multi-observers-observer container test" time="whatever"></testcase>
<testcase name="Run multi-stage test multi-observers - multi-observers-inject-observer container test" time="whatever">
<properties></properties>
</testcase>
<testcase name="Run multi-stage test multi-observers - multi-observers-observer container test" time="whatever">
<properties></properties>
</testcase>
<testcase name="Run multi-stage test post phase" time="whatever">
<properties></properties>
<system-out>The collected steps of multi-stage phase post.</system-out>
</testcase>
<testcase name="Run multi-stage test pre phase" time="whatever">
<properties></properties>
<system-out>The collected steps of multi-stage phase pre.</system-out>
</testcase>
<testcase name="Run multi-stage test test phase" time="whatever">
<properties></properties>
<system-out>The collected steps of multi-stage phase test.</system-out>
</testcase>
</testsuite>
Expand Down
19 changes: 15 additions & 4 deletions test/e2e/observer/artifacts/simple-observer-junit_operator.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,29 @@
<testsuites>
<testsuite name="step graph" tests="7" skipped="0" failures="0" time="whatever">
<properties></properties>
<testcase name="Find the input image os and tag it into the pipeline" time="whatever"></testcase>
<testcase name="Find the input image os and tag it into the pipeline" time="whatever">
<properties></properties>
</testcase>
<testcase name="Run multi-stage test post phase" time="whatever">
<properties></properties>
<system-out>The collected steps of multi-stage phase post.</system-out>
</testcase>
<testcase name="Run multi-stage test pre phase" time="whatever">
<properties></properties>
<system-out>The collected steps of multi-stage phase pre.</system-out>
</testcase>
<testcase name="Run multi-stage test test phase" time="whatever">
<properties></properties>
<system-out>The collected steps of multi-stage phase test.</system-out>
</testcase>
<testcase name="Run multi-stage test with-observer - with-observer-check-shared-dir container test" time="whatever"></testcase>
<testcase name="Run multi-stage test with-observer - with-observer-create-kubeconfig container test" time="whatever"></testcase>
<testcase name="Run multi-stage test with-observer - with-observer-observer container test" time="whatever"></testcase>
<testcase name="Run multi-stage test with-observer - with-observer-check-shared-dir container test" time="whatever">
<properties></properties>
</testcase>
<testcase name="Run multi-stage test with-observer - with-observer-create-kubeconfig container test" time="whatever">
<properties></properties>
</testcase>
<testcase name="Run multi-stage test with-observer - with-observer-observer container test" time="whatever">
<properties></properties>
</testcase>
</testsuite>
</testsuites>
Loading