Skip to content

TRT-2812: Fix synthetic test map key collision from PR #3768 #3799

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 1 commit into from
Jul 21, 2026
Merged

TRT-2812: Fix synthetic test map key collision from PR #3768 #3799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 9 additions & 8 deletions pkg/dataloader/prowloader/extract_test_cases_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/openshift/sippy/pkg/apis/junit"
sippyprocessingv1 "github.com/openshift/sippy/pkg/apis/sippyprocessing/v1"
"github.com/openshift/sippy/pkg/dataloader/prowloader/types"
)

func TestExtractTestCases(t *testing.T) {
Expand All @@ -15,7 +16,7 @@ func TestExtractTestCases(t *testing.T) {
tests := []struct {
name string
suite *junit.TestSuite
expected map[testCaseKey]*testCaseEntry
expected map[testCaseKey]*types.TestCaseEntry
}{
{
name: "passing test",
Expand All @@ -25,7 +26,7 @@ func TestExtractTestCases(t *testing.T) {
{Name: "test-a", Duration: 1.5},
},
},
expected: map[testCaseKey]*testCaseEntry{
expected: map[testCaseKey]*types.TestCaseEntry{
{SuiteName: "openshift-tests", TestName: "test-a"}: {
TestName: "test-a",
SuiteName: "openshift-tests",
Expand All @@ -42,7 +43,7 @@ func TestExtractTestCases(t *testing.T) {
{Name: "test-a", Duration: 2.0, FailureOutput: &junit.FailureOutput{Output: failMsg}},
},
},
expected: map[testCaseKey]*testCaseEntry{
expected: map[testCaseKey]*types.TestCaseEntry{
{SuiteName: "openshift-tests", TestName: "test-a"}: {
TestName: "test-a",
SuiteName: "openshift-tests",
Expand All @@ -60,7 +61,7 @@ func TestExtractTestCases(t *testing.T) {
{Name: "test-a", SkipMessage: &junit.SkipMessage{Message: "skipped"}},
},
},
expected: map[testCaseKey]*testCaseEntry{},
expected: map[testCaseKey]*types.TestCaseEntry{},
},
{
name: "flake from pass then fail",
Expand All @@ -71,7 +72,7 @@ func TestExtractTestCases(t *testing.T) {
{Name: "test-a", Duration: 2.0, FailureOutput: &junit.FailureOutput{Output: failMsg}},
},
},
expected: map[testCaseKey]*testCaseEntry{
expected: map[testCaseKey]*types.TestCaseEntry{
{SuiteName: "openshift-tests", TestName: "test-a"}: {
TestName: "test-a",
SuiteName: "openshift-tests",
Expand All @@ -97,7 +98,7 @@ func TestExtractTestCases(t *testing.T) {
},
},
},
expected: map[testCaseKey]*testCaseEntry{
expected: map[testCaseKey]*types.TestCaseEntry{
{SuiteName: "a.b", TestName: "c"}: {
TestName: "c",
SuiteName: "a.b",
Expand Down Expand Up @@ -128,7 +129,7 @@ func TestExtractTestCases(t *testing.T) {
},
},
},
expected: map[testCaseKey]*testCaseEntry{
expected: map[testCaseKey]*types.TestCaseEntry{
{SuiteName: "openshift-tests", TestName: "parent-test"}: {
TestName: "parent-test",
SuiteName: "openshift-tests",
Expand All @@ -147,7 +148,7 @@ func TestExtractTestCases(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
testCases := make(map[testCaseKey]*testCaseEntry)
testCases := make(map[testCaseKey]*types.TestCaseEntry)
extractTestCases(tt.suite, testCases)
assert.Equal(t, tt.expected, testCases)
})
Expand Down
24 changes: 6 additions & 18 deletions pkg/dataloader/prowloader/prow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"net/url"
"os"
"regexp"
"slices"
"strconv"
"sync"
"time"
Expand Down Expand Up @@ -42,6 +43,7 @@ import (
"github.com/openshift/sippy/pkg/dataloader/prowloader/gcs"
"github.com/openshift/sippy/pkg/dataloader/prowloader/github"
"github.com/openshift/sippy/pkg/dataloader/prowloader/testconversion"
"github.com/openshift/sippy/pkg/dataloader/prowloader/types"
"github.com/openshift/sippy/pkg/db"
"github.com/openshift/sippy/pkg/db/models"
"github.com/openshift/sippy/pkg/github/commenter"
Expand Down Expand Up @@ -1596,15 +1598,6 @@ type testCaseKey struct {
TestName string
}

// testCaseEntry holds raw test case data with string names before ID resolution.
type testCaseEntry struct {
TestName string
SuiteName string
Status int
Duration float64
Output *string
}

func (pl *ProwLoader) prowJobRunTestsFromGCS(ctx context.Context, pj *prow.ProwJob, id, prowJobID uint, prowJobRelease, path string, junitPaths []string) ([]prowJobRunTestRow, int, sippyprocessingv1.JobOverallResult, error) {
bkt := pl.gcsClient.Bucket(pj.Spec.DecorationConfig.GCSConfiguration.Bucket)
gcsJobRun := gcs.NewGCSJobRun(bkt, path)
Expand All @@ -1615,7 +1608,7 @@ func (pl *ProwLoader) prowJobRunTestsFromGCS(ctx context.Context, pj *prow.ProwJ
return nil, 0, "", err
}

testCases := make(map[testCaseKey]*testCaseEntry)
testCases := make(map[testCaseKey]*types.TestCaseEntry)
for _, suite := range suites.Suites {
if !db.IsSuiteImportable(suite.Name) {
log.Infof("skipping suite %q as it's not listed for import", suite.Name)
Expand All @@ -1624,12 +1617,7 @@ func (pl *ProwLoader) prowJobRunTestsFromGCS(ctx context.Context, pj *prow.ProwJ
extractTestCases(suite, testCases)
}

oldTestCases := make(map[string]*models.ProwJobRunTest, len(testCases))
for _, tc := range testCases {
oldTestCases[tc.TestName] = &models.ProwJobRunTest{
Status: tc.Status,
}
}
oldTestCases := slices.Collect(maps.Values(testCases))
syntheticSuite, jobResult := testconversion.ConvertProwJobRunToSyntheticTests(*pj, oldTestCases, pl.syntheticTestManager)

if !db.IsSuiteImportable(syntheticSuite.Name) {
Expand Down Expand Up @@ -1663,7 +1651,7 @@ func (pl *ProwLoader) prowJobRunTestsFromGCS(ctx context.Context, pj *prow.ProwJ
return results, failures, jobResult, nil
}

func extractTestCases(suite *junit.TestSuite, testCases map[testCaseKey]*testCaseEntry) {
func extractTestCases(suite *junit.TestSuite, testCases map[testCaseKey]*types.TestCaseEntry) {
for _, tc := range suite.TestCases {
if testidentification.IsIgnoredTest(tc.Name) {
continue
Expand All @@ -1682,7 +1670,7 @@ func extractTestCases(suite *junit.TestSuite, testCases map[testCaseKey]*testCas
key := testCaseKey{SuiteName: suite.Name, TestName: tc.Name}

if existing, ok := testCases[key]; !ok {
testCases[key] = &testCaseEntry{
testCases[key] = &types.TestCaseEntry{
TestName: tc.Name,
SuiteName: suite.Name,
Status: int(status),
Expand Down
62 changes: 34 additions & 28 deletions pkg/dataloader/prowloader/testconversion/testconversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ import (
"github.com/openshift/sippy/pkg/apis/junit"
"github.com/openshift/sippy/pkg/apis/prow"
v1 "github.com/openshift/sippy/pkg/apis/sippyprocessing/v1"
"github.com/openshift/sippy/pkg/db/models"
"github.com/openshift/sippy/pkg/dataloader/prowloader/types"
"github.com/openshift/sippy/pkg/synthetictests"
"github.com/openshift/sippy/pkg/testidentification"
)

func ConvertProwJobRunToSyntheticTests(pj prow.ProwJob, tests map[string]*models.ProwJobRunTest, manager synthetictests.SyntheticTestManager) (*junit.TestSuite, v1.JobOverallResult) {
func ConvertProwJobRunToSyntheticTests(pj prow.ProwJob, tests []*types.TestCaseEntry, manager synthetictests.SyntheticTestManager) (*junit.TestSuite, v1.JobOverallResult) {
jrr := v1.RawJobRunResult{
Job: pj.Spec.Job,
Errored: pj.Status.State == prow.ErrorState,
Expand All @@ -22,35 +22,41 @@ func ConvertProwJobRunToSyntheticTests(pj prow.ProwJob, tests map[string]*models
return syntheticTests, jrr.OverallResult
}

func testsToRawJobRunResult(jrr *v1.RawJobRunResult, tests map[string]*models.ProwJobRunTest) {
for name, test := range tests {
// Skip non-suite tests (e.g. prowjob-junit, step graph) — their
// failures don't represent real test signal.
if testidentification.IsNonSuiteTest(name) {
func testsToRawJobRunResult(jrr *v1.RawJobRunResult, tests []*types.TestCaseEntry) {
for _, tc := range tests {
if testidentification.IsNonSuiteTest(tc.SuiteName, tc.TestName) {
continue
}

switch v1.TestStatus(test.Status) {
switch v1.TestStatus(tc.Status) {
case v1.TestStatusSuccess, v1.TestStatusFlake: // success, flake(failed one or more times but ultimately succeeded)
switch {
case testidentification.IsOverallTest(name):
case testidentification.IsOverallTest(tc.TestName):
jrr.Succeeded = true
// if the overall job succeeded, install is always considered successful, even for jobs
// that don't have an explicitly defined install test.
jrr.InstallStatus = testidentification.Success
case testidentification.IsOperatorHealthTest(name):
if jrr.InstallStatus != testidentification.Failure {
jrr.InstallStatus = testidentification.Success
}
case testidentification.IsOperatorHealthTest(tc.TestName):
jrr.FinalOperatorStates = append(jrr.FinalOperatorStates, v1.OperatorState{
Name: testidentification.GetOperatorNameFromTest(name),
Name: testidentification.GetOperatorNameFromTest(tc.TestName),
State: testidentification.Success,
})
case testidentification.IsInstallStepEquivalent(name):
jrr.InstallStatus = testidentification.Success
case testidentification.IsUpgradeStartedTest(name):
case testidentification.IsInstallStepEquivalent(tc.TestName):
if jrr.InstallStatus != testidentification.Failure {
jrr.InstallStatus = testidentification.Success
}
case testidentification.IsUpgradeStartedTest(tc.TestName):
jrr.UpgradeStarted = true
case testidentification.IsOperatorsUpgradedTest(name):
jrr.UpgradeForOperatorsStatus = testidentification.Success
case testidentification.IsMachineConfigPoolsUpgradedTest(name):
jrr.UpgradeForMachineConfigPoolsStatus = testidentification.Success
case testidentification.IsOperatorsUpgradedTest(tc.TestName):
if jrr.UpgradeForOperatorsStatus != testidentification.Failure {
jrr.UpgradeForOperatorsStatus = testidentification.Success
}
case testidentification.IsMachineConfigPoolsUpgradedTest(tc.TestName):
if jrr.UpgradeForMachineConfigPoolsStatus != testidentification.Failure {
jrr.UpgradeForMachineConfigPoolsStatus = testidentification.Success
}
default:
// Any other non-special test contributes to overall test status
if jrr.TestsStatus == "" {
Expand All @@ -60,26 +66,26 @@ func testsToRawJobRunResult(jrr *v1.RawJobRunResult, tests map[string]*models.Pr
case v1.TestStatusFailure:
// only add the failing test and name if it has predictive value. We excluded all the non-predictive ones above except for these
// which we use to set various JobRunResult markers
if !testidentification.IsOverallTest(name) {
jrr.FailedTestNames = append(jrr.FailedTestNames, name)
if !testidentification.IsOverallTest(tc.TestName) {
jrr.FailedTestNames = append(jrr.FailedTestNames, tc.TestName)
jrr.TestFailures++
}

switch {
case testidentification.IsOverallTest(name):
case testidentification.IsOverallTest(tc.TestName):
jrr.Failed = true
case testidentification.IsOperatorHealthTest(name):
case testidentification.IsOperatorHealthTest(tc.TestName):
jrr.FinalOperatorStates = append(jrr.FinalOperatorStates, v1.OperatorState{
Name: testidentification.GetOperatorNameFromTest(name),
Name: testidentification.GetOperatorNameFromTest(tc.TestName),
State: testidentification.Failure,
})
case testidentification.IsInstallStepEquivalent(name):
case testidentification.IsInstallStepEquivalent(tc.TestName):
jrr.InstallStatus = testidentification.Failure
case testidentification.IsUpgradeStartedTest(name):
case testidentification.IsUpgradeStartedTest(tc.TestName):
jrr.UpgradeStarted = true // this is still true because we definitely started
case testidentification.IsOperatorsUpgradedTest(name):
case testidentification.IsOperatorsUpgradedTest(tc.TestName):
jrr.UpgradeForOperatorsStatus = testidentification.Failure
case testidentification.IsMachineConfigPoolsUpgradedTest(name):
case testidentification.IsMachineConfigPoolsUpgradedTest(tc.TestName):
jrr.UpgradeForMachineConfigPoolsStatus = testidentification.Failure
default:
jrr.TestsStatus = testidentification.Failure
Expand Down
Loading