From c349ea631e0fe835055122c95a996178ad2aa572 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Thu, 27 Aug 2026 19:41:31 +0000 Subject: [PATCH] fix(componentreadiness): make cell status aggregation deterministic --- .../componentreadiness/component_report.go | 4 +- .../component_report_test.go | 51 ++++++++++++++++ pkg/apis/api/componentreport/crtest/types.go | 20 +++++++ .../api/componentreport/crtest/types_test.go | 58 +++++++++++++++++++ 4 files changed, 130 insertions(+), 3 deletions(-) diff --git a/pkg/api/componentreadiness/component_report.go b/pkg/api/componentreadiness/component_report.go index 36e457923e..0f3d9b3ade 100644 --- a/pkg/api/componentreadiness/component_report.go +++ b/pkg/api/componentreadiness/component_report.go @@ -455,9 +455,7 @@ type cellStatus struct { func getNewCellStatus(testID crtest.Identification, testStats testdetails.TestComparison, existingCellStatus *cellStatus, includeAllTests bool) cellStatus { var newCellStatus cellStatus if existingCellStatus != nil { - if (testStats.ReportStatus < crtest.NotSignificant && testStats.ReportStatus < existingCellStatus.status) || - (existingCellStatus.status == crtest.NotSignificant && testStats.ReportStatus == crtest.SignificantImprovement) { - // We want to show the significant improvement if assessment is not regression + if crtest.CompareCellStatus(testStats.ReportStatus, existingCellStatus.status) < 0 { newCellStatus.status = testStats.ReportStatus } else { newCellStatus.status = existingCellStatus.status diff --git a/pkg/api/componentreadiness/component_report_test.go b/pkg/api/componentreadiness/component_report_test.go index 3b5a8bbbc5..dd8ceb7ad2 100644 --- a/pkg/api/componentreadiness/component_report_test.go +++ b/pkg/api/componentreadiness/component_report_test.go @@ -1869,5 +1869,56 @@ func Test_componentReportGenerator_assessComponentStatus(t *testing.T) { } } +func TestGetNewCellStatusOrderIndependent(t *testing.T) { + testID := crtest.Identification{ + RowIdentification: crtest.RowIdentification{ + Component: "component", + Capability: "cap", + TestName: "test", + }, + } + makeStats := func(s crtest.Status) testdetails.TestComparison { + return testdetails.TestComparison{ReportStatus: s} + } + statusForOrder := func(first, second crtest.Status) crtest.Status { + cell := getNewCellStatus(testID, makeStats(first), nil, false) + cell = getNewCellStatus(testID, makeStats(second), &cell, false) + return cell.status + } + + tests := []struct { + name string + first crtest.Status + second crtest.Status + want crtest.Status + }{ + { + name: "NotSignificant and MissingBasis", + first: crtest.NotSignificant, + second: crtest.MissingBasis, + want: crtest.NotSignificant, + }, + { + name: "NotSignificant and MissingSample", + first: crtest.NotSignificant, + second: crtest.MissingSample, + want: crtest.MissingSample, + }, + { + name: "NotSignificant and SignificantImprovement", + first: crtest.NotSignificant, + second: crtest.SignificantImprovement, + want: crtest.SignificantImprovement, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, statusForOrder(tt.first, tt.second)) + assert.Equal(t, tt.want, statusForOrder(tt.second, tt.first)) + }) + } +} + // TestCopyIncludeVariantsAndRemoveOverrides moved to dataprovider/bigquery package // where the function now lives. diff --git a/pkg/apis/api/componentreport/crtest/types.go b/pkg/apis/api/componentreport/crtest/types.go index 95ea3f211f..d19af8e774 100644 --- a/pkg/apis/api/componentreport/crtest/types.go +++ b/pkg/apis/api/componentreport/crtest/types.go @@ -1,6 +1,7 @@ package crtest import ( + "cmp" "sort" "strings" "time" @@ -63,6 +64,25 @@ const ( SignificantImprovement Status = 300 ) +// CompareCellStatus compares two statuses for aggregation into a Component +// Readiness cell. It returns a negative value when first wins cell aggregation, +// zero when the statuses have equal precedence, and a positive value when +// second wins cell aggregation. Lower raw status values win, except that +// SignificantImprovement wins over every other status greater than or equal to +// NotSignificant. +func CompareCellStatus(first, second Status) int { + if first == second { + return 0 + } + if first == SignificantImprovement && second >= NotSignificant { + return -1 + } + if second == SignificantImprovement && first >= NotSignificant { + return 1 + } + return cmp.Compare(first, second) +} + func StringForStatus(s Status) string { switch s { case ExtremeRegression: diff --git a/pkg/apis/api/componentreport/crtest/types_test.go b/pkg/apis/api/componentreport/crtest/types_test.go index c9398d6189..53e3c3e341 100644 --- a/pkg/apis/api/componentreport/crtest/types_test.go +++ b/pkg/apis/api/componentreport/crtest/types_test.go @@ -108,6 +108,64 @@ func TestVariantRoundTrip(t *testing.T) { assert.Equal(t, value, gotValue) } +func TestCompareCellStatus(t *testing.T) { + tests := []struct { + name string + first Status + second Status + want int + }{ + { + name: "equal improvements have equal precedence", + first: SignificantImprovement, + second: SignificantImprovement, + want: 0, + }, + { + name: "ordinary lower status wins", + first: SignificantRegression, + second: NotSignificant, + want: -1, + }, + { + name: "arbitrary negative status follows raw ordering", + first: Status(-9999), + second: FailedFixedRegression, + want: -1, + }, + { + name: "arbitrary positive status follows raw ordering", + first: Status(50), + second: MissingBasis, + want: -1, + }, + { + name: "improvement does not override a negative status", + first: SignificantImprovement, + second: Status(-1), + want: 1, + }, + { + name: "improvement wins at NotSignificant boundary", + first: SignificantImprovement, + second: NotSignificant, + want: -1, + }, + { + name: "improvement wins as second status above boundary", + first: Status(1), + second: SignificantImprovement, + want: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, CompareCellStatus(tt.first, tt.second)) + }) + } +} + func TestColumnEncodeEmpty(t *testing.T) { col := ColumnIdentification{Variants: map[string]string{}} encoded := col.Encode()