Trt 2782 all tests details links UI#3763
Open
neisw wants to merge 5 commits into
Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
Contributor
|
[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
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 (17)
💤 Files with no reviewable changes (6)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughThe component readiness API now exposes Level 4 ChangesAll-tests HATEOAS navigation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ComponentReportGenerator
participant LinkInjector
participant FrontendGrid
ComponentReportGenerator->>ComponentReportGenerator: collect Level 4 all_tests
ComponentReportGenerator->>LinkInjector: process each test comparison
LinkInjector-->>ComponentReportGenerator: attach test_details links
ComponentReportGenerator-->>FrontendGrid: return all_tests summaries
FrontendGrid->>FrontendGrid: convert HATEOAS link to UI URL
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}
Contributor
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
Contributor
Author
|
/retest-required |
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
|
Scheduling required tests: |
neisw
force-pushed
the
trt-2782-all-tests-details-links-ui
branch
from
July 14, 2026 00:42
e6b3762 to
c95f5b6
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/componentreadiness/alltests_links_test.go (1)
53-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "RegressedTests unchanged by AllTests addition" subtest.
This subtest is byte-for-byte identical to the one at lines 216-226 in
TestAllTestsLinksPresent. Extract a shared helper to avoid drift between the two copies.♻️ Proposed helper extraction
func assertRegressedTestsUnchanged(t *testing.T, rows []componentreport.ReportRow) { for _, row := range rows { for _, col := range row.Columns { for _, rt := range col.RegressedTests { assert.Less(t, int(rt.ReportStatus), int(crtest.MissingSample), "RegressedTests should only contain tests with regression status, got %d for %s", rt.ReportStatus, rt.TestName) } } } }- t.Run("RegressedTests unchanged by AllTests addition", func(t *testing.T) { - for _, row := range report.Rows { - for _, col := range row.Columns { - for _, rt := range col.RegressedTests { - assert.Less(t, int(rt.ReportStatus), int(crtest.MissingSample), - "RegressedTests should only contain tests with regression status, got %d for %s", - rt.ReportStatus, rt.TestName) - } - } - } - }) + t.Run("RegressedTests unchanged by AllTests addition", func(t *testing.T) { + assertRegressedTestsUnchanged(t, report.Rows) + })Also applies to: 216-226
🤖 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 53 - 63, Extract the duplicated “RegressedTests unchanged by AllTests addition” assertion into a shared assertRegressedTestsUnchanged helper, accepting the report rows and testing context. Replace both copies in TestAllTestsLinksPresent with calls to this helper while preserving the existing assertion and subtest behavior.
🤖 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 `@test/e2e/componentreadiness/alltests_links_test.go`:
- Around line 53-63: Extract the duplicated “RegressedTests unchanged by
AllTests addition” assertion into a shared assertRegressedTestsUnchanged helper,
accepting the report rows and testing context. Replace both copies in
TestAllTestsLinksPresent with calls to this helper while preserving the existing
assertion and subtest behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: dd7da4da-9316-4774-82f5-b17e772dc726
📒 Files selected for processing (20)
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.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 (6)
- sippy-ng/src/component_readiness/TriagedTestsPanel.js
- sippy-ng/src/component_readiness/Triage.js
- sippy-ng/src/component_readiness/ComponentReadiness.js
- sippy-ng/src/component_readiness/CompReadyEnvCapability.js
- sippy-ng/src/component_readiness/CompReadyEnvCapabilities.js
- sippy-ng/src/component_readiness/RegressedTestsModal.js
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/api/componentreadiness/component_report_test.go
- sippy-ng/src/component_readiness/TriagePotentialMatches.js
- sippy-ng/src/component_readiness/CompReadyEnvCapabilityTest.js
- sippy-ng/src/component_readiness/CompCapTestRow.js
- sippy-ng/src/component_readiness/CompReadyUtils.test.js
- docs/plans/trt-2782-all-tests-links.md
- sippy-ng/src/component_readiness/CompReadyTestCell.js
- sippy-ng/src/component_readiness/RegressedTestsPanel.js
- sippy-ng/src/component_readiness/TriagedRegressionTestList.js
- sippy-ng/src/component_readiness/ComponentReadinessToolBar.js
- pkg/api/componentreadiness/component_report.go
- sippy-ng/src/component_readiness/CompReadyUtils.js
- docs/plans/trt-2782-frontend-hateoas-links.md
neisw
force-pushed
the
trt-2782-all-tests-details-links-ui
branch
from
July 14, 2026 16:06
c95f5b6 to
735c6e1
Compare
Contributor
|
Scheduling required tests: |
Contributor
|
@neisw: 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.
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