TRT-2769: WIP: Rework feature gate test filtering to cover component owned jobs#3731
TRT-2769: WIP: Rework feature gate test filtering to cover component owned jobs#3731dgoodwin wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
[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 |
dgoodwin
changed the title
openshift-ci
Bot
added
approved
WalkthroughAdds seeded feature-gate scenarios, HATEOAS links in API responses, a frontend feature-gate detail page with linked test-result tabs, updated list navigation, and end-to-end API coverage. ChangesFeature Gate Detail Flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant FeatureGates
participant FeatureGateDetail
participant SippyServer
participant TestResultsAPI
Browser->>FeatureGates: Load feature gates
FeatureGates->>SippyServer: GET /api/feature_gates
SippyServer-->>FeatureGates: Return gates with links
Browser->>FeatureGateDetail: Navigate to ui_detail
FeatureGateDetail->>SippyServer: Fetch filtered feature gate
SippyServer-->>FeatureGateDetail: Return gate metadata and links
FeatureGateDetail->>TestResultsAPI: Fetch annotation or capability results
TestResultsAPI-->>FeatureGateDetail: Return test rows
FeatureGateDetail-->>Browser: Render details and selected tab
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (17 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
sippy-ng/src/tests/FeatureGateDetail.js (2)
136-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing existing filter helpers instead of hand-building the payload.
This manually builds a filter object with
JSON.stringify+safeEncodeURIComponent. The repo providesfilterFor/single/multiple/multiple_orfor building filter payloads andpathForAPIWithFilterfor appending the encodedfilters=query param — reusing these would keep encoding consistent with the rest of the codebase and reduce duplicated logic.As per path instructions, "Build filter payloads with
filterFor(column, operator, value)plussingle()/multiple()/multiple_or()" and "usepathForAPIWithFilter(apiPath, filter)to append the encodedfilters=...query param".🤖 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 136 - 154, The FeatureGateDetail test is manually constructing and encoding a filter payload instead of using the shared filter helpers. Update the fetch URL-building logic to create the filter with filterFor plus single/multiple/multiple_or as appropriate, and append it with pathForAPIWithFilter rather than JSON.stringify and safeEncodeURIComponent. Use the existing FeatureGateDetail fetch call and the API path construction there as the place to swap in the shared helpers so encoding stays consistent.Source: Path instructions
148-154: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
releaseisn't URL-encoded in the fetch URL.
releaseis concatenated directly into the query string withoutencodeURIComponent/safeEncodeURIComponent. Low risk given release names are typically simple version strings, but worth guarding for consistency with the rest of the file (which does encode the filter).🤖 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 148 - 154, The fetch URL construction in FeatureGateDetail’s request builder concatenates release directly into the query string, unlike filterParam which is encoded. Update the URL assembly in the fetch call to pass release through encodeURIComponent or safeEncodeURIComponent before appending it, so the query string is consistently encoded and safe.pkg/sippyserver/server.go (1)
693-698: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUndocumented trailing
]anchor; alsoreleaseis interpolated unescaped into generated URLs.The
"FeatureGate:%s]"trick is non-obvious and has no comment explaining why the bracket is there — worth a one-line comment for future maintainers. Separately,releaseis embedded via plain%sintests_by_annotation,tests_by_capability, andui_detailwithouturl.QueryEscape/url.PathEscape(unlikefg.FeatureGate), which is inconsistent even though current risk is low since an invalidreleasevalue would simply fail to match any DB rows upstream.As per coding guidelines, "Keep comments minimal and helpful, and make them explain the 'why' rather than the 'what'."
♻️ Suggested tweak
+ // Trailing "]" anchors the match to the annotation's closing bracket so a shorter + // gate name can't match as a prefix of a longer, similarly-named gate. annotationFilter := url.QueryEscape(fmt.Sprintf( `{"items":[{"columnField":"name","operatorValue":"contains","value":"FeatureGate:%s]"}]}`, fg.FeatureGate)) fg.Links["tests_by_annotation"] = fmt.Sprintf( "%s/api/tests?release=%s&filter=%s", - baseAPIURL, release, annotationFilter) + baseAPIURL, url.QueryEscape(release), annotationFilter)🤖 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/sippyserver/server.go` around lines 693 - 698, Add a brief explanatory comment near the FeatureGate annotation filter in server.go to document why the trailing ] is intentionally included in the value, and make the generated URLs consistent by escaping release before interpolating it into tests_by_annotation, tests_by_capability, and ui_detail in the same code path that already uses url.QueryEscape for fg.FeatureGate.Source: Coding guidelines
test/e2e/util/e2erequest.go (1)
42-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
nolintjustification for the new exported signature.The
#nosec/nolintcomment on theclient.Get(url)call says the URL is "constructed from test helper's hardcoded localhost base URL," butSippyGetAbsoluteis now also invoked directly with server-returned HATEOAS links (seefeature_gates_test.go), not justBuildE2EURLoutput. Low risk given this is e2e test code targeting the same server, but worth updating the comment so the suppression rationale stays accurate.🤖 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 `@test/e2e/util/e2erequest.go` around lines 42 - 48, The suppression comment on SippyGetAbsolute’s client.Get(url) call is outdated because this exported helper can now receive direct HATEOAS links, not only BuildE2EURL output. Update the nolint/gosec justification in SippyGetAbsolute to describe the broader e2e test usage accurately, while keeping the same suppression on the http.Client request line.
🤖 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/sippyserver/server.go`:
- Around line 690-710: The HATEOAS link builder in injectFeatureGateHATEOASLinks
is too narrow and can return incorrect test links. Update tests_by_annotation to
match both feature-gate tag forms, including OCPFeatureGate alongside
FeatureGate, and adjust tests_by_capability to use exact array membership
semantics (“has entry”) rather than substring-based contains so unrelated tests
are not matched by prefix collisions.
In `@sippy-ng/src/App.js`:
- Around line 262-275: The redirect logic in FeatureGateDetailWrapper is
updating the wrong path segment for the “latest” release case. When useParams()
returns release === 'latest', change the navigation in
RedirectLatestReleaseWrapper usage so it targets the full
/feature_gates/${defaultRelease}/${feature_gate} URL instead of a relative ../
redirect. Keep the existing release and feature_gate values from
FeatureGateDetailWrapper/FeatureGateDetail, but ensure the redirect writes the
release segment so the loop stops.
In `@sippy-ng/src/tests/FeatureGateDetail.js`:
- Around line 101-112: The sorting state in DataGrid is controlled but never
updated, so header clicks cannot change the order. Update the FeatureGateDetail
DataGrid setup to manage sort state with a component state value, and wire
`onSortModelChange` to persist user changes instead of always passing the fixed
`sortModel` literal. Keep the existing default sort for initial render, but make
sure `DataGrid` in `FeatureGateDetail` reflects the current sort model on
subsequent renders.
- Around line 204-213: SimpleBreadcrumbs is being passed a crumbs prop that it
ignores, so the Feature Gates back link never renders. Update the
FeatureGateDetail test/setup to use the supported previousPage prop on
SimpleBreadcrumbs instead of crumbs, keeping release and currentPage as-is so
the breadcrumb can show the Feature Gates link correctly.
---
Nitpick comments:
In `@pkg/sippyserver/server.go`:
- Around line 693-698: Add a brief explanatory comment near the FeatureGate
annotation filter in server.go to document why the trailing ] is intentionally
included in the value, and make the generated URLs consistent by escaping
release before interpolating it into tests_by_annotation, tests_by_capability,
and ui_detail in the same code path that already uses url.QueryEscape for
fg.FeatureGate.
In `@sippy-ng/src/tests/FeatureGateDetail.js`:
- Around line 136-154: The FeatureGateDetail test is manually constructing and
encoding a filter payload instead of using the shared filter helpers. Update the
fetch URL-building logic to create the filter with filterFor plus
single/multiple/multiple_or as appropriate, and append it with
pathForAPIWithFilter rather than JSON.stringify and safeEncodeURIComponent. Use
the existing FeatureGateDetail fetch call and the API path construction there as
the place to swap in the shared helpers so encoding stays consistent.
- Around line 148-154: The fetch URL construction in FeatureGateDetail’s request
builder concatenates release directly into the query string, unlike filterParam
which is encoded. Update the URL assembly in the fetch call to pass release
through encodeURIComponent or safeEncodeURIComponent before appending it, so the
query string is consistently encoded and safe.
In `@test/e2e/util/e2erequest.go`:
- Around line 42-48: The suppression comment on SippyGetAbsolute’s
client.Get(url) call is outdated because this exported helper can now receive
direct HATEOAS links, not only BuildE2EURL output. Update the nolint/gosec
justification in SippyGetAbsolute to describe the broader e2e test usage
accurately, while keeping the same suppression on the http.Client request line.
🪄 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: d8d39b25-4310-40ad-8d3d-fa26362b5e58
📒 Files selected for processing (8)
cmd/sippy/seed_data.gopkg/apis/api/types.gopkg/sippyserver/server.gosippy-ng/src/App.jssippy-ng/src/tests/FeatureGateDetail.jssippy-ng/src/tests/FeatureGates.jstest/e2e/feature_gates_test.gotest/e2e/util/e2erequest.go
| func injectFeatureGateHATEOASLinks(fg *apitype.FeatureGate, release, baseAPIURL, baseFrontendURL string) { | ||
| fg.Links = make(map[string]string, 3) | ||
|
|
||
| annotationFilter := url.QueryEscape(fmt.Sprintf( | ||
| `{"items":[{"columnField":"name","operatorValue":"contains","value":"FeatureGate:%s]"}]}`, | ||
| fg.FeatureGate)) | ||
| fg.Links["tests_by_annotation"] = fmt.Sprintf( | ||
| "%s/api/tests?release=%s&filter=%s", | ||
| baseAPIURL, release, annotationFilter) | ||
|
|
||
| capabilityFilter := url.QueryEscape(fmt.Sprintf( | ||
| `{"items":[{"columnField":"name","operatorValue":"contains","value":"openshift-tests should work"},{"columnField":"variants","operatorValue":"contains","value":"Capability:%s"}],"linkOperator":"and"}`, | ||
| fg.FeatureGate)) | ||
| fg.Links["tests_by_capability"] = fmt.Sprintf( | ||
| "%s/api/tests?release=%s&filter=%s", | ||
| baseAPIURL, release, capabilityFilter) | ||
|
|
||
| fg.Links["ui_detail"] = fmt.Sprintf( | ||
| "%s/feature_gates/%s/%s", | ||
| baseFrontendURL, release, url.PathEscape(fg.FeatureGate)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C5 '"variants"' --type=go pkg/filter pkg/db/query 2>/dev/null
rg -n -C5 'columnField.*variants|case "variants"' --type=goRepository: openshift/sippy
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and search for feature-gate link construction and filter handling.
ast-grep outline pkg/sippyserver/server.go --view expanded || true
rg -n -C4 'injectFeatureGateHATEOASLinks|tests_by_capability|tests_by_annotation|FeatureGate:' pkg/sippyserver/server.go
rg -n -C4 'columnField.*variants|variants.*contains|case "variants"|operatorValue.*contains' pkg --type=goRepository: openshift/sippy
Length of output: 18733
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any filter/query code that may define "contains" semantics.
rg -n -C4 'contains' pkg --type=go
rg -n -C4 '"variants"' pkg --type=go
rg -n -C4 'FeatureGate|feature gate|feature_gate' pkg --type=goRepository: openshift/sippy
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the filter implementation to determine how "contains" behaves for array fields.
ast-grep outline pkg/filter --view expanded || true
rg -n -C4 'func .*contains|Operator: "contains"|strings.Contains|Contains' pkg/filter --type=go
sed -n '1,260p' pkg/filter/filterable.goRepository: openshift/sippy
Length of output: 20233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how feature-gate annotations are represented and whether the trailing bracket is intentional.
rg -n -C3 'FeatureGate:' pkg --type=go
rg -n -C3 'FeatureGate.*\]' pkg --type=go
rg -n -C3 'annotations|annotation' pkg/apis pkg/filter pkg/sippyserver --type=go | head -n 200Repository: openshift/sippy
Length of output: 12971
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the feature-gate response type to see how links and gate names are represented.
sed -n '1040,1105p' pkg/apis/api/types.goRepository: openshift/sippy
Length of output: 1594
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for existing feature-gate names that might already share prefixes.
rg -n 'FeatureGate:' pkg --type=goRepository: openshift/sippy
Length of output: 562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the in-memory array filter semantics for the exact operator we should use here.
sed -n '640,660p' pkg/filter/filterable.goRepository: openshift/sippy
Length of output: 587
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the FeatureGate API type to see how its name/value fields are used.
sed -n '1070,1100p' pkg/apis/api/types.goRepository: openshift/sippy
Length of output: 713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for OCPFeatureGate usage to understand whether it should be included in the link filter.
rg -n 'OCPFeatureGate' .Repository: openshift/sippy
Length of output: 983
Include both feature-gate tag forms
tests_by_annotationonly matchesFeatureGate:%s], but the data also carriesOCPFeatureGate:%s]tags, so this link can miss valid tests.tests_by_capabilityshould use exact array membership (has entry) instead ofcontains; array filtering here is substring-based, so prefix collisions can pull in unrelated 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/sippyserver/server.go` around lines 690 - 710, The HATEOAS link builder
in injectFeatureGateHATEOASLinks is too narrow and can return incorrect test
links. Update tests_by_annotation to match both feature-gate tag forms,
including OCPFeatureGate alongside FeatureGate, and adjust tests_by_capability
to use exact array membership semantics (“has entry”) rather than
substring-based contains so unrelated tests are not matched by prefix
collisions.
| <DataGrid | ||
| loading={!isLoaded} | ||
| rows={rows} | ||
| columns={columns} | ||
| getRowId={(row) => row.name} | ||
| getRowHeight={() => 'auto'} | ||
| autoHeight={true} | ||
| rowsPerPageOptions={[10, 25, 50]} | ||
| pageSize={25} | ||
| sortModel={[{ field: 'current_pass_percentage', sort: 'asc' }]} | ||
| disableSelectionOnClick | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Sorting is non-functional: sortModel is set but never updated.
DataGrid is given a fixed sortModel without onSortModelChange. Since sortModel makes the grid a controlled component for sorting, clicking column headers won't actually reorder rows — the prop will be reset back to the fixed value each render.
🔧 Proposed fix
+ const [sortModel, setSortModel] = React.useState([
+ { field: 'current_pass_percentage', sort: 'asc' },
+ ])
...
<DataGrid
loading={!isLoaded}
rows={rows}
columns={columns}
getRowId={(row) => row.name}
getRowHeight={() => 'auto'}
autoHeight={true}
rowsPerPageOptions={[10, 25, 50]}
pageSize={25}
- sortModel={[{ field: 'current_pass_percentage', sort: 'asc' }]}
+ sortModel={sortModel}
+ onSortModelChange={setSortModel}
disableSelectionOnClick
/>📝 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.
| <DataGrid | |
| loading={!isLoaded} | |
| rows={rows} | |
| columns={columns} | |
| getRowId={(row) => row.name} | |
| getRowHeight={() => 'auto'} | |
| autoHeight={true} | |
| rowsPerPageOptions={[10, 25, 50]} | |
| pageSize={25} | |
| sortModel={[{ field: 'current_pass_percentage', sort: 'asc' }]} | |
| disableSelectionOnClick | |
| /> | |
| const [sortModel, setSortModel] = React.useState([ | |
| { field: 'current_pass_percentage', sort: 'asc' }, | |
| ]) | |
| <DataGrid | |
| loading={!isLoaded} | |
| rows={rows} | |
| columns={columns} | |
| getRowId={(row) => row.name} | |
| getRowHeight={() => 'auto'} | |
| autoHeight={true} | |
| rowsPerPageOptions={[10, 25, 50]} | |
| pageSize={25} | |
| sortModel={sortModel} | |
| onSortModelChange={setSortModel} | |
| disableSelectionOnClick | |
| /> |
🤖 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 101 - 112, The sorting
state in DataGrid is controlled but never updated, so header clicks cannot
change the order. Update the FeatureGateDetail DataGrid setup to manage sort
state with a component state value, and wire `onSortModelChange` to persist user
changes instead of always passing the fixed `sortModel` literal. Keep the
existing default sort for initial render, but make sure `DataGrid` in
`FeatureGateDetail` reflects the current sort model on subsequent renders.
| <SimpleBreadcrumbs | ||
| release={release} | ||
| currentPage={featureGate} | ||
| crumbs={[ | ||
| { | ||
| text: 'Feature Gates', | ||
| link: `/feature_gates/${release}`, | ||
| }, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the referenced test file and breadcrumb component
git ls-files | rg 'sippy-ng/src/tests/FeatureGateDetail\.js|SimpleBreadcrumbs|Breadcrumbs' -n
# Show the relevant component definition and the usage site with line numbers
for f in $(git ls-files | rg 'SimpleBreadcrumbs|Breadcrumbs' | head -n 20); do
echo "### $f"
wc -l "$f"
done
echo
echo "### Usage site"
sed -n '180,240p' sippy-ng/src/tests/FeatureGateDetail.js
echo
echo "### Search for SimpleBreadcrumbs definitions/usages"
rg -n "function SimpleBreadcrumbs|const SimpleBreadcrumbs|class SimpleBreadcrumbs|<SimpleBreadcrumbs|previousPage=|crumbs=" sippy-ng/src -g '!**/node_modules/**'Repository: openshift/sippy
Length of output: 4379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' sippy-ng/src/components/SimpleBreadcrumbs.js
echo
echo "### Nearby examples that use the supported prop shape"
sed -n '380,420p' sippy-ng/src/jobs/JobAnalysis.js
echo
sed -n '160,190p' sippy-ng/src/tests/TestAnalysis.jsRepository: openshift/sippy
Length of output: 2619
Use previousPage for the Feature Gates breadcrumb link SimpleBreadcrumbs only renders release, previousPage, and currentPage, so crumbs is ignored and the back link never appears.
🤖 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 204 - 213,
SimpleBreadcrumbs is being passed a crumbs prop that it ignores, so the Feature
Gates back link never renders. Update the FeatureGateDetail test/setup to use
the supported previousPage prop on SimpleBreadcrumbs instead of crumbs, keeping
release and currentPage as-is so the breadcrumb can show the Feature Gates link
correctly.
|
Scheduling required tests: |
- Centralizes the logic for how to find test results for a featuregate for re-use in the openshift/api verification job. - Feature Gate API now returns hateoas links clients can follow. - Includes a link to the tests tagged for the feature gate name. - Includes a link to the synthetic "openshift-tests should work" as an indicator the job fully passed, for any jobs tied to a Capability. - Replaces the hack for featuregates containing Install to automatically route to all install results and exclude actual tests for the FG. - Adds feature gate seed data and e2e tests.
dgoodwin
force-pushed
the
fg-filter-logic
branch
from
086d074 to
246b4da
Compare
dgoodwin
changed the title
|
@dgoodwin: This pull request references TRT-2769 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 bug to target the "5.0.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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@sippy-ng/src/tests/FeatureGateDetail.js`:
- Around line 20-45: Update the TestResultsSection useEffect to call
setLoaded(true) and clear or reset the rows when apiUrl is falsy before
returning, so tabs without a specific test link finish loading and display the
empty state instead of remaining on the spinner.
🪄 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: abb599d9-a446-4314-a94a-70d9bc66ca73
📒 Files selected for processing (8)
cmd/sippy/seed_data.gopkg/apis/api/types.gopkg/sippyserver/server.gosippy-ng/src/App.jssippy-ng/src/tests/FeatureGateDetail.jssippy-ng/src/tests/FeatureGates.jstest/e2e/feature_gates_test.gotest/e2e/util/e2erequest.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/apis/api/types.go
- sippy-ng/src/tests/FeatureGates.js
- pkg/sippyserver/server.go
- test/e2e/util/e2erequest.go
- test/e2e/feature_gates_test.go
- cmd/sippy/seed_data.go
- sippy-ng/src/App.js
| function TestResultsSection({ title, apiUrl, release }) { | ||
| const [rows, setRows] = React.useState([]) | ||
| const [isLoaded, setLoaded] = React.useState(false) | ||
| const [fetchError, setFetchError] = React.useState('') | ||
|
|
||
| useEffect(() => { | ||
| if (!apiUrl) return | ||
| setLoaded(false) | ||
| setFetchError('') | ||
|
|
||
| fetch(apiUrl) | ||
| .then((response) => { | ||
| if (response.status !== 200) { | ||
| throw new Error('server returned ' + response.status) | ||
| } | ||
| return response.json() | ||
| }) | ||
| .then((json) => { | ||
| setRows(json || []) | ||
| setLoaded(true) | ||
| }) | ||
| .catch((error) => { | ||
| setFetchError('Could not retrieve ' + title + ': ' + error) | ||
| setLoaded(true) | ||
| }) | ||
| }, [apiUrl, title]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Loading state never resolves when apiUrl is empty.
If gate.links exists but the specific link (tests_by_annotation/tests_by_capability) is missing, apiUrl is falsy and the effect returns at line 26 without ever calling setLoaded(true). Since isLoaded starts false, the tab is stuck showing the loading spinner and "... tests" forever, with no indication that there's simply no data for that tab.
🐛 Proposed fix
useEffect(() => {
- if (!apiUrl) return
+ if (!apiUrl) {
+ setRows([])
+ setLoaded(true)
+ return
+ }
setLoaded(false)
setFetchError('')📝 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.
| function TestResultsSection({ title, apiUrl, release }) { | |
| const [rows, setRows] = React.useState([]) | |
| const [isLoaded, setLoaded] = React.useState(false) | |
| const [fetchError, setFetchError] = React.useState('') | |
| useEffect(() => { | |
| if (!apiUrl) return | |
| setLoaded(false) | |
| setFetchError('') | |
| fetch(apiUrl) | |
| .then((response) => { | |
| if (response.status !== 200) { | |
| throw new Error('server returned ' + response.status) | |
| } | |
| return response.json() | |
| }) | |
| .then((json) => { | |
| setRows(json || []) | |
| setLoaded(true) | |
| }) | |
| .catch((error) => { | |
| setFetchError('Could not retrieve ' + title + ': ' + error) | |
| setLoaded(true) | |
| }) | |
| }, [apiUrl, title]) | |
| function TestResultsSection({ title, apiUrl, release }) { | |
| const [rows, setRows] = React.useState([]) | |
| const [isLoaded, setLoaded] = React.useState(false) | |
| const [fetchError, setFetchError] = React.useState('') | |
| useEffect(() => { | |
| if (!apiUrl) { | |
| setRows([]) | |
| setLoaded(true) | |
| return | |
| } | |
| setLoaded(false) | |
| setFetchError('') | |
| fetch(apiUrl) | |
| .then((response) => { | |
| if (response.status !== 200) { | |
| throw new Error('server returned ' + response.status) | |
| } | |
| return response.json() | |
| }) | |
| .then((json) => { | |
| setRows(json || []) | |
| setLoaded(true) | |
| }) | |
| .catch((error) => { | |
| setFetchError('Could not retrieve ' + title + ': ' + error) | |
| setLoaded(true) | |
| }) | |
| }, [apiUrl, title]) |
🤖 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 20 - 45, Update the
TestResultsSection useEffect to call setLoaded(true) and clear or reset the rows
when apiUrl is falsy before returning, so tabs without a specific test link
finish loading and display the empty state instead of remaining on the spinner.
|
Scheduling required tests: |
|
@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
for re-use in the openshift/api verification job.
indicator the job fully passed, for any jobs tied to a Capability.
route to all install results and exclude actual tests for the FG.
Summary by CodeRabbit
New Features
Bug Fixes
Tests