Improvements and fixes for new Feature Gate page#3779
Conversation
We want to eventually move these to using tests should all work, but that requires these jobs to move to more minimal test suites that do not yet exist
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughThe change adds AWS dual-stack install synthetic data, selects install-specific capability tests in server links and frontend filters, updates feature-gate routing and test tables, and adds end-to-end coverage for the capability link. ChangesFeature-gate capability flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant FeatureGateDetailWrapper
participant FeatureGateDetail
participant TestTable
participant TestsAPI
FeatureGateDetailWrapper->>FeatureGateDetail: resolve latest release and render detail
FeatureGateDetail->>TestTable: pass annotation or capability filters
TestTable->>TestsAPI: fetch filtered tests
TestsAPI-->>TestTable: return test rows
TestTable-->>FeatureGateDetail: report loaded row count
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 19 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (19 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
dgoodwin
changed the title
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cmd/sippy/seed_data.go`:
- Around line 376-382: Update the test-count mapping for
test-cap-aws-dual-stack-install so it does not seed the same install should
succeed: overall test already included by test-install-overall through
allJobCounts. Make the mappings exclusive or assign this capability a distinct
test name, ensuring each test/run combination is seeded only once.
In `@sippy-ng/src/tests/FeatureGateDetail.js`:
- Around line 74-79: Update handleAnnotationDataLoaded so automatic tab
selection occurs only when count === 0, preserving the existing setActiveTab and
tabAutoSelected behavior for empty annotation data.
- Around line 200-217: Update the two TestTable instances in FeatureGateDetail
so their URL-backed query state is isolated per tab. Provide distinct
query-state prefixes or controlled values for the annotation and capability
tables covering filters, sort, sortField, view, period, and pageSize, while
preserving each table’s existing filterModel behavior.
🪄 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: 4e76bfa2-c646-4cbb-b4f5-94fab772c6d5
📒 Files selected for processing (7)
cmd/sippy/seed_data.gopkg/sippyserver/server.gosippy-ng/src/App.jssippy-ng/src/tests/FeatureGateDetail.jssippy-ng/src/tests/FeatureGates.jssippy-ng/src/tests/TestTable.jstest/e2e/feature_gates_test.go
| const handleAnnotationDataLoaded = React.useCallback((count) => { | ||
| if (!tabAutoSelected.current && count <= 1) { | ||
| setActiveTab(1) | ||
| tabAutoSelected.current = true | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Only auto-switch when annotations are empty.
Line 75 uses count <= 1, so a gate with one annotated test is redirected to capability results despite having annotation data. Change this to count === 0.
🤖 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/tests/FeatureGateDetail.js` around lines 74 - 79, Update
handleAnnotationDataLoaded so automatic tab selection occurs only when count ===
0, preserving the existing setActiveTab and tabAutoSelected behavior for empty
annotation data.
There was a problem hiding this comment.
This is required because there's an implicit Overall row. We should comment this.
There was a problem hiding this comment.
@dgoodwin Thanks for the clarification — that makes sense, and it means my original suggestion to change this to count === 0 was incorrect. Since TestTable always includes an implicit "Overall" row, count === 1 actually means there are no real per-test annotation rows, so count <= 1 is the right threshold.
Here's a suggested comment to make that explicit:
const handleAnnotationDataLoaded = React.useCallback((count) => {
// TestTable always includes an implicit "Overall" row, so a count of 1
// means there are no real annotated tests. Auto-switch to the capability
// tab in that case.
if (!tabAutoSelected.current && count <= 1) {
setActiveTab(1)
tabAutoSelected.current = true
}
}, [])✏️ Learnings added
Learnt from: dgoodwin
Repo: openshift/sippy PR: 3779
File: sippy-ng/src/tests/FeatureGateDetail.js:74-79
Timestamp: 2026-07-15T18:06:01.572Z
Learning: In sippy-ng/src/tests/FeatureGateDetail.js, `TestTable` (sippy-ng/src/tests/TestTable.js) always includes an implicit "Overall" row in its loaded row count reported via `onDataLoaded`. So in `handleAnnotationDataLoaded`, checking `count <= 1` (not `count === 0`) is the correct way to detect "no real annotated tests" and auto-switch the active tab to "Tests by Capability".
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| <Box sx={{ display: activeTab === 0 ? 'block' : 'none' }}> | ||
| <TestTable | ||
| key={'fg-annotation-' + featureGate} | ||
| release={release} | ||
| collapse={false} | ||
| filterModel={annotationFilter} | ||
| onDataLoaded={handleAnnotationDataLoaded} | ||
| /> | ||
| )} | ||
| </Box> | ||
|
|
||
| {activeTab === 1 && gate.links && ( | ||
| <TestResultsSection | ||
| title="Tests matching capability variant" | ||
| apiUrl={gate.links.tests_by_capability} | ||
| <Box sx={{ display: activeTab === 1 ? 'block' : 'none' }}> | ||
| <TestTable | ||
| key={'fg-capability-' + featureGate} | ||
| release={release} | ||
| collapse={false} | ||
| filterModel={capabilityFilter} | ||
| /> | ||
| )} | ||
| </Box> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Isolate URL state for the two TestTable instances.
Both mounted tables use the same URL-backed query keys (filters, sort, sortField, view, period, and pageSize). Filtering or sorting one tab updates the hidden tab too; when revisited, it can fetch with the other tab’s filter and show incorrect results. Add a per-instance query-state prefix (or make this state fully controlled) and pass distinct values for annotation and capability tables.
🤖 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/tests/FeatureGateDetail.js` around lines 200 - 217, Update the
two TestTable instances in FeatureGateDetail so their URL-backed query state is
isolated per tab. Provide distinct query-state prefixes or controlled values for
the annotation and capability tables covering filters, sort, sortField, view,
period, and pageSize, while preserving each table’s existing filterModel
behavior.
There was a problem hiding this comment.
Beyond scope for this I think.
There was a problem hiding this comment.
@dgoodwin, understood — acknowledged as out of scope for this PR. Let me know if you'd like me to open a follow-up issue to track isolating the URL-backed query state (filters, sort, sortField, view, period, pageSize) for the two TestTable instances, and I'll create it.
|
Scheduling required tests: |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/sippy/seed_data.go (1)
377-380: 🗄️ Data Integrity & Integration | 🟠 MajorKeep capability and general infrastructure test mappings exclusive.
Line 377 still reuses
install should succeed: infrastructure, whichtest-install-infraseeds throughallJobCountsfor the new capability job. Because tests are resolved by name, the first 50 test/run combinations can be inserted twice, causing uniqueness failures or inflated results. Exclude the capability job from the general mapping, or use a distinct test name and update its consumers.🤖 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 `@cmd/sippy/seed_data.go` around lines 377 - 380, Update the seed mappings around the capability entry for awsAmd64CapabilityAWSDualStackInstall so it does not reuse the test name “install should succeed: infrastructure” from allJobCounts/test-install-infra. Exclude this capability job from the general infrastructure mapping, or assign it a distinct test name and update every consumer consistently, ensuring each test/run combination is seeded only once.
🤖 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.
Duplicate comments:
In `@cmd/sippy/seed_data.go`:
- Around line 377-380: Update the seed mappings around the capability entry for
awsAmd64CapabilityAWSDualStackInstall so it does not reuse the test name
“install should succeed: infrastructure” from allJobCounts/test-install-infra.
Exclude this capability job from the general infrastructure mapping, or assign
it a distinct test name and update every consumer consistently, ensuring each
test/run combination is seeded only once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 27c7e1e4-eb19-48cb-bd55-dd583614024e
📒 Files selected for processing (3)
cmd/sippy/seed_data.gosippy-ng/src/tests/FeatureGateDetail.jstest/e2e/feature_gates_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/feature_gates_test.go
- sippy-ng/src/tests/FeatureGateDetail.js
|
Scheduling required tests: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, neisw 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 |
|
@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. |
2 participants
I was a bit ambitious with the move to capability jobs and all tests should pass, this is not feasible for installer gates in flight as they still run full test suites that fully pass very rarely, and no true conformance suite exists yet. For now, install gates will return to requiring good pass results for install should succeed tests as they did before.
Summary by CodeRabbit