From 36f26ac519be8bdf21a39926406f495c05ff29b8 Mon Sep 17 00:00:00 2001 From: xueqzhan Date: Mon, 26 Jan 2026 20:34:39 -0500 Subject: [PATCH 1/2] Filter CR query by key tests --- cmd/sippy/component_readiness.go | 4 + cmd/sippy/serve.go | 1 + .../releasefallback/releasefallback.go | 4 +- .../query/querygenerators.go | 68 +++-- .../query/querygenerators_test.go | 288 ++++++++++++++++++ .../queryparamparser_test.go | 2 +- .../utils/queryparamparser.go | 4 + pkg/apis/api/componentreport/reqopts/types.go | 19 +- pkg/flags/component_readiness.go | 15 + pkg/sippyserver/server.go | 9 +- 10 files changed, 382 insertions(+), 32 deletions(-) create mode 100644 pkg/api/componentreadiness/query/querygenerators_test.go diff --git a/cmd/sippy/component_readiness.go b/cmd/sippy/component_readiness.go index 4678c74bf..6b105c79d 100644 --- a/cmd/sippy/component_readiness.go +++ b/cmd/sippy/component_readiness.go @@ -186,6 +186,9 @@ func (f *ComponentReadinessFlags) runServerMode() error { log.WithError(err).Warn("unable to initialize Jira client, bug filing will be disabled") } + // Get exclusive test names for mass failure filtering + exclusiveTestNames := f.ComponentReadinessFlags.GetMassFailureTestNames() + server := sippyserver.NewServer( sippyserver.ModeOpenShift, f.APIFlags.ListenAddr, @@ -206,6 +209,7 @@ func (f *ComponentReadinessFlags) runServerMode() error { f.APIFlags.EnableWriteEndpoints, "", // No chat API in Component Readiness jiraClient, + exclusiveTestNames, ) if f.APIFlags.MetricsAddr != "" { diff --git a/cmd/sippy/serve.go b/cmd/sippy/serve.go index da6df4a08..568471a0b 100644 --- a/cmd/sippy/serve.go +++ b/cmd/sippy/serve.go @@ -175,6 +175,7 @@ func NewServeCommand() *cobra.Command { f.APIFlags.EnableWriteEndpoints, f.APIFlags.ChatAPIURL, jiraClient, + f.ComponentReadinessFlags.GetMassFailureTestNames(), ) if f.APIFlags.MetricsAddr != "" { diff --git a/pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go b/pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go index 8dfcfad5b..eebafdd37 100644 --- a/pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go +++ b/pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go @@ -541,7 +541,7 @@ func (f *fallbackTestQueryGenerator) getCacheKey() fallbackTestQueryGeneratorCac func (f *fallbackTestQueryGenerator) getTestFallbackRelease(ctx context.Context) (bq.ReportTestStatus, []error) { commonQuery, groupByQuery, queryParameters := query.BuildComponentReportQuery( f.client, f.ReqOptions, f.allVariants, f.ReqOptions.VariantOption.IncludeVariants, - query.DefaultJunitTable, false) + query.DefaultJunitTable, false, f.ReqOptions.AdvancedOption.ExclusiveTestNames...) before := time.Now() log.Infof("Starting Fallback (%s) QueryTestStatus", f.BaseRelease) errs := []error{} @@ -564,7 +564,7 @@ func (f *fallbackTestQueryGenerator) getTestFallbackRelease(ctx context.Context) }, }...) - baseStatus, baseErrs := query.FetchTestStatusResults(ctx, baseQuery) + baseStatus, baseErrs := query.FetchTestStatusResults(ctx, baseQuery, f.ReqOptions.AdvancedOption.ExclusiveTestNames) if len(baseErrs) != 0 { errs = append(errs, baseErrs...) diff --git a/pkg/api/componentreadiness/query/querygenerators.go b/pkg/api/componentreadiness/query/querygenerators.go index 6e65aaa6e..aa328a63f 100644 --- a/pkg/api/componentreadiness/query/querygenerators.go +++ b/pkg/api/componentreadiness/query/querygenerators.go @@ -135,9 +135,8 @@ func NewBaseQueryGenerator( func (b *baseQueryGenerator) QueryTestStatus(ctx context.Context) (bq.ReportTestStatus, []error) { - commonQuery, groupByQuery, queryParameters := BuildComponentReportQuery(b.client, b.ReqOptions, b.allVariants, b.ReqOptions.VariantOption.IncludeVariants, DefaultJunitTable, false) + commonQuery, groupByQuery, queryParameters := BuildComponentReportQuery(b.client, b.ReqOptions, b.allVariants, b.ReqOptions.VariantOption.IncludeVariants, DefaultJunitTable, false, b.ReqOptions.AdvancedOption.ExclusiveTestNames...) - before := time.Now() errs := []error{} baseString := commonQuery + ` AND jv_Release.variant_value = @BaseRelease` baseQuery := b.client.Query(ctx, bqlabel.CRJunitBase, baseString+groupByQuery) @@ -158,14 +157,12 @@ func (b *baseQueryGenerator) QueryTestStatus(ctx context.Context) (bq.ReportTest }, }...) - baseStatus, baseErrs := FetchTestStatusResults(ctx, baseQuery) + baseStatus, baseErrs := FetchTestStatusResults(ctx, baseQuery, b.ReqOptions.AdvancedOption.ExclusiveTestNames) if len(baseErrs) != 0 { errs = append(errs, baseErrs...) } - log.Infof("Base QueryTestStatus completed in %s with %d base results from db", time.Since(before), len(baseStatus)) - return bq.ReportTestStatus{BaseStatus: baseStatus}, errs } @@ -207,9 +204,8 @@ func NewSampleQueryGenerator( } func (s *sampleQueryGenerator) QueryTestStatus(ctx context.Context) (bq.ReportTestStatus, []error) { - commonQuery, groupByQuery, queryParameters := BuildComponentReportQuery(s.client, s.ReqOptions, s.allVariants, s.IncludeVariants, s.JunitTable, true) + commonQuery, groupByQuery, queryParameters := BuildComponentReportQuery(s.client, s.ReqOptions, s.allVariants, s.IncludeVariants, s.JunitTable, true, s.ReqOptions.AdvancedOption.ExclusiveTestNames...) - before := time.Now() errs := []error{} sampleString := commonQuery // Only set sample release when PR and payload options are not set @@ -267,18 +263,17 @@ func (s *sampleQueryGenerator) QueryTestStatus(ctx context.Context) (bq.ReportTe }...) } - sampleStatus, sampleErrs := FetchTestStatusResults(ctx, sampleQuery) + sampleStatus, sampleErrs := FetchTestStatusResults(ctx, sampleQuery, s.ReqOptions.AdvancedOption.ExclusiveTestNames) if len(sampleErrs) != 0 { errs = append(errs, sampleErrs...) } - log.Infof("Sample QueryTestStatus completed in %s with %d sample results db", time.Since(before), len(sampleStatus)) - return bq.ReportTestStatus{SampleStatus: sampleStatus}, errs } // BuildComponentReportQuery returns the common query for the higher level summary component summary. +// If exclusiveTestNames is provided, any job containing tests from this set will have all other tests excluded. func BuildComponentReportQuery( client *bqcachedclient.Client, reqOptions reqopts.RequestOptions, @@ -286,6 +281,7 @@ func BuildComponentReportQuery( includeVariants map[string][]string, junitTable string, isSample bool, + exclusiveTestNames ...string, ) (string, string, []bigquery.QueryParameter) { // Parts of the query, including the columns returned, are dynamic, based on the list of variants we're told to work with. // Variants will be returned as columns with names like: variant_[VariantName] @@ -313,12 +309,35 @@ func BuildComponentReportQuery( // TODO: last_failure here explicitly uses success_val not adjusted_success_val, this ensures we // show the last time the test failed, not flaked. if you enable the flakes as failures feature (which is // non default today), the last failure time will be wrong which can impact things like failed fix detection. - queryString := fmt.Sprintf(`WITH latest_component_mapping AS ( - SELECT * - FROM %s.component_mapping cm - WHERE created_at = ( - SELECT MAX(created_at) - FROM %s.component_mapping)) + // Build the WITH clause - add jobs_with_failed_exclusive_tests CTE if exclusiveTestNames is provided + withClause := "" + if len(exclusiveTestNames) > 0 { + withClause = fmt.Sprintf(`WITH jobs_with_failed_exclusive_tests AS ( + SELECT DISTINCT prowjob_build_id + FROM %s.%s AS junit + WHERE modified_time >= DATETIME(@From) + AND modified_time < DATETIME(@To) + AND test_name IN UNNEST(@ExclusiveTestNames) + AND success_val = 0 + ), + latest_component_mapping AS ( + SELECT * + FROM %s.component_mapping cm + WHERE created_at = ( + SELECT MAX(created_at) + FROM %s.component_mapping))`, + client.Dataset, junitTable, client.Dataset, client.Dataset) + } else { + withClause = fmt.Sprintf(`WITH latest_component_mapping AS ( + SELECT * + FROM %s.component_mapping cm + WHERE created_at = ( + SELECT MAX(created_at) + FROM %s.component_mapping))`, + client.Dataset, client.Dataset) + } + + queryString := fmt.Sprintf(`%s SELECT ANY_VALUE(test_name HAVING MAX prowjob_start) AS test_name, ANY_VALUE(testsuite HAVING MAX prowjob_start) AS test_suite, @@ -333,13 +352,26 @@ func BuildComponentReportQuery( FROM (%s) INNER JOIN latest_component_mapping cm ON testsuite = cm.suite AND test_name = cm.name `, - client.Dataset, client.Dataset, selectVariants, fmt.Sprintf(dedupedJunitTable, jobNameQueryPortion, client.Dataset, junitTable, client.Dataset, client.Dataset, jobRunAnnotationToIgnore)) + withClause, selectVariants, fmt.Sprintf(dedupedJunitTable, jobNameQueryPortion, client.Dataset, junitTable, client.Dataset, client.Dataset, jobRunAnnotationToIgnore)) queryString += joinVariants queryString += `WHERE cm.staff_approved_obsolete = false AND (variant_registry_job_name LIKE 'periodic-%%' OR variant_registry_job_name LIKE 'release-%%' OR variant_registry_job_name LIKE 'aggregator-%%')` commonParams := []bigquery.QueryParameter{} + + // Add filtering logic for exclusive tests + if len(exclusiveTestNames) > 0 { + queryString += ` + AND ( + test_name IN UNNEST(@ExclusiveTestNames) + OR prowjob_build_id NOT IN (SELECT prowjob_build_id FROM jobs_with_failed_exclusive_tests) + )` + commonParams = append(commonParams, bigquery.QueryParameter{ + Name: "ExclusiveTestNames", + Value: exclusiveTestNames, + }) + } if reqOptions.AdvancedOption.IgnoreDisruption { queryString += ` AND NOT 'Disruption' in UNNEST(capabilities)` } @@ -592,7 +624,7 @@ func filterByCrossCompareVariants(crossCompare []string, variantGroups map[strin return } -func FetchTestStatusResults(ctx context.Context, query *bigquery.Query) (map[string]bq.TestStatus, []error) { +func FetchTestStatusResults(ctx context.Context, query *bigquery.Query, exclusiveTestNames []string) (map[string]bq.TestStatus, []error) { errs := []error{} status := map[string]bq.TestStatus{} diff --git a/pkg/api/componentreadiness/query/querygenerators_test.go b/pkg/api/componentreadiness/query/querygenerators_test.go new file mode 100644 index 000000000..7b86d1f86 --- /dev/null +++ b/pkg/api/componentreadiness/query/querygenerators_test.go @@ -0,0 +1,288 @@ +package query + +import ( + "strings" + "testing" + + "github.com/openshift/sippy/pkg/apis/api/componentreport/crtest" + "github.com/openshift/sippy/pkg/apis/api/componentreport/reqopts" + bqcachedclient "github.com/openshift/sippy/pkg/bigquery" + "github.com/openshift/sippy/pkg/util/sets" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestBuildComponentReportQuery_ExclusiveTestFiltering(t *testing.T) { + mockClient := &bqcachedclient.Client{ + Dataset: "test_dataset", + } + + allJobVariants := crtest.JobVariants{ + Variants: map[string][]string{ + "Platform": {"aws", "gcp"}, + "Network": {"sdn", "ovn"}, + }, + } + + baseReqOptions := reqopts.RequestOptions{ + VariantOption: reqopts.Variants{ + ColumnGroupBy: sets.NewString("Platform"), + DBGroupBy: sets.NewString("Network"), + IncludeVariants: map[string][]string{}, + }, + AdvancedOption: reqopts.Advanced{ + IgnoreDisruption: true, + }, + } + + includeVariants := map[string][]string{} + + tests := []struct { + name string + exclusiveTestNames []string + expectedCTE bool + expectedFilter bool + expectedParam bool + expectedCTEContent string + }{ + { + name: "No exclusive tests - no filtering", + exclusiveTestNames: nil, + expectedCTE: false, + expectedFilter: false, + expectedParam: false, + }, + { + name: "Empty exclusive tests - no filtering", + exclusiveTestNames: []string{}, + expectedCTE: false, + expectedFilter: false, + expectedParam: false, + }, + { + name: "With exclusive tests - filtering applied", + exclusiveTestNames: []string{ + "[sig-cluster-lifecycle] Cluster completes upgrade", + "install should succeed: overall", + }, + expectedCTE: true, + expectedFilter: true, + expectedParam: true, + expectedCTEContent: "jobs_with_failed_exclusive_tests", + }, + { + name: "Single exclusive test - filtering applied", + exclusiveTestNames: []string{ + "install should succeed: overall", + }, + expectedCTE: true, + expectedFilter: true, + expectedParam: true, + expectedCTEContent: "jobs_with_failed_exclusive_tests", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reqOptions := baseReqOptions + reqOptions.AdvancedOption.ExclusiveTestNames = tt.exclusiveTestNames + + commonQuery, _, queryParams := BuildComponentReportQuery( + mockClient, + reqOptions, + allJobVariants, + includeVariants, + DefaultJunitTable, + false, + tt.exclusiveTestNames..., + ) + + // Check if CTE is present when expected + if tt.expectedCTE { + assert.Contains(t, commonQuery, "jobs_with_failed_exclusive_tests", + "Query should contain jobs_with_failed_exclusive_tests CTE") + assert.Contains(t, commonQuery, "AND success_val = 0", + "CTE should only identify jobs where exclusive tests FAILED (success_val = 0)") + assert.Contains(t, commonQuery, "test_name IN UNNEST(@ExclusiveTestNames)", + "CTE should filter by exclusive test names") + } else { + assert.NotContains(t, commonQuery, "jobs_with_failed_exclusive_tests", + "Query should not contain jobs_with_failed_exclusive_tests CTE when no exclusive tests") + } + + // Check if filtering WHERE clause is present when expected + if tt.expectedFilter { + assert.Contains(t, commonQuery, "test_name IN UNNEST(@ExclusiveTestNames)", + "Query should include exclusive test names filter") + assert.Contains(t, commonQuery, "prowjob_build_id NOT IN (SELECT prowjob_build_id FROM jobs_with_failed_exclusive_tests)", + "Query should exclude tests from jobs with failed exclusive tests") + } + + // Check if query parameter is present when expected + if tt.expectedParam { + foundParam := false + for _, param := range queryParams { + if param.Name == "ExclusiveTestNames" { + foundParam = true + assert.Equal(t, tt.exclusiveTestNames, param.Value, + "ExclusiveTestNames parameter should match input") + break + } + } + assert.True(t, foundParam, "ExclusiveTestNames parameter should be present in query parameters") + } else { + for _, param := range queryParams { + assert.NotEqual(t, "ExclusiveTestNames", param.Name, + "ExclusiveTestNames parameter should not be present when no exclusive tests") + } + } + + // Verify CTE content structure if CTE is expected + if tt.expectedCTEContent != "" { + // Extract the CTE section + parts := strings.Split(commonQuery, "latest_component_mapping") + require.Greater(t, len(parts), 1, "Query should contain latest_component_mapping CTE") + + cteSection := parts[0] + assert.Contains(t, cteSection, tt.expectedCTEContent, + "Query should contain expected CTE") + + // Verify the CTE only selects prowjob_build_id for jobs where exclusive tests failed + // Use a more lenient check that ignores whitespace variations + normalizedCTE := strings.ReplaceAll(strings.ReplaceAll(cteSection, "\t", " "), "\n", " ") + assert.Contains(t, normalizedCTE, "DISTINCT prowjob_build_id", + "CTE should select distinct prowjob_build_id") + } + }) + } +} + +func TestBuildComponentReportQuery_ExclusiveTestLogic(t *testing.T) { + // This test verifies the specific logic: we only exclude OTHER tests from jobs + // where exclusive tests FAILED (not just present) + mockClient := &bqcachedclient.Client{ + Dataset: "test_dataset", + } + + allJobVariants := crtest.JobVariants{ + Variants: map[string][]string{ + "Platform": {"aws"}, + }, + } + + reqOptions := reqopts.RequestOptions{ + VariantOption: reqopts.Variants{ + ColumnGroupBy: sets.NewString("Platform"), + DBGroupBy: sets.NewString(), + IncludeVariants: map[string][]string{}, + }, + AdvancedOption: reqopts.Advanced{ + ExclusiveTestNames: []string{"install should succeed: overall"}, + }, + } + + commonQuery, _, _ := BuildComponentReportQuery( + mockClient, + reqOptions, + allJobVariants, + map[string][]string{}, + DefaultJunitTable, + false, + "install should succeed: overall", + ) + + // The query should: + // 1. Create a CTE that identifies jobs where exclusive tests FAILED + assert.Contains(t, commonQuery, "WITH jobs_with_failed_exclusive_tests AS", + "Should create CTE for failed exclusive tests") + + // 2. The CTE should check success_val = 0 (failure) + cteEnd := strings.Index(commonQuery, "latest_component_mapping") + require.Greater(t, cteEnd, 0, "Should contain latest_component_mapping CTE") + + cteSection := commonQuery[:cteEnd] + assert.Contains(t, cteSection, "success_val = 0", + "CTE should only match FAILED exclusive tests (success_val = 0), not all instances") + + // 3. Include the exclusive test itself OR tests from jobs without failed exclusive tests + assert.Contains(t, commonQuery, "test_name IN UNNEST(@ExclusiveTestNames)", + "Should always include the exclusive tests themselves") + assert.Contains(t, commonQuery, "prowjob_build_id NOT IN (SELECT prowjob_build_id FROM jobs_with_failed_exclusive_tests)", + "Should exclude other tests from jobs where exclusive tests failed") + + // 4. Verify the logic is an OR condition (include exclusive tests OR tests from clean jobs) + // Normalize whitespace for easier parsing + normalizedQuery := strings.ReplaceAll(strings.ReplaceAll(commonQuery, "\t", " "), "\n", " ") + normalizedQuery = strings.Join(strings.Fields(normalizedQuery), " ") // Collapse all whitespace + + // The query should contain the OR logic between the two conditions + assert.Contains(t, normalizedQuery, "test_name IN UNNEST(@ExclusiveTestNames) OR prowjob_build_id NOT IN", + "Should have OR condition between exclusive test filter and job filter") +} + +func TestBuildComponentReportQuery_WithAndWithoutExclusiveTests(t *testing.T) { + // This test compares queries with and without exclusive tests to ensure + // the base query structure is the same, only the filtering differs + mockClient := &bqcachedclient.Client{ + Dataset: "test_dataset", + } + + allJobVariants := crtest.JobVariants{ + Variants: map[string][]string{ + "Platform": {"aws"}, + }, + } + + baseReqOptions := reqopts.RequestOptions{ + VariantOption: reqopts.Variants{ + ColumnGroupBy: sets.NewString("Platform"), + DBGroupBy: sets.NewString(), + IncludeVariants: map[string][]string{}, + }, + AdvancedOption: reqopts.Advanced{}, + } + + // Query without exclusive tests + queryWithout, _, paramsWithout := BuildComponentReportQuery( + mockClient, + baseReqOptions, + allJobVariants, + map[string][]string{}, + DefaultJunitTable, + false, + ) + + // Query with exclusive tests + reqOptionsWithExclusive := baseReqOptions + reqOptionsWithExclusive.AdvancedOption.ExclusiveTestNames = []string{"install should succeed: overall"} + + queryWith, _, paramsWith := BuildComponentReportQuery( + mockClient, + reqOptionsWithExclusive, + allJobVariants, + map[string][]string{}, + DefaultJunitTable, + false, + "install should succeed: overall", + ) + + // Both should have the component_mapping CTE + assert.Contains(t, queryWithout, "latest_component_mapping", + "Query without exclusive tests should have component mapping CTE") + assert.Contains(t, queryWith, "latest_component_mapping", + "Query with exclusive tests should have component mapping CTE") + + // Only the query with exclusive tests should have the filtering CTE + assert.NotContains(t, queryWithout, "jobs_with_failed_exclusive_tests", + "Query without exclusive tests should not have filtering CTE") + assert.Contains(t, queryWith, "jobs_with_failed_exclusive_tests", + "Query with exclusive tests should have filtering CTE") + + // Check parameters + assert.Len(t, paramsWithout, 0, "Query without exclusive tests should have no extra parameters") + assert.Len(t, paramsWith, 1, "Query with exclusive tests should have 1 parameter") + if len(paramsWith) > 0 { + assert.Equal(t, "ExclusiveTestNames", paramsWith[0].Name, + "Parameter should be named ExclusiveTestNames") + } +} diff --git a/pkg/api/componentreadiness/queryparamparser_test.go b/pkg/api/componentreadiness/queryparamparser_test.go index df8db4720..515c3d4ca 100644 --- a/pkg/api/componentreadiness/queryparamparser_test.go +++ b/pkg/api/componentreadiness/queryparamparser_test.go @@ -402,7 +402,7 @@ func TestParseComponentReportRequest(t *testing.T) { // path/body are irrelevant at this point in time, we only parse query params in the func being tested req, err := http.NewRequest("GET", "https://example.com/path?"+params.Encode(), nil) require.NoError(t, err) - options, _, err := utils.ParseComponentReportRequest(views, releases, req, allJobVariants, time.Duration(0), []v2.VariantJunitTableOverride{}) + options, _, err := utils.ParseComponentReportRequest(views, releases, req, allJobVariants, time.Duration(0), []v2.VariantJunitTableOverride{}, nil) if tc.errMessage != "" { require.Error(t, err) diff --git a/pkg/api/componentreadiness/utils/queryparamparser.go b/pkg/api/componentreadiness/utils/queryparamparser.go index f86604f8e..a58253dcc 100644 --- a/pkg/api/componentreadiness/utils/queryparamparser.go +++ b/pkg/api/componentreadiness/utils/queryparamparser.go @@ -25,6 +25,7 @@ func ParseComponentReportRequest( allJobVariants crtest.JobVariants, crTimeRoundingFactor time.Duration, overrides []configv1.VariantJunitTableOverride, + exclusiveTestNames []string, ) ( opts reqopts.RequestOptions, warnings []string, @@ -141,6 +142,9 @@ func ParseComponentReportRequest( } opts.CacheOption.CRTimeRoundingFactor = crTimeRoundingFactor + // Set exclusive test names from the command-line flag + opts.AdvancedOption.ExclusiveTestNames = exclusiveTestNames + return } diff --git a/pkg/apis/api/componentreport/reqopts/types.go b/pkg/apis/api/componentreport/reqopts/types.go index 04567d180..78f48bdb8 100644 --- a/pkg/apis/api/componentreport/reqopts/types.go +++ b/pkg/apis/api/componentreport/reqopts/types.go @@ -98,13 +98,14 @@ type Variants struct { } type Advanced struct { - MinimumFailure int `json:"minimum_failure" yaml:"minimum_failure"` - Confidence int `json:"confidence" yaml:"confidence"` - PityFactor int `json:"pity_factor" yaml:"pity_factor"` - PassRateRequiredNewTests int `json:"pass_rate_required_new_tests" yaml:"pass_rate_required_new_tests"` - PassRateRequiredAllTests int `json:"pass_rate_required_all_tests" yaml:"pass_rate_required_all_tests"` - IgnoreMissing bool `json:"ignore_missing" yaml:"ignore_missing"` - IgnoreDisruption bool `json:"ignore_disruption" yaml:"ignore_disruption"` - FlakeAsFailure bool `json:"flake_as_failure" yaml:"flake_as_failure"` - IncludeMultiReleaseAnalysis bool `json:"include_multi_release_analysis" yaml:"include_multi_release_analysis"` + MinimumFailure int `json:"minimum_failure" yaml:"minimum_failure"` + Confidence int `json:"confidence" yaml:"confidence"` + PityFactor int `json:"pity_factor" yaml:"pity_factor"` + PassRateRequiredNewTests int `json:"pass_rate_required_new_tests" yaml:"pass_rate_required_new_tests"` + PassRateRequiredAllTests int `json:"pass_rate_required_all_tests" yaml:"pass_rate_required_all_tests"` + IgnoreMissing bool `json:"ignore_missing" yaml:"ignore_missing"` + IgnoreDisruption bool `json:"ignore_disruption" yaml:"ignore_disruption"` + FlakeAsFailure bool `json:"flake_as_failure" yaml:"flake_as_failure"` + IncludeMultiReleaseAnalysis bool `json:"include_multi_release_analysis" yaml:"include_multi_release_analysis"` + ExclusiveTestNames []string `json:"exclusive_test_names,omitempty" yaml:"exclusive_test_names,omitempty"` } diff --git a/pkg/flags/component_readiness.go b/pkg/flags/component_readiness.go index 62a26808c..adc3ef9a3 100644 --- a/pkg/flags/component_readiness.go +++ b/pkg/flags/component_readiness.go @@ -21,6 +21,7 @@ type ComponentReadinessFlags struct { ComponentReadinessViewsFile string CRTimeRoundingFactor time.Duration CORSAllowedOrigin string + ExcludeMassFailures bool } func NewComponentReadinessFlags() *ComponentReadinessFlags { @@ -32,6 +33,7 @@ func (f *ComponentReadinessFlags) BindFlags(fs *pflag.FlagSet) { fs.StringVar(&f.ComponentReadinessViewsFile, "views", "", "Optional yaml file for predefined Component Readiness views") fs.DurationVar(&f.CRTimeRoundingFactor, "component-readiness-time-rounding-factor", defaultCRTimeRoundingFactor, factorUsage) fs.StringVar(&f.CORSAllowedOrigin, "cors-allowed-origin", "*", "Optional allowed origin for CORS") + fs.BoolVar(&f.ExcludeMassFailures, "exclude-mass-failures", false, "Exclude other tests from jobs containing tests known to cause mass failures") } func (f *ComponentReadinessFlags) ParseViewsFile() (*api.SippyViews, error) { @@ -56,6 +58,19 @@ func (f *ComponentReadinessFlags) ParseViewsFile() (*api.SippyViews, error) { return vf, nil } +// GetMassFailureTestNames returns the hard-coded list of test names that are known to cause mass failures. +// When these tests appear in a job, all other tests in that job should be excluded from analysis. +func (f *ComponentReadinessFlags) GetMassFailureTestNames() []string { + if !f.ExcludeMassFailures { + return nil + } + return []string{ + "[sig-cluster-lifecycle] Cluster completes upgrade", + "install should succeed: overall", + "[Jira:\"Test Framework\"] there should not be mass test failures", + } +} + func (f *ComponentReadinessFlags) validateViews(views *api.SippyViews) error { for _, view := range views.ComponentReadiness { // If using variant cross compare, those variants must not appear in the dbGroupBy: diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index 93bde9cb1..2ec84ade5 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -86,6 +86,7 @@ func NewServer( enableWriteEndpoints bool, chatAPIURL string, jiraClient *jira.Client, + exclusiveTestNames []string, ) *Server { server := &Server{ @@ -109,6 +110,7 @@ func NewServer( enableWriteAPIs: enableWriteEndpoints, chatAPIURL: chatAPIURL, jiraClient: jiraClient, + exclusiveTestNames: exclusiveTestNames, } if bigQueryClient != nil { @@ -164,6 +166,7 @@ type Server struct { chatAPIURL string jiraClient *jira.Client rateLimiters map[string]*rateLimiter + exclusiveTestNames []string } type rateLimiter struct { @@ -992,7 +995,8 @@ func (s *Server) getComponentReportFromRequest(req *http.Request) (componentrepo } options, warnings, err := utils.ParseComponentReportRequest(s.views.ComponentReadiness, allReleases, req, allJobVariants, s.crTimeRoundingFactor, - s.config.ComponentReadinessConfig.VariantJunitTableOverrides) + s.config.ComponentReadinessConfig.VariantJunitTableOverrides, s.exclusiveTestNames) + if err != nil { return componentreport.ComponentReport{}, err } @@ -1047,7 +1051,8 @@ func (s *Server) jsonComponentReportTestDetailsFromBigQuery(w http.ResponseWrite } reqOptions, _, err := utils.ParseComponentReportRequest(s.views.ComponentReadiness, allReleases, req, allJobVariants, s.crTimeRoundingFactor, - s.config.ComponentReadinessConfig.VariantJunitTableOverrides) + s.config.ComponentReadinessConfig.VariantJunitTableOverrides, s.exclusiveTestNames) + if err != nil { failureResponse(w, http.StatusBadRequest, err.Error()) return From 7c39f9927568938ea9df013ca73acfa7e4a2cc46 Mon Sep 17 00:00:00 2001 From: xueqzhan Date: Thu, 19 Feb 2026 13:22:24 -0500 Subject: [PATCH 2/2] Prioritize key tests so that only one will be counted for regression calculation --- .../query/querygenerators.go | 76 +++++++++++++---- .../query/querygenerators_test.go | 82 ++++++++++++------- pkg/flags/component_readiness.go | 2 +- 3 files changed, 113 insertions(+), 47 deletions(-) diff --git a/pkg/api/componentreadiness/query/querygenerators.go b/pkg/api/componentreadiness/query/querygenerators.go index aa328a63f..7c8087405 100644 --- a/pkg/api/componentreadiness/query/querygenerators.go +++ b/pkg/api/componentreadiness/query/querygenerators.go @@ -47,6 +47,7 @@ const ( // So, this sorts the data, partitioning by the 3-tuple of file_path/test_name/testsuite - // preferring flakes, then successes, then failures, and we get the first row of each // partition. + // partition. dedupedJunitTable = ` WITH deduped_testcases AS ( SELECT @@ -272,8 +273,24 @@ func (s *sampleQueryGenerator) QueryTestStatus(ctx context.Context) (bq.ReportTe return bq.ReportTestStatus{SampleStatus: sampleStatus}, errs } +// buildPriorityCaseStatement generates a SQL CASE statement that assigns priority based on test position in the list. +// Lower index = higher priority. This is used to ensure when multiple exclusive tests appear in the same job, +// only the highest priority one is counted. +func buildPriorityCaseStatement(exclusiveTestNames []string) string { + var caseStatements []string + for i, testName := range exclusiveTestNames { + // Escape single quotes in test name for SQL + escapedTestName := strings.ReplaceAll(testName, "'", "''") + caseStatements = append(caseStatements, fmt.Sprintf("WHEN test_name = '%s' THEN %d", escapedTestName, i)) + } + // Add a default case to handle any unexpected tests (should not happen due to IN UNNEST filter) + caseStatements = append(caseStatements, fmt.Sprintf("ELSE %d", len(exclusiveTestNames))) + return strings.Join(caseStatements, "\n\t\t\t\t\t\t\t") +} + // BuildComponentReportQuery returns the common query for the higher level summary component summary. -// If exclusiveTestNames is provided, any job containing tests from this set will have all other tests excluded. +// If exclusiveTestNames is provided, jobs containing tests from this set will have all other tests excluded, +// and only the highest priority (earliest in the list) exclusive test will be included. func BuildComponentReportQuery( client *bqcachedclient.Client, reqOptions reqopts.RequestOptions, @@ -290,7 +307,7 @@ func BuildComponentReportQuery( joinVariants := "" groupByVariants := "" for _, v := range sortedKeys(allJobVariants.Variants) { - joinVariants += fmt.Sprintf("LEFT JOIN %s.job_variants jv_%s ON variant_registry_job_name = jv_%s.job_name AND jv_%s.variant_name = '%s'\n", + joinVariants += fmt.Sprintf("LEFT JOIN %s.job_variants jv_%s ON junit_data.variant_registry_job_name = jv_%s.job_name AND jv_%s.variant_name = '%s'\n", client.Dataset, v, v, v, v) } for _, v := range reqOptions.VariantOption.DBGroupBy.List() { @@ -312,21 +329,40 @@ func BuildComponentReportQuery( // Build the WITH clause - add jobs_with_failed_exclusive_tests CTE if exclusiveTestNames is provided withClause := "" if len(exclusiveTestNames) > 0 { - withClause = fmt.Sprintf(`WITH jobs_with_failed_exclusive_tests AS ( - SELECT DISTINCT prowjob_build_id + // Create a CTE that identifies the highest priority (lowest index) exclusive test in each job + // This ensures when multiple exclusive tests appear in the same job, only the highest priority one is used + withClause = fmt.Sprintf(`WITH exclusive_test_priorities AS ( + SELECT + prowjob_build_id, + test_name, + -- Find the index/priority of each test (lower index = higher priority) + CASE + %s + END AS test_priority FROM %s.%s AS junit WHERE modified_time >= DATETIME(@From) AND modified_time < DATETIME(@To) AND test_name IN UNNEST(@ExclusiveTestNames) AND success_val = 0 ), + jobs_with_highest_priority_test AS ( + SELECT + prowjob_build_id, + test_name + FROM exclusive_test_priorities + WHERE test_priority = ( + SELECT MIN(test_priority) + FROM exclusive_test_priorities ep2 + WHERE ep2.prowjob_build_id = exclusive_test_priorities.prowjob_build_id + ) + ), latest_component_mapping AS ( SELECT * FROM %s.component_mapping cm WHERE created_at = ( SELECT MAX(created_at) FROM %s.component_mapping))`, - client.Dataset, junitTable, client.Dataset, client.Dataset) + buildPriorityCaseStatement(exclusiveTestNames), client.Dataset, junitTable, client.Dataset, client.Dataset) } else { withClause = fmt.Sprintf(`WITH latest_component_mapping AS ( SELECT * @@ -339,33 +375,40 @@ func BuildComponentReportQuery( queryString := fmt.Sprintf(`%s SELECT - ANY_VALUE(test_name HAVING MAX prowjob_start) AS test_name, - ANY_VALUE(testsuite HAVING MAX prowjob_start) AS test_suite, + ANY_VALUE(junit_data.test_name HAVING MAX junit_data.prowjob_start) AS test_name, + ANY_VALUE(junit_data.testsuite HAVING MAX junit_data.prowjob_start) AS test_suite, cm.id as test_id, %s COUNT(cm.id) AS total_count, - SUM(adjusted_success_val) AS success_count, - SUM(adjusted_flake_count) AS flake_count, - MAX(CASE WHEN success_val = 0 THEN prowjob_start ELSE NULL END) AS last_failure, + SUM(junit_data.adjusted_success_val) AS success_count, + SUM(junit_data.adjusted_flake_count) AS flake_count, + MAX(CASE WHEN junit_data.success_val = 0 THEN junit_data.prowjob_start ELSE NULL END) AS last_failure, ANY_VALUE(cm.component) AS component, ANY_VALUE(cm.capabilities) AS capabilities, - FROM (%s) - INNER JOIN latest_component_mapping cm ON testsuite = cm.suite AND test_name = cm.name + FROM (%s) AS junit_data + INNER JOIN latest_component_mapping cm ON junit_data.testsuite = cm.suite AND junit_data.test_name = cm.name `, withClause, selectVariants, fmt.Sprintf(dedupedJunitTable, jobNameQueryPortion, client.Dataset, junitTable, client.Dataset, client.Dataset, jobRunAnnotationToIgnore)) queryString += joinVariants queryString += `WHERE cm.staff_approved_obsolete = false AND - (variant_registry_job_name LIKE 'periodic-%%' OR variant_registry_job_name LIKE 'release-%%' OR variant_registry_job_name LIKE 'aggregator-%%')` + (junit_data.variant_registry_job_name LIKE 'periodic-%%' OR junit_data.variant_registry_job_name LIKE 'release-%%' OR junit_data.variant_registry_job_name LIKE 'aggregator-%%')` commonParams := []bigquery.QueryParameter{} - // Add filtering logic for exclusive tests + // Add filtering logic for exclusive tests with priority + // Only include the highest priority test from each job, and exclude all other tests from those jobs if len(exclusiveTestNames) > 0 { queryString += ` AND ( - test_name IN UNNEST(@ExclusiveTestNames) - OR prowjob_build_id NOT IN (SELECT prowjob_build_id FROM jobs_with_failed_exclusive_tests) + -- Include tests from jobs that don't have any failed exclusive tests + junit_data.prowjob_build_id NOT IN (SELECT prowjob_build_id FROM jobs_with_highest_priority_test) + -- Or include only the highest priority exclusive test from jobs that have them + OR EXISTS ( + SELECT 1 FROM jobs_with_highest_priority_test j + WHERE j.prowjob_build_id = junit_data.prowjob_build_id + AND j.test_name = junit_data.test_name + ) )` commonParams = append(commonParams, bigquery.QueryParameter{ Name: "ExclusiveTestNames", @@ -659,6 +702,7 @@ func FetchTestStatusResults(ctx context.Context, query *bigquery.Query, exclusiv status[testIDStr] = testStatus } + return status, errs } diff --git a/pkg/api/componentreadiness/query/querygenerators_test.go b/pkg/api/componentreadiness/query/querygenerators_test.go index 7b86d1f86..2c96fd2e2 100644 --- a/pkg/api/componentreadiness/query/querygenerators_test.go +++ b/pkg/api/componentreadiness/query/querygenerators_test.go @@ -68,7 +68,7 @@ func TestBuildComponentReportQuery_ExclusiveTestFiltering(t *testing.T) { expectedCTE: true, expectedFilter: true, expectedParam: true, - expectedCTEContent: "jobs_with_failed_exclusive_tests", + expectedCTEContent: "jobs_with_highest_priority_test", }, { name: "Single exclusive test - filtering applied", @@ -78,7 +78,7 @@ func TestBuildComponentReportQuery_ExclusiveTestFiltering(t *testing.T) { expectedCTE: true, expectedFilter: true, expectedParam: true, - expectedCTEContent: "jobs_with_failed_exclusive_tests", + expectedCTEContent: "jobs_with_highest_priority_test", }, } @@ -99,23 +99,31 @@ func TestBuildComponentReportQuery_ExclusiveTestFiltering(t *testing.T) { // Check if CTE is present when expected if tt.expectedCTE { - assert.Contains(t, commonQuery, "jobs_with_failed_exclusive_tests", - "Query should contain jobs_with_failed_exclusive_tests CTE") + assert.Contains(t, commonQuery, "jobs_with_highest_priority_test", + "Query should contain jobs_with_highest_priority_test CTE") + assert.Contains(t, commonQuery, "exclusive_test_priorities", + "Query should contain exclusive_test_priorities CTE for priority calculation") assert.Contains(t, commonQuery, "AND success_val = 0", "CTE should only identify jobs where exclusive tests FAILED (success_val = 0)") assert.Contains(t, commonQuery, "test_name IN UNNEST(@ExclusiveTestNames)", "CTE should filter by exclusive test names") + assert.Contains(t, commonQuery, "test_priority", + "CTE should calculate test priority based on list order") } else { - assert.NotContains(t, commonQuery, "jobs_with_failed_exclusive_tests", - "Query should not contain jobs_with_failed_exclusive_tests CTE when no exclusive tests") + assert.NotContains(t, commonQuery, "jobs_with_highest_priority_test", + "Query should not contain jobs_with_highest_priority_test CTE when no exclusive tests") + assert.NotContains(t, commonQuery, "exclusive_test_priorities", + "Query should not contain exclusive_test_priorities CTE when no exclusive tests") } // Check if filtering WHERE clause is present when expected if tt.expectedFilter { - assert.Contains(t, commonQuery, "test_name IN UNNEST(@ExclusiveTestNames)", - "Query should include exclusive test names filter") - assert.Contains(t, commonQuery, "prowjob_build_id NOT IN (SELECT prowjob_build_id FROM jobs_with_failed_exclusive_tests)", - "Query should exclude tests from jobs with failed exclusive tests") + assert.Contains(t, commonQuery, "junit_data.prowjob_build_id NOT IN (SELECT prowjob_build_id FROM jobs_with_highest_priority_test)", + "Query should include tests from jobs without failed exclusive tests") + assert.Contains(t, commonQuery, "EXISTS", + "Query should use EXISTS to match highest priority test from jobs with exclusive tests") + assert.Contains(t, commonQuery, "j.prowjob_build_id = junit_data.prowjob_build_id", + "Query should match both prowjob_build_id and test_name in EXISTS clause") } // Check if query parameter is present when expected @@ -147,11 +155,13 @@ func TestBuildComponentReportQuery_ExclusiveTestFiltering(t *testing.T) { assert.Contains(t, cteSection, tt.expectedCTEContent, "Query should contain expected CTE") - // Verify the CTE only selects prowjob_build_id for jobs where exclusive tests failed - // Use a more lenient check that ignores whitespace variations + // Verify the CTE selects prowjob_build_id and test_name for priority-based filtering + // The new structure identifies the highest priority test per job normalizedCTE := strings.ReplaceAll(strings.ReplaceAll(cteSection, "\t", " "), "\n", " ") - assert.Contains(t, normalizedCTE, "DISTINCT prowjob_build_id", - "CTE should select distinct prowjob_build_id") + assert.Contains(t, normalizedCTE, "prowjob_build_id", + "CTE should select prowjob_build_id") + assert.Contains(t, normalizedCTE, "test_name", + "CTE should select test_name to identify the highest priority test") } }) } @@ -192,9 +202,11 @@ func TestBuildComponentReportQuery_ExclusiveTestLogic(t *testing.T) { ) // The query should: - // 1. Create a CTE that identifies jobs where exclusive tests FAILED - assert.Contains(t, commonQuery, "WITH jobs_with_failed_exclusive_tests AS", - "Should create CTE for failed exclusive tests") + // 1. Create CTEs that identify the highest priority test in each job + assert.Contains(t, commonQuery, "WITH exclusive_test_priorities AS", + "Should create CTE for calculating test priorities") + assert.Contains(t, commonQuery, "jobs_with_highest_priority_test AS", + "Should create CTE for identifying highest priority test per job") // 2. The CTE should check success_val = 0 (failure) cteEnd := strings.Index(commonQuery, "latest_component_mapping") @@ -204,20 +216,26 @@ func TestBuildComponentReportQuery_ExclusiveTestLogic(t *testing.T) { assert.Contains(t, cteSection, "success_val = 0", "CTE should only match FAILED exclusive tests (success_val = 0), not all instances") - // 3. Include the exclusive test itself OR tests from jobs without failed exclusive tests - assert.Contains(t, commonQuery, "test_name IN UNNEST(@ExclusiveTestNames)", - "Should always include the exclusive tests themselves") - assert.Contains(t, commonQuery, "prowjob_build_id NOT IN (SELECT prowjob_build_id FROM jobs_with_failed_exclusive_tests)", - "Should exclude other tests from jobs where exclusive tests failed") - - // 4. Verify the logic is an OR condition (include exclusive tests OR tests from clean jobs) + // 3. Priority-based filtering: only include highest priority test from jobs with exclusive test failures + assert.Contains(t, commonQuery, "test_priority", + "Should calculate test priority based on list order") + assert.Contains(t, commonQuery, "junit_data.prowjob_build_id NOT IN (SELECT prowjob_build_id FROM jobs_with_highest_priority_test)", + "Should include tests from jobs without failed exclusive tests") + assert.Contains(t, commonQuery, "EXISTS", + "Should use EXISTS to match the highest priority test from jobs with exclusive test failures") + assert.Contains(t, commonQuery, "j.prowjob_build_id = junit_data.prowjob_build_id", + "Should match prowjob_build_id in EXISTS clause") + assert.Contains(t, commonQuery, "j.test_name = junit_data.test_name", + "Should match test_name in EXISTS clause") + + // 4. Verify the logic correctly filters based on priority // Normalize whitespace for easier parsing normalizedQuery := strings.ReplaceAll(strings.ReplaceAll(commonQuery, "\t", " "), "\n", " ") normalizedQuery = strings.Join(strings.Fields(normalizedQuery), " ") // Collapse all whitespace - // The query should contain the OR logic between the two conditions - assert.Contains(t, normalizedQuery, "test_name IN UNNEST(@ExclusiveTestNames) OR prowjob_build_id NOT IN", - "Should have OR condition between exclusive test filter and job filter") + // The query should contain the priority-based filtering logic using EXISTS + assert.Contains(t, normalizedQuery, "jobs_with_highest_priority_test j", + "Should use jobs_with_highest_priority_test in EXISTS subquery") } func TestBuildComponentReportQuery_WithAndWithoutExclusiveTests(t *testing.T) { @@ -272,11 +290,15 @@ func TestBuildComponentReportQuery_WithAndWithoutExclusiveTests(t *testing.T) { assert.Contains(t, queryWith, "latest_component_mapping", "Query with exclusive tests should have component mapping CTE") - // Only the query with exclusive tests should have the filtering CTE - assert.NotContains(t, queryWithout, "jobs_with_failed_exclusive_tests", + // Only the query with exclusive tests should have the filtering CTEs + assert.NotContains(t, queryWithout, "jobs_with_highest_priority_test", "Query without exclusive tests should not have filtering CTE") - assert.Contains(t, queryWith, "jobs_with_failed_exclusive_tests", + assert.NotContains(t, queryWithout, "exclusive_test_priorities", + "Query without exclusive tests should not have priority calculation CTE") + assert.Contains(t, queryWith, "jobs_with_highest_priority_test", "Query with exclusive tests should have filtering CTE") + assert.Contains(t, queryWith, "exclusive_test_priorities", + "Query with exclusive tests should have priority calculation CTE") // Check parameters assert.Len(t, paramsWithout, 0, "Query without exclusive tests should have no extra parameters") diff --git a/pkg/flags/component_readiness.go b/pkg/flags/component_readiness.go index adc3ef9a3..5b8b8c85f 100644 --- a/pkg/flags/component_readiness.go +++ b/pkg/flags/component_readiness.go @@ -65,8 +65,8 @@ func (f *ComponentReadinessFlags) GetMassFailureTestNames() []string { return nil } return []string{ - "[sig-cluster-lifecycle] Cluster completes upgrade", "install should succeed: overall", + "[sig-cluster-lifecycle] Cluster completes upgrade", "[Jira:\"Test Framework\"] there should not be mass test failures", } }