TRT-2471: Consider mass failures during CR query#3285
TRT-2471: Consider mass failures during CR query#3285xueqzhan wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@xueqzhan: This pull request references TRT-2471 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe PR implements exclusive test name filtering for component readiness reports. It introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xueqzhan 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
|
@xueqzhan: This pull request references TRT-2471 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go (1)
541-567:⚠️ Potential issue | 🟡 MinorRemove unused
exclusiveTestNamesparameter fromFetchTestStatusResults.The
exclusiveTestNamesparameter is declared in the function signature but never used in the function body. Query-level filtering for exclusive test names is already handled byBuildComponentReportQuery(which incorporates the filtering into the SQL query itself), making this parameter redundant. Either remove the parameter or document why it's retained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go` around lines 541 - 567, The FetchTestStatusResults function's signature includes an unused exclusiveTestNames parameter; remove that parameter from the function declaration (query.FetchTestStatusResults) and then update all call sites (e.g., this file's fallbackTestQueryGenerator.getTestFallbackRelease where it's called as query.FetchTestStatusResults(ctx, baseQuery, f.ReqOptions.AdvancedOption.ExclusiveTestNames)) to call the new two-argument form query.FetchTestStatusResults(ctx, baseQuery). Ensure you update any other references, associated unit tests, and imports accordingly so compilation succeeds.
🧹 Nitpick comments (2)
pkg/api/componentreadiness/queryparamparser_test.go (1)
405-405: Consider adding test coverage for non-nil exclusiveTestNames.The test correctly passes
nilfor backward compatibility, but there are no test cases verifying behavior whenexclusiveTestNamesis provided. Consider adding a test case to verifyopts.AdvancedOption.ExclusiveTestNamesis correctly populated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/componentreadiness/queryparamparser_test.go` at line 405, Add a unit test in queryparamparser_test.go that calls utils.ParseComponentReportRequest with a non-nil exclusiveTestNames slice (instead of nil) and asserts that the returned options.AdvancedOption.ExclusiveTestNames is populated with the same values; specifically exercise ParseComponentReportRequest (the call in the existing test that currently passes nil) and add assertions on the returned options.AdvancedOption.ExclusiveTestNames to confirm it contains the expected test names and preserves order/contents.pkg/api/componentreadiness/query/querygenerators.go (1)
670-707: Remove unusedexclusiveTestNamesparameter fromFetchTestStatusResultsfunction signature.The
exclusiveTestNamesparameter is accepted but never referenced in the function body. Exclusive test filtering is handled at query construction time inBuildComponentReportQuery, so this parameter serves no purpose inFetchTestStatusResults. Removing it will simplify the API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/componentreadiness/query/querygenerators.go` around lines 670 - 707, The FetchTestStatusResults function accepts an unused exclusiveTestNames parameter; remove exclusiveTestNames from the function signature of FetchTestStatusResults and update all call sites to stop passing that argument (the query already handles exclusivity in BuildComponentReportQuery), keeping the function body unchanged other than the signature; ensure any imports or interfaces that referenced the old signature are updated accordingly so compilation succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/api/componentreadiness/query/querygenerators.go`:
- Around line 50-51: Remove the duplicated comment line "// partition." that
appears immediately before the dedupedJunitTable declaration; locate the comment
just above the dedupedJunitTable variable in querygenerators.go and delete the
extra duplicate so only a single "// partition." comment remains.
---
Outside diff comments:
In `@pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go`:
- Around line 541-567: The FetchTestStatusResults function's signature includes
an unused exclusiveTestNames parameter; remove that parameter from the function
declaration (query.FetchTestStatusResults) and then update all call sites (e.g.,
this file's fallbackTestQueryGenerator.getTestFallbackRelease where it's called
as query.FetchTestStatusResults(ctx, baseQuery,
f.ReqOptions.AdvancedOption.ExclusiveTestNames)) to call the new two-argument
form query.FetchTestStatusResults(ctx, baseQuery). Ensure you update any other
references, associated unit tests, and imports accordingly so compilation
succeeds.
---
Nitpick comments:
In `@pkg/api/componentreadiness/query/querygenerators.go`:
- Around line 670-707: The FetchTestStatusResults function accepts an unused
exclusiveTestNames parameter; remove exclusiveTestNames from the function
signature of FetchTestStatusResults and update all call sites to stop passing
that argument (the query already handles exclusivity in
BuildComponentReportQuery), keeping the function body unchanged other than the
signature; ensure any imports or interfaces that referenced the old signature
are updated accordingly so compilation succeeds.
In `@pkg/api/componentreadiness/queryparamparser_test.go`:
- Line 405: Add a unit test in queryparamparser_test.go that calls
utils.ParseComponentReportRequest with a non-nil exclusiveTestNames slice
(instead of nil) and asserts that the returned
options.AdvancedOption.ExclusiveTestNames is populated with the same values;
specifically exercise ParseComponentReportRequest (the call in the existing test
that currently passes nil) and add assertions on the returned
options.AdvancedOption.ExclusiveTestNames to confirm it contains the expected
test names and preserves order/contents.
| // partition. | ||
| dedupedJunitTable = ` |
There was a problem hiding this comment.
Remove duplicate comment line.
Line 50 appears to be a duplicate of the comment ending on line 49 (// partition.). This looks like an editing artifact.
Proposed fix
// 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 = `📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // partition. | |
| dedupedJunitTable = ` | |
| // 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. | |
| dedupedJunitTable = ` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/api/componentreadiness/query/querygenerators.go` around lines 50 - 51,
Remove the duplicated comment line "// partition." that appears immediately
before the dedupedJunitTable declaration; locate the comment just above the
dedupedJunitTable variable in querygenerators.go and delete the extra duplicate
so only a single "// partition." comment remains.
|
Scheduling required tests: |
|
@xueqzhan: 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. |
2 participants
We use a list of key tests to detect mass failure situation. When one of those key tests appear in the failed test list in a job, we don't consider the other tests for the purpose of regression calculation. This will clear the board and only show regression in the key test component.
Current list of key tests:
The list is ordered. Only the lowest indexed one is counted if multiple key tests appear in a job's failed tests.
Summary by CodeRabbit
Release Notes
--exclude-mass-failurescommand-line flag to filter out tests that commonly fail across multiple components, improving accuracy of component readiness reports