Proposed Implementation for Spot Check Jobs#3612
Open
dgoodwin wants to merge 26 commits into
Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
Contributor
|
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)
WalkthroughThis PR introduces spot-check job monitoring for the OpenShift component readiness pipeline. It adds new BigQuery queries, a middleware-driven analysis system, variant classification for rare/spot-check jobs, and frontend UI updates to monitor rarely-run jobs like CPU partitioning and etcd scaling within a configurable time window. ChangesSpot-Check Jobs Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
|
Scheduling required tests: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
sippy-ng/src/component_readiness/CompReadyTestDetailRow.js (1)
1-271: ⚡ Quick winRun ESLint and Prettier to ensure code consistency.
After making changes to frontend files, run the following commands to maintain code quality:
npx eslint . --fix npx prettier --write .🤖 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 `@sippy-ng/src/component_readiness/CompReadyTestDetailRow.js` around lines 1 - 271, Run ESLint and Prettier to fix formatting and lint issues introduced in CompReadyTestDetailRow.js: execute "npx eslint . --fix" and "npx prettier --write ." then review and stage the resulting changes (focus on formatting and lint fixes around the CompReadyTestDetailRow component and helper functions like getJobRunColor, isMassFailure, testJobDetailCell, and getSelectingCheckBox), resolve any remaining lint errors reported (e.g., unused imports or console statements), and commit the auto-fixed files.Source: Coding guidelines
docs/plans/spot-check-jobs.md (1)
80-85: 💤 Low valueAdd language specifier to fenced code block.
The code block at line 80 is missing a language identifier. Specify
textor leave it as plain markdown to silence the linter.📝 Proposed fix
+```text candidate → spotcheck ↑ add pattern to setSpotCheckComponent in ocp.go +```🤖 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 `@docs/plans/spot-check-jobs.md` around lines 80 - 85, The fenced code block in docs/plans/spot-check-jobs.md is missing a language specifier; update the block around the snippet referencing "candidate → spotcheck" and the note "add pattern to setSpotCheckComponent in ocp.go" by adding a language tag (e.g., ```text) on the opening fence so the linter recognizes it as a text block. Ensure the closing triple backticks remain present and that the content mentioning setSpotCheckComponent and ocp.go is unchanged.pkg/api/componentreadiness/dataprovider/bigquery/provider.go (1)
514-542: 💤 Low valueType assertions on raw BigQuery values can panic if types are unexpected.
The type assertions (e.g.,
val.(string),val.(int64)) will panic if BigQuery returns a different type than expected. While nil is checked, type mismatches are not handled. Consider using type assertion with ok check, or use a typed struct likeQuerySpotCheckJobRunDetailsdoes on lines 627-633.🛡️ Example defensive pattern
- if val, ok := rawRow["spot_check_component"]; ok && val != nil { - group.Component = val.(string) + if val, ok := rawRow["spot_check_component"]; ok && val != nil { + if s, ok := val.(string); ok { + group.Component = s + } }🤖 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/api/componentreadiness/dataprovider/bigquery/provider.go` around lines 514 - 542, The current code assigns values from rawRow into a dataprovider.SpotCheckGroup using unchecked type assertions (e.g., val.(string), val.(int64), val.([]bigquery.Value), val.(time.Time)) which can panic on unexpected types; update the parsing in the block that builds group (the variable group, rawRow map access, the loop over columnGroupByVariants.List()) to perform safe type assertions (v, ok := val.(string) / int64 / []bigquery.Value / time.Time) or use type switches and only set fields when assertions succeed, converting numeric types safely to int and iterating elements defensively for JobNames; optionally centralize this into a helper like parseSpotCheckRow(rawRow) to mirror the safer pattern used by QuerySpotCheckJobRunDetails.pkg/api/componentreadiness/utils/queryparamparser.go (1)
59-71: 💤 Low valueConsider extracting spot-check resolution into a shared helper.
The spot-check sample resolution logic (constructing
spotCheckRelativeand callingGetViewReleaseOptions) is duplicated across these two files. Extracting it into a helper function inpkg/api/componentreadiness/utilswould reduce duplication and make future changes easier.♻️ Possible helper function structure
// In pkg/api/componentreadiness/utils/utils.go or similar: // ResolveSpotCheckSample resolves a spot check sample from view configuration // to absolute start/end times using the provided releases and rounding parameters. func ResolveSpotCheckSample( releases []v1.Release, sampleReleaseName string, spotCheckConfig *crview.SpotCheckSample, roundingFactor, roundingOffset time.Duration, ) (*reqopts.Release, error) { if spotCheckConfig == nil { return nil, nil } spotCheckRelative := reqopts.RelativeRelease{ Release: reqopts.Release{Name: sampleReleaseName}, RelativeStart: spotCheckConfig.RelativeStart, RelativeEnd: spotCheckConfig.RelativeEnd, } resolved, err := GetViewReleaseOptions( releases, "spot_check", spotCheckRelative, roundingFactor, roundingOffset) if err != nil { return nil, err } return &resolved, nil }Then both call sites become:
if view.SpotCheckSample != nil { resolved, err := utils.ResolveSpotCheckSample( releases, view.SampleRelease.Name, view.SpotCheckSample, crTimeRoundingFactor, crTimeRoundingOffset) if err != nil { // handle error } opts.SpotCheckSample = resolved }🤖 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/api/componentreadiness/utils/queryparamparser.go` around lines 59 - 71, The spot-check resolution block (constructing spotCheckRelative and calling GetViewReleaseOptions) is duplicated; extract it into a helper like ResolveSpotCheckSample in pkg/api/componentreadiness/utils so both call sites reuse it. Implement ResolveSpotCheckSample to accept releases, sampleReleaseName, view.SpotCheckSample, crTimeRoundingFactor and crTimeRoundingOffset, return either a resolved reqopts.Release (or nil) and error, and replace the inline code that builds spotCheckRelative and calls GetViewReleaseOptions with a call to ResolveSpotCheckSample that assigns opts.SpotCheckSample and propagates any error.pkg/api/componentreadiness/middleware/newtestpassrate/newtestpassrate.go (1)
52-54: ⚡ Quick winAdd comment explaining the MissingBasis status logic.
The condition at lines 52-53 has subtle semantics: when a new test shows
NotSignificantvia pass-rate analysis butPassRateRequiredAllTestsis disabled, the status is changed toMissingBasisto avoid incorrectly reporting "no regression" for a test that lacks historical baseline data. A brief inline comment would clarify this intent for future maintainers.🤖 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/api/componentreadiness/middleware/newtestpassrate/newtestpassrate.go` around lines 52 - 54, Add an inline comment above the condition that mutates testStats.ReportStatus to crtest.MissingBasis to explain the semantics: when a test's pass-rate analysis yields crtest.NotSignificant but historical baseline checking is disabled (opts.PassRateRequiredAllTests == 0), we treat the result as missing basis rather than "no regression" to avoid false negatives; reference the symbols testStats.ReportStatus, crtest.NotSignificant, opts.PassRateRequiredAllTests, and crtest.MissingBasis in the comment so future maintainers understand why the status is changed.pkg/api/componentreadiness/middleware/spotcheckjobs/spotcheckjobs.go (1)
277-292: ⚡ Quick winClarify comment about component case handling.
The comment at lines 286-288 states "we need title case for component" but the code at line 289 returns
componentwithout any case conversion (it's already lowercase fromparts[1]).The code appears correct for matching the BigQuery query's
COALESCEfallback, but the comment is misleading. Either:
- Update the comment to remove "title case for component" since the component is used as-is (lowercase), or
- If title case is actually required, add
component = strings.Title(component)at line 289🤖 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/api/componentreadiness/middleware/spotcheckjobs/spotcheckjobs.go` around lines 277 - 292, The comment in componentCapabilityFromTestID is misleading about component casing — update the function's comment to remove the "we need title case for component" phrase and state that the component is returned as-is (lowercased) while the capability is converted from dash-separated to space-separated for BigQuery matching; if title case is actually required instead, modify the function to convert component (parts[1]) to title case (e.g., via strings.Title or equivalent) and update the comment accordingly.pkg/api/componentreadiness/middleware/fisherexact/fisherexact.go (1)
147-153: 💤 Low valueDocument or make configurable the extreme regression threshold.
The
getRegressionStatusfunction uses a hardcoded0.15(15% pass-rate drop) threshold to distinguish betweenExtremeRegressionandSignificantRegression. This policy decision should either be:
- Documented with an inline comment explaining why 15% is the appropriate threshold, or
- Made configurable via
reqopts.Advancedso it can be tuned per-release or per-view🤖 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/api/componentreadiness/middleware/fisherexact/fisherexact.go` around lines 147 - 153, The hardcoded 0.15 threshold in getRegressionStatus should be configurable and documented: add a constant DefaultExtremeRegressionThreshold = 0.15 with an inline comment, change getRegressionStatus to accept an extremeThreshold float64 (or read reqopts.Advanced.ExtremeRegressionThreshold if that options struct is available), use that threshold instead of 0.15, and update all callers to pass the configured value (falling back to DefaultExtremeRegressionThreshold when reqopts.Advanced is nil or the field is zero); also add a short comment in getRegressionStatus explaining the threshold semantics.
🤖 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/api/componentreadiness/middleware/alltestspassrate/alltestspassrate.go`:
- Around line 41-51: Add unit tests for AllTestsPassRate.Analyze covering the
conditional branch and delegation to analysis.BuildPassRateTestStats: create
tests that (1) set reqOptions.AdvancedOption.PassRateRequiredAllTests == 0 and
assert Analyze returns (false, nil) and does not modify testStats; (2) set
PassRateRequiredAllTests > 0 with testdetails.TestComparison representing a
passing case and assert Analyze returns (true, nil) and that
testStats.Status/Significance reflect NotSignificant; and (3) set
PassRateRequiredAllTests > 0 with a failing case and assert Analyze returns
(true, nil) and that testStats reflects a regression status; use
crtest.Identification zero value for the id param and validate that
analysis.BuildPassRateTestStats effects on testdetails.TestComparison are
applied (you can stub or construct expected testStats values). Ensure tests
reference AllTestsPassRate, Analyze,
reqOptions.AdvancedOption.PassRateRequiredAllTests,
analysis.BuildPassRateTestStats and testdetails.TestComparison.
In `@pkg/api/componentreadiness/middleware/fisherexact/fisherexact.go`:
- Around line 46-137: Add comprehensive unit tests for the FisherExact.Analyze
method to cover all conditional branches and policy decisions: create
table-driven tests that construct testdetails.TestComparison inputs (including
BaseStats, SampleStats, RequiredConfidence, PityAdjustment) and
FisherExact.reqOptions.AdvancedOption settings to exercise missing-data (sample
total 0 with IgnoreMissing true/false), minimum-failure triage (MinimumFailure
threshold), improved performance (sample pass rate > base with fisherExactTest
returning significant), degraded within-pity (drop <= PityFactor →
NotSignificant), degraded beyond-pity (fisherExactTest significant producing
SignificantRegression or ExtremeRegression depending on drop < 15% vs >= 15%),
and confidence threshold behavior (RequiredConfidence overrides option). For
each case assert returned bool/error, TestComparison.ReportStatus, FisherExact
value, and Explanations; mock or control fisherExactTest outputs where needed to
deterministically simulate significance.
In `@pkg/api/componentreadiness/middleware/newtestpassrate/newtestpassrate.go`:
- Around line 41-56: Add unit tests exercising NewTestPassRate.Analyze covering
all branches: (1) when reqOptions.AdvancedOption.PassRateRequiredNewTests == 0
it should return handled=false; (2) when testStats.BaseStats != nil and
BaseStats.Total()>0 it should return handled=false; (3) for a new test
(BaseStats nil) with passing samples assert analysis.BuildPassRateTestStats
yields ReportStatus == crtest.NotSignificant; (4) for a new test with failing
samples assert BuildPassRateTestStats results in a regression status (e.g.,
SignificantRegression/ExtremeRegression); and (5) when ReportStatus remains
crtest.NotSignificant and PassRateRequiredAllTests == 0 assert Analyze sets
ReportStatus to crtest.MissingBasis. Create test fixtures for
testdetails.TestComparison inputs, stub or exercise
analysis.BuildPassRateTestStats as appropriate, and assert the returned bool and
testStats.ReportStatus for each case.
---
Nitpick comments:
In `@docs/plans/spot-check-jobs.md`:
- Around line 80-85: The fenced code block in docs/plans/spot-check-jobs.md is
missing a language specifier; update the block around the snippet referencing
"candidate → spotcheck" and the note "add pattern to setSpotCheckComponent in
ocp.go" by adding a language tag (e.g., ```text) on the opening fence so the
linter recognizes it as a text block. Ensure the closing triple backticks remain
present and that the content mentioning setSpotCheckComponent and ocp.go is
unchanged.
In `@pkg/api/componentreadiness/dataprovider/bigquery/provider.go`:
- Around line 514-542: The current code assigns values from rawRow into a
dataprovider.SpotCheckGroup using unchecked type assertions (e.g., val.(string),
val.(int64), val.([]bigquery.Value), val.(time.Time)) which can panic on
unexpected types; update the parsing in the block that builds group (the
variable group, rawRow map access, the loop over columnGroupByVariants.List())
to perform safe type assertions (v, ok := val.(string) / int64 /
[]bigquery.Value / time.Time) or use type switches and only set fields when
assertions succeed, converting numeric types safely to int and iterating
elements defensively for JobNames; optionally centralize this into a helper like
parseSpotCheckRow(rawRow) to mirror the safer pattern used by
QuerySpotCheckJobRunDetails.
In `@pkg/api/componentreadiness/middleware/fisherexact/fisherexact.go`:
- Around line 147-153: The hardcoded 0.15 threshold in getRegressionStatus
should be configurable and documented: add a constant
DefaultExtremeRegressionThreshold = 0.15 with an inline comment, change
getRegressionStatus to accept an extremeThreshold float64 (or read
reqopts.Advanced.ExtremeRegressionThreshold if that options struct is
available), use that threshold instead of 0.15, and update all callers to pass
the configured value (falling back to DefaultExtremeRegressionThreshold when
reqopts.Advanced is nil or the field is zero); also add a short comment in
getRegressionStatus explaining the threshold semantics.
In `@pkg/api/componentreadiness/middleware/newtestpassrate/newtestpassrate.go`:
- Around line 52-54: Add an inline comment above the condition that mutates
testStats.ReportStatus to crtest.MissingBasis to explain the semantics: when a
test's pass-rate analysis yields crtest.NotSignificant but historical baseline
checking is disabled (opts.PassRateRequiredAllTests == 0), we treat the result
as missing basis rather than "no regression" to avoid false negatives; reference
the symbols testStats.ReportStatus, crtest.NotSignificant,
opts.PassRateRequiredAllTests, and crtest.MissingBasis in the comment so future
maintainers understand why the status is changed.
In `@pkg/api/componentreadiness/middleware/spotcheckjobs/spotcheckjobs.go`:
- Around line 277-292: The comment in componentCapabilityFromTestID is
misleading about component casing — update the function's comment to remove the
"we need title case for component" phrase and state that the component is
returned as-is (lowercased) while the capability is converted from
dash-separated to space-separated for BigQuery matching; if title case is
actually required instead, modify the function to convert component (parts[1])
to title case (e.g., via strings.Title or equivalent) and update the comment
accordingly.
In `@pkg/api/componentreadiness/utils/queryparamparser.go`:
- Around line 59-71: The spot-check resolution block (constructing
spotCheckRelative and calling GetViewReleaseOptions) is duplicated; extract it
into a helper like ResolveSpotCheckSample in pkg/api/componentreadiness/utils so
both call sites reuse it. Implement ResolveSpotCheckSample to accept releases,
sampleReleaseName, view.SpotCheckSample, crTimeRoundingFactor and
crTimeRoundingOffset, return either a resolved reqopts.Release (or nil) and
error, and replace the inline code that builds spotCheckRelative and calls
GetViewReleaseOptions with a call to ResolveSpotCheckSample that assigns
opts.SpotCheckSample and propagates any error.
In `@sippy-ng/src/component_readiness/CompReadyTestDetailRow.js`:
- Around line 1-271: Run ESLint and Prettier to fix formatting and lint issues
introduced in CompReadyTestDetailRow.js: execute "npx eslint . --fix" and "npx
prettier --write ." then review and stage the resulting changes (focus on
formatting and lint fixes around the CompReadyTestDetailRow component and helper
functions like getJobRunColor, isMassFailure, testJobDetailCell, and
getSelectingCheckBox), resolve any remaining lint errors reported (e.g., unused
imports or console statements), and commit the auto-fixed files.
🪄 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: 183bc474-56cf-4f22-9ec0-c05022cdf713
📒 Files selected for processing (33)
config/views.yamldocs/plans/spot-check-jobs.mdpkg/api/componentreadiness/component_report.gopkg/api/componentreadiness/component_report_test.gopkg/api/componentreadiness/dataprovider/bigquery/provider.gopkg/api/componentreadiness/dataprovider/interface.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/middleware/alltestspassrate/alltestspassrate.gopkg/api/componentreadiness/middleware/analysis/passrate.gopkg/api/componentreadiness/middleware/fisherexact/fisherexact.gopkg/api/componentreadiness/middleware/interface.gopkg/api/componentreadiness/middleware/linkinjector/linkinjector.gopkg/api/componentreadiness/middleware/list.gopkg/api/componentreadiness/middleware/newtestpassrate/newtestpassrate.gopkg/api/componentreadiness/middleware/regressionallowances/regressionallowances.gopkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback.gopkg/api/componentreadiness/middleware/spotcheckjobs/spotcheckjobs.gopkg/api/componentreadiness/middleware/spotcheckjobs/spotcheckjobs_test.gopkg/api/componentreadiness/test_details.gopkg/api/componentreadiness/utils/queryparamparser.gopkg/apis/api/componentreport/crtest/types.gopkg/apis/api/componentreport/crview/types.gopkg/apis/api/componentreport/reqopts/types.gopkg/bigquery/bqlabel/labels.gopkg/dataloader/regressioncacheloader/regressioncacheloader.gopkg/variantregistry/ocp.gopkg/variantregistry/ocp_test.gopkg/variantregistry/snapshot.yamlsippy-ng/src/component_readiness/CompReadyTestDetailRow.jssippy-ng/src/component_readiness/CompReadyTestPanel.jssippy-ng/src/component_readiness/CompReadyUtils.jssippy-ng/src/component_readiness/TestDetailsReport.js
Comment on lines
+41
to
+51
| // Analyze claims all tests when PassRateRequiredAllTests is configured, applying a raw | ||
| // pass rate comparison instead of Fisher's exact test. | ||
| func (a *AllTestsPassRate) Analyze(_ crtest.Identification, testStats *testdetails.TestComparison) (bool, error) { | ||
| opts := a.reqOptions.AdvancedOption | ||
| if opts.PassRateRequiredAllTests == 0 { | ||
| return false, nil | ||
| } | ||
|
|
||
| analysis.BuildPassRateTestStats(testStats, float64(opts.PassRateRequiredAllTests), opts) | ||
| return true, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add unit tests for the Analyze method.
The Analyze method, while simpler than newtestpassrate, still contains conditional logic and delegates to analysis.BuildPassRateTestStats. As per coding guidelines, new Go functions and methods should have corresponding unit tests.
Consider adding test cases:
- PassRateRequiredAllTests == 0 (should return false, not handle)
- PassRateRequiredAllTests > 0 with passing test (should return true, set NotSignificant)
- PassRateRequiredAllTests > 0 with failing test (should return true, set regression status)
As per coding guidelines: "New or modified functionality must include test coverage: New Go functions and methods should have corresponding unit tests."
🤖 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/api/componentreadiness/middleware/alltestspassrate/alltestspassrate.go`
around lines 41 - 51, Add unit tests for AllTestsPassRate.Analyze covering the
conditional branch and delegation to analysis.BuildPassRateTestStats: create
tests that (1) set reqOptions.AdvancedOption.PassRateRequiredAllTests == 0 and
assert Analyze returns (false, nil) and does not modify testStats; (2) set
PassRateRequiredAllTests > 0 with testdetails.TestComparison representing a
passing case and assert Analyze returns (true, nil) and that
testStats.Status/Significance reflect NotSignificant; and (3) set
PassRateRequiredAllTests > 0 with a failing case and assert Analyze returns
(true, nil) and that testStats reflects a regression status; use
crtest.Identification zero value for the id param and validate that
analysis.BuildPassRateTestStats effects on testdetails.TestComparison are
applied (you can stub or construct expected testStats values). Ensure tests
reference AllTestsPassRate, Analyze,
reqOptions.AdvancedOption.PassRateRequiredAllTests,
analysis.BuildPassRateTestStats and testdetails.TestComparison.
Source: Coding guidelines
Comment on lines
+46
to
+137
| // Analyze is the catch-all analysis middleware. It always claims the test and performs | ||
| // Fisher's exact test to determine regression significance. | ||
| func (f *FisherExact) Analyze(_ crtest.Identification, testStats *testdetails.TestComparison) (bool, error) { | ||
| logger := log.WithField("middleware", "FisherExact") | ||
| opts := f.reqOptions.AdvancedOption | ||
|
|
||
| if testStats.RequiredConfidence == 0 { | ||
| testStats.RequiredConfidence = opts.Confidence | ||
| } | ||
|
|
||
| fisherExactVal := 0.0 | ||
| testStats.Comparison = crtest.FisherExact | ||
|
|
||
| status := crtest.MissingBasis | ||
| if testStats.SampleStats.Total() == 0 { | ||
| if opts.IgnoreMissing { | ||
| status = crtest.NotSignificant | ||
| } else { | ||
| status = crtest.MissingSample | ||
| } | ||
| testStats.ReportStatus = status | ||
| testStats.FisherExact = thrift.Float64Ptr(0.0) | ||
| testStats.Explanations = append(testStats.Explanations, analysis.ExplanationNoRegression) | ||
| return true, nil | ||
| } else if testStats.BaseStats != nil && testStats.BaseStats.Total() != 0 { | ||
| samplePass := testStats.SampleStats.Passes(opts.FlakeAsFailure) | ||
| basePass := testStats.BaseStats.Passes(opts.FlakeAsFailure) | ||
| basisPassPercentage := float64(basePass) / float64(testStats.BaseStats.Total()) | ||
| effectivePityFactor := float64(opts.PityFactor) + testStats.PityAdjustment | ||
| effectiveMinimumFailure := opts.MinimumFailure | ||
|
|
||
| status = crtest.NotSignificant | ||
|
|
||
| samplePassPercentage := float64(samplePass) / float64(testStats.SampleStats.Total()) | ||
|
|
||
| if effectiveMinimumFailure != 0 && | ||
| (testStats.SampleStats.Total()-samplePass) < effectiveMinimumFailure { | ||
| if status <= crtest.SignificantTriagedRegression { | ||
| testStats.Explanations = append(testStats.Explanations, | ||
| fmt.Sprintf("%s regression detected.", crtest.StringForStatus(status))) | ||
| } | ||
| testStats.ReportStatus = status | ||
| testStats.FisherExact = thrift.Float64Ptr(0.0) | ||
| return true, nil | ||
| } | ||
| significant := false | ||
| improved := samplePassPercentage >= basisPassPercentage | ||
|
|
||
| if improved { | ||
| significant, fisherExactVal = fisherExactTest(testStats.RequiredConfidence, testStats.BaseStats.Total()-basePass, basePass, testStats.SampleStats.Total()-samplePass, samplePass) | ||
| } else if basisPassPercentage-samplePassPercentage > effectivePityFactor/100 { | ||
| significant, fisherExactVal = fisherExactTest(testStats.RequiredConfidence, testStats.SampleStats.Total()-samplePass, samplePass, testStats.BaseStats.Total()-basePass, basePass) | ||
| } | ||
| logger.Debugf("computed Fisher info: signifcant: %v, fisherExact: %v", significant, fisherExactVal) | ||
| if significant { | ||
| if improved { | ||
| status = crtest.SignificantImprovement | ||
| } else { | ||
| status = getRegressionStatus(basisPassPercentage, samplePassPercentage) | ||
| } | ||
| } | ||
| } | ||
| logger.Debugf("computed status: %d", int(status)) | ||
| testStats.ReportStatus = status | ||
| testStats.FisherExact = thrift.Float64Ptr(fisherExactVal) | ||
|
|
||
| baseRelease := "no basis" | ||
| if testStats.BaseStats != nil { | ||
| baseRelease = testStats.BaseStats.Release | ||
| } | ||
| if testStats.ReportStatus <= crtest.SignificantTriagedRegression { | ||
| logger.Debugf("regression detected against: %s", baseRelease) | ||
|
|
||
| if testStats.ReportStatus <= crtest.SignificantRegression { | ||
| testStats.Explanations = append(testStats.Explanations, | ||
| fmt.Sprintf("%s regression detected.", crtest.StringForStatus(testStats.ReportStatus))) | ||
| testStats.Explanations = append(testStats.Explanations, | ||
| fmt.Sprintf("Fishers Exact probability of a regression: %.2f%%.", float64(100)-*testStats.FisherExact)) | ||
| testStats.Explanations = append(testStats.Explanations, | ||
| fmt.Sprintf("Test pass rate dropped from %.2f%% to %.2f%%.", | ||
| testStats.BaseStats.SuccessRate*float64(100), | ||
| testStats.SampleStats.SuccessRate*float64(100))) | ||
| } else { | ||
| testStats.Explanations = append(testStats.Explanations, | ||
| fmt.Sprintf("%s regression detected.", crtest.StringForStatus(testStats.ReportStatus))) | ||
| } | ||
| } else { | ||
| logger.Debugf("NO regression detected against: %s", baseRelease) | ||
| } | ||
|
|
||
| return true, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add comprehensive unit tests for the Fisher exact analysis logic.
The Analyze method is the most complex middleware with multiple conditional branches and policy decisions. As per coding guidelines, new functions with non-trivial behavior should have unit tests.
The method should be tested with cases covering:
- Missing data scenarios: sample=0 with IgnoreMissing true/false → NotSignificant/MissingSample
- Minimum failure triage: degraded but below MinimumFailure threshold → NotSignificant
- Improved performance: sample pass rate > base → SignificantImprovement when Fisher test passes
- Degraded performance within pity: drop ≤ pity factor → NotSignificant
- Degraded performance beyond pity: Fisher significant + drop < 15% → SignificantRegression
- Extreme degradation: Fisher significant + drop ≥ 15% → ExtremeRegression
- Confidence requirement: Fisher test respects RequiredConfidence parameter
As per coding guidelines: "New or modified functionality must include test coverage: New Go functions and methods should have corresponding unit tests."
🤖 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/api/componentreadiness/middleware/fisherexact/fisherexact.go` around
lines 46 - 137, Add comprehensive unit tests for the FisherExact.Analyze method
to cover all conditional branches and policy decisions: create table-driven
tests that construct testdetails.TestComparison inputs (including BaseStats,
SampleStats, RequiredConfidence, PityAdjustment) and
FisherExact.reqOptions.AdvancedOption settings to exercise missing-data (sample
total 0 with IgnoreMissing true/false), minimum-failure triage (MinimumFailure
threshold), improved performance (sample pass rate > base with fisherExactTest
returning significant), degraded within-pity (drop <= PityFactor →
NotSignificant), degraded beyond-pity (fisherExactTest significant producing
SignificantRegression or ExtremeRegression depending on drop < 15% vs >= 15%),
and confidence threshold behavior (RequiredConfidence overrides option). For
each case assert returned bool/error, TestComparison.ReportStatus, FisherExact
value, and Explanations; mock or control fisherExactTest outputs where needed to
deterministically simulate significance.
Source: Coding guidelines
Comment on lines
+41
to
+56
| // Analyze claims tests that have no base stats (new tests) when PassRateRequiredNewTests is configured. | ||
| func (n *NewTestPassRate) Analyze(_ crtest.Identification, testStats *testdetails.TestComparison) (bool, error) { | ||
| opts := n.reqOptions.AdvancedOption | ||
| if opts.PassRateRequiredNewTests == 0 { | ||
| return false, nil | ||
| } | ||
| if testStats.BaseStats != nil && testStats.BaseStats.Total() > 0 { | ||
| return false, nil | ||
| } | ||
|
|
||
| analysis.BuildPassRateTestStats(testStats, float64(opts.PassRateRequiredNewTests), opts) | ||
| if testStats.ReportStatus == crtest.NotSignificant && opts.PassRateRequiredAllTests == 0 { | ||
| testStats.ReportStatus = crtest.MissingBasis | ||
| } | ||
| return true, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add unit tests for the Analyze method.
The Analyze method contains non-trivial conditional logic with multiple code paths:
- Early return when
PassRateRequiredNewTestsis 0 - Early return when
BaseStatsexists - Call to
analysis.BuildPassRateTestStats - Conditional override to
MissingBasisstatus
As per coding guidelines, new Go functions with non-trivial behavior should have corresponding unit tests. Consider adding test cases covering:
- PassRateRequiredNewTests == 0 (should not handle)
- BaseStats present (should not handle)
- New test with passing sample → NotSignificant
- New test with failing sample → SignificantRegression/ExtremeRegression
- Status override to MissingBasis when PassRateRequiredAllTests == 0
As per coding guidelines: "New or modified functionality must include test coverage: New Go functions and methods should have corresponding unit tests."
🤖 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/api/componentreadiness/middleware/newtestpassrate/newtestpassrate.go`
around lines 41 - 56, Add unit tests exercising NewTestPassRate.Analyze covering
all branches: (1) when reqOptions.AdvancedOption.PassRateRequiredNewTests == 0
it should return handled=false; (2) when testStats.BaseStats != nil and
BaseStats.Total()>0 it should return handled=false; (3) for a new test
(BaseStats nil) with passing samples assert analysis.BuildPassRateTestStats
yields ReportStatus == crtest.NotSignificant; (4) for a new test with failing
samples assert BuildPassRateTestStats results in a regression status (e.g.,
SignificantRegression/ExtremeRegression); and (5) when ReportStatus remains
crtest.NotSignificant and PassRateRequiredAllTests == 0 assert Analyze sets
ReportStatus to crtest.MissingBasis. Create test fixtures for
testdetails.TestComparison inputs, stub or exercise
analysis.BuildPassRateTestStats as appropriate, and assert the returned bool and
testStats.ReportStatus for each case.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/componentreadiness/middleware/fisherexact/fisherexact_test.go (1)
191-202: ⚡ Quick winUse
strings.Containsfor substring search.The manual substring search implementation reimplements
strings.Contains. As per coding guidelines, favor clarity over cleverness and prefer standard library functions.♻️ Proposed refactor
+import ( + "strings" +) + if tt.explanationContains != "" { - assert.True(t, func() bool { - for _, e := range tt.inputStats.Explanations { - if len(e) >= len(tt.explanationContains) { - for i := 0; i <= len(e)-len(tt.explanationContains); i++ { - if e[i:i+len(tt.explanationContains)] == tt.explanationContains { - return true - } - } - } - } - return false - }(), "expected an explanation containing %q, got: %v", tt.explanationContains, tt.inputStats.Explanations) + found := false + for _, e := range tt.inputStats.Explanations { + if strings.Contains(e, tt.explanationContains) { + found = true + break + } + } + assert.True(t, found, "expected an explanation containing %q, got: %v", tt.explanationContains, tt.inputStats.Explanations) }🤖 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/api/componentreadiness/middleware/fisherexact/fisherexact_test.go` around lines 191 - 202, Replace the manual substring search in the test assertion with strings.Contains: instead of the anonymous loop over tt.inputStats.Explanations comparing slices, call strings.Contains(e, tt.explanationContains) for each explanation e (and ensure the test imports the "strings" package). Update the assertion to assert that any e in tt.inputStats.Explanations satisfies strings.Contains(e, tt.explanationContains) and keep the existing failure message and parameters (tt.explanationContains, tt.inputStats.Explanations).Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@pkg/api/componentreadiness/middleware/fisherexact/fisherexact_test.go`:
- Around line 191-202: Replace the manual substring search in the test assertion
with strings.Contains: instead of the anonymous loop over
tt.inputStats.Explanations comparing slices, call strings.Contains(e,
tt.explanationContains) for each explanation e (and ensure the test imports the
"strings" package). Update the assertion to assert that any e in
tt.inputStats.Explanations satisfies strings.Contains(e, tt.explanationContains)
and keep the existing failure message and parameters (tt.explanationContains,
tt.inputStats.Explanations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3de19042-3be6-4114-b3dc-ce92770ed6b8
📒 Files selected for processing (7)
pkg/api/componentreadiness/middleware/alltestspassrate/alltestspassrate_test.gopkg/api/componentreadiness/middleware/fisherexact/fisherexact_test.gopkg/api/componentreadiness/middleware/newtestpassrate/newtestpassrate_test.gopkg/variantregistry/ocp.gopkg/variantregistry/ocp_test.gopkg/variantregistry/snapshot.gopkg/variantregistry/snapshot.yaml
✅ Files skipped from review due to trivial changes (1)
- pkg/api/componentreadiness/middleware/newtestpassrate/newtestpassrate_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/variantregistry/ocp_test.go
- pkg/variantregistry/ocp.go
- pkg/variantregistry/snapshot.yaml
Contributor
|
Scheduling required tests: |
Contributor
|
Scheduling required tests: |
Contributor
|
@dgoodwin: all tests passed! 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From the next-gen QE effort, we are looking to implement spot check jobs, jobs which are much less sensitive, have an owning component/capability, test a specific install configuration or stable feature, once in awhile, to confirm it still works. The goal is for them to run just once a month, be periodically retried if they fail every day or so up to perhaps four attempts, and after roughly 2 failures the component goes regressed in component readiness. One pass is all we require.
This is intended to provide a mechanism to land the many jobs QE has for obscure configurations, and for feature jobs that ran often for GA promotion to continue producing signal without a lot of cost. Examples today would include etcd-scaling job (which has never passed, but this is due to monitortests, spot check jobs will not run failable monitortests: openshift/origin#31212)
This PR adds the mechanism to have them mark as regressions in component readiness.
This attempts something we talked about for a long time, what if component readiness could show signal for other things.
Middleware concept pays off as this stays fairly localized to middleware that gets it's chance to inject the synthetic test results and handle all drilldown/reports.
Jobs must have capabilities/components for this system, I did this via the variant registry.
This replaces the admittedly terrible prior attempt, rarely run jobs: #3562
Implications:
Summary by CodeRabbit
New Features
UI Changes