Trt 2782 all tests details links UI#3763
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: 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 |
openshift-ci
Bot
added
the
approved
WalkthroughThe component readiness API now includes ChangesAll-tests HATEOAS links
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ComponentReport
participant LinkInjector
participant FrontendGrid
ComponentReport->>LinkInjector: process all test comparisons
LinkInjector-->>ComponentReport: attach test_details links
ComponentReport-->>FrontendGrid: return all_tests summaries
FrontendGrid->>FrontendGrid: generate UI link from test HATEOAS data
FrontendGrid-->>FrontendGrid: render clickable test cell
🚥 Pre-merge checks | ✅ 19 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (19 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
sippy-ng/src/component_readiness/TriagedRegressionTestList.js (1)
245-275: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnguarded
hrefwhengenerateTestDetailsReportLinkreturnsnull.Per the simplification described in the plan doc,
generateTestDetailsReportLinknow returnsnullwhen no HATEOAS link is present. Here,item.urlis used directly ashrefwithout a null check (<a href={item.url} ...>), unlikeCompReadyTestCell.js, which guards with a ternary (link ? <a href={link}>... : ...). If a triaged regression'sTestComparisonlacks atest_detailslink, the icon renders inside a non-functional anchor that still looks clickable.🔧 Proposed guard
- <a href={item.url} target="_blank" rel="noopener noreferrer"> - <CompSeverityIcon - status={item.status} - explanations={item.explanations} - /> - </a> + {item.url ? ( + <a href={item.url} target="_blank" rel="noopener noreferrer"> + <CompSeverityIcon + status={item.status} + explanations={item.explanations} + /> + </a> + ) : ( + <CompSeverityIcon + status={item.status} + explanations={item.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 `@sippy-ng/src/component_readiness/TriagedRegressionTestList.js` around lines 245 - 275, Update the renderCell logic for the triaged regression status in TriagedRegressionTestList so a null result from generateTestDetailsReportLink does not render the icon inside an anchor with an invalid href. Mirror the guarded link behavior used by CompReadyTestCell.js: render the anchor only when item.url is present, and render the same icon without a link otherwise.pkg/api/componentreadiness/component_report.go (1)
125-159: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winAvoid double post-analysis for regressed tests
AllTestsalready includes the regressed entries, so those cells run throughPostAnalysistwice. Skip the overlap in the second loop or derive one slice from the other to avoid redundant middleware work.🤖 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/component_report.go` around lines 125 - 159, The PostAnalysis method currently processes regressed tests twice because AllTests includes those entries. Update the AllTests loop in ComponentReportGenerator.PostAnalysis to skip entries already represented in RegressedTests, or otherwise derive the non-regressed subset, while preserving post-analysis for tests that are only in AllTests.sippy-ng/src/component_readiness/RegressedTestsPanel.js (1)
285-313: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard against
nulllink fromgenerateTestDetailsReportLink.
generateTestDetailsReportLinkcan now returnnull(no HATEOAS link found), but the anchor here is rendered unconditionally. UnlikeCompReadyTestCell.js, which conditionally wraps the icon only when a link exists, this will silently render a non-functional<a>with nohref.🐛 Proposed fix to guard the link
- <a - href={generateTestDetailsReportLink(params.row)} - target="_blank" - rel="noopener noreferrer" - > - <CompSeverityIcon - status={ - params.row.effective_status - ? params.row.effective_status - : params.row.status - } - explanations={params.row.explanations} - /> - </a> + {(() => { + const link = generateTestDetailsReportLink(params.row) + const icon = ( + <CompSeverityIcon + status={ + params.row.effective_status + ? params.row.effective_status + : params.row.status + } + explanations={params.row.explanations} + /> + ) + return link ? ( + <a href={link} target="_blank" rel="noopener noreferrer"> + {icon} + </a> + ) : ( + icon + ) + })()}🤖 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/RegressedTestsPanel.js` around lines 285 - 313, Update the status column’s renderCell callback to store the result of generateTestDetailsReportLink(params.row) and render the anchor only when that link is non-null; otherwise render CompSeverityIcon directly while preserving its existing status and explanations props.sippy-ng/src/component_readiness/TriagePotentialMatches.js (1)
352-379: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard against
nullURL in the status cell.Since
generateTestDetailsReportLinkcan returnnullwhen no HATEOAS link exists,testDetailsUrlmay benulland the anchor at Line 371 renders with nohref, leaving a non-functional link around the icon. Consider conditionally rendering the anchor only whenurlis truthy, mirroring the pattern inCompReadyTestCell.js.🐛 Proposed fix to guard the link
renderCell: (params) => ( <div className={classes.statusCell}> - <a href={params.value.url} target="_blank" rel="noopener noreferrer"> - <CompSeverityIcon - status={params.value.status} - explanations={params.value.explanations} - /> - </a> + {params.value.url ? ( + <a href={params.value.url} target="_blank" rel="noopener noreferrer"> + <CompSeverityIcon + status={params.value.status} + explanations={params.value.explanations} + /> + </a> + ) : ( + <CompSeverityIcon + status={params.value.status} + explanations={params.value.explanations} + /> + )} </div> ),🤖 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/TriagePotentialMatches.js` around lines 352 - 379, Update the status cell’s renderCell logic in TriagePotentialMatches to render the anchor only when params.value.url is truthy, while always rendering CompSeverityIcon. Preserve the existing link attributes and icon status/explanations handling when a URL is available, mirroring the conditional-link pattern used by CompReadyTestCell.
🧹 Nitpick comments (2)
pkg/api/componentreadiness/component_report_test.go (1)
1220-1227: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertion doesn't actually validate "populated" for most cases.
The comment claims this validates that
AllTests"is populated," butassert.GreaterOrEqual(len(AllTests), len(RegressedTests))is trivially true (>= 0) wheneverRegressedTestsis empty. Most test cases here (e.g.NotSignificant,SignificantImprovement,MissingBasisAndSamplestatuses) have zeroRegressedTests, so the assertion doesn't catch a backend regression whereAllTestsis left empty for those columns, even though the corresponding e2e test (alltests_links_test.go) expectsAllTeststo be populated with links forNotSignificant/SignificantImprovement/MissingBasisstatuses.Consider asserting
AllTestsis non-empty whenever the column status isn'tMissingBasisAndSample(i.e., there is real base/sample data), to actually cover the population claim.♻️ Suggested stronger assertion
// AllTests should contain at least as many entries as RegressedTests assert.GreaterOrEqual(t, len(report.Rows[ir].Columns[ic].AllTests), len(report.Rows[ir].Columns[ic].RegressedTests), "AllTests should be a superset of RegressedTests for row %d col %d", ir, ic) + // For columns with real base/sample data, AllTests should be populated. + if report.Rows[ir].Columns[ic].Status != crtest.MissingBasisAndSample { + assert.NotEmpty(t, report.Rows[ir].Columns[ic].AllTests, + "AllTests should be populated for row %d col %d with status %d", + ir, ic, report.Rows[ir].Columns[ic].Status) + } // Clear AllTests for the deep comparison below; the count check above // validates it is populated. report.Rows[ir].Columns[ic].AllTests = nil🤖 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/component_report_test.go` around lines 1220 - 1227, Strengthen the AllTests validation in the report comparison loop so columns whose status is not MissingBasisAndSample require a non-empty AllTests collection, while retaining the existing superset check against RegressedTests. Keep the exception for MissingBasisAndSample and continue clearing AllTests only after both validations.test/e2e/componentreadiness/alltests_links_test.go (1)
26-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBoth key assertions can be skipped depending on live view data.
The two subtests that actually verify
test_detailslinks exist (regressed tests have test_details links in AllTestsandnon-regressed tests have test_details links in AllTests) botht.Skipif the fetched view happens to lack matching rows. Since the view used isviews[0]from whatever the live environment returns, these core assertions may run without ever executing in some environments/CI runs, reducing this e2e test's regression-catching value.Consider selecting/asserting a view known to contain both regressed and non-regressed test data (or failing loudly if no suitable view is found) rather than silently skipping the primary checks.
Also applies to: 59-86
🤖 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/componentreadiness/alltests_links_test.go` around lines 26 - 57, The link assertions in the subtests “regressed tests have test_details links in AllTests” and “non-regressed tests have test_details links in AllTests” can be silently skipped when the selected live view lacks matching rows. Update the view selection or setup to use a view containing both regressed and non-regressed data, and fail the test when no suitable view exists instead of calling t.Skip; keep the existing test_details link assertions unchanged.
🤖 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.
Outside diff comments:
In `@pkg/api/componentreadiness/component_report.go`:
- Around line 125-159: The PostAnalysis method currently processes regressed
tests twice because AllTests includes those entries. Update the AllTests loop in
ComponentReportGenerator.PostAnalysis to skip entries already represented in
RegressedTests, or otherwise derive the non-regressed subset, while preserving
post-analysis for tests that are only in AllTests.
In `@sippy-ng/src/component_readiness/RegressedTestsPanel.js`:
- Around line 285-313: Update the status column’s renderCell callback to store
the result of generateTestDetailsReportLink(params.row) and render the anchor
only when that link is non-null; otherwise render CompSeverityIcon directly
while preserving its existing status and explanations props.
In `@sippy-ng/src/component_readiness/TriagedRegressionTestList.js`:
- Around line 245-275: Update the renderCell logic for the triaged regression
status in TriagedRegressionTestList so a null result from
generateTestDetailsReportLink does not render the icon inside an anchor with an
invalid href. Mirror the guarded link behavior used by CompReadyTestCell.js:
render the anchor only when item.url is present, and render the same icon
without a link otherwise.
In `@sippy-ng/src/component_readiness/TriagePotentialMatches.js`:
- Around line 352-379: Update the status cell’s renderCell logic in
TriagePotentialMatches to render the anchor only when params.value.url is
truthy, while always rendering CompSeverityIcon. Preserve the existing link
attributes and icon status/explanations handling when a URL is available,
mirroring the conditional-link pattern used by CompReadyTestCell.
---
Nitpick comments:
In `@pkg/api/componentreadiness/component_report_test.go`:
- Around line 1220-1227: Strengthen the AllTests validation in the report
comparison loop so columns whose status is not MissingBasisAndSample require a
non-empty AllTests collection, while retaining the existing superset check
against RegressedTests. Keep the exception for MissingBasisAndSample and
continue clearing AllTests only after both validations.
In `@test/e2e/componentreadiness/alltests_links_test.go`:
- Around line 26-57: The link assertions in the subtests “regressed tests have
test_details links in AllTests” and “non-regressed tests have test_details links
in AllTests” can be silently skipped when the selected live view lacks matching
rows. Update the view selection or setup to use a view containing both regressed
and non-regressed data, and fail the test when no suitable view exists instead
of calling t.Skip; keep the existing test_details link assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 77ccaa38-f04f-47bd-8e1b-f0035fecb88f
📒 Files selected for processing (22)
docs/plans/trt-2782-all-tests-links.mddocs/plans/trt-2782-frontend-hateoas-links.mdpkg/api/componentreadiness/component_report.gopkg/api/componentreadiness/component_report_test.gopkg/api/componentreadiness/middleware/linkinjector/linkinjector.gopkg/apis/api/componentreport/types.gosippy-ng/src/component_readiness/CompCapTestRow.jssippy-ng/src/component_readiness/CompReadyEnvCapabilities.jssippy-ng/src/component_readiness/CompReadyEnvCapability.jssippy-ng/src/component_readiness/CompReadyEnvCapabilityTest.jssippy-ng/src/component_readiness/CompReadyTestCell.jssippy-ng/src/component_readiness/CompReadyUtils.jssippy-ng/src/component_readiness/CompReadyUtils.test.jssippy-ng/src/component_readiness/ComponentReadiness.jssippy-ng/src/component_readiness/ComponentReadinessToolBar.jssippy-ng/src/component_readiness/RegressedTestsModal.jssippy-ng/src/component_readiness/RegressedTestsPanel.jssippy-ng/src/component_readiness/Triage.jssippy-ng/src/component_readiness/TriagePotentialMatches.jssippy-ng/src/component_readiness/TriagedRegressionTestList.jssippy-ng/src/component_readiness/TriagedTestsPanel.jstest/e2e/componentreadiness/alltests_links_test.go
💤 Files with no reviewable changes (7)
- sippy-ng/src/component_readiness/CompReadyEnvCapabilities.js
- sippy-ng/src/component_readiness/ComponentReadiness.js
- sippy-ng/src/component_readiness/CompReadyEnvCapability.js
- sippy-ng/src/component_readiness/TriagedTestsPanel.js
- sippy-ng/src/component_readiness/Triage.js
- pkg/api/componentreadiness/middleware/linkinjector/linkinjector.go
- sippy-ng/src/component_readiness/RegressedTestsModal.js
openshift-ci
Bot
added
the
ready-for-human-review
|
@neisw: The following tests failed, say
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. |
1 participant
UI implementation of TRT-2782. Relies on #3761
Allows navigation to non regressed test details:
Continues to support navigation to regressed tests:
Summary by CodeRabbit
New Features
Documentation
Tests