Fix null Explanations causing regressed tests modal crash#3605
Fix null Explanations causing regressed tests modal crash#3605redhat-chai-bot wants to merge 1 commit into
Conversation
Initialize Explanations slice to []string{} instead of nil at both
TestComparison creation sites (component_report.go, test_details.go).
Go serializes nil slices as JSON null, which crashes the
CompSeverityIcon component when it tries to access explanations.length.
Also add a defensive != null check in CompSeverityIcon.js to guard
against any future nil serialization.
Closes: TRT-2660
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
ready-for-human-review
WalkthroughThis PR initializes the ChangesExplanations Field Initialization
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: redhat-chai-bot The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @redhat-chai-bot. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![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
🤖 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/component_readiness/CompSeverityIcon.js`:
- Around line 22-24: Add a regression test for CompSeverityIcon that verifies
the component safely handles explanations being null, undefined, and an empty
array: render the component with explanations=null, explanations=undefined, and
explanations=[], assert that rendering does not throw, that no exception occurs
in the tooltip generation code path (the logic around the explanations guard and
toolTip), and that the tooltip fallback/placeholder is shown when explanations
is falsy or empty; add the tests to the existing test suite for this component
so they fail before the guard fix and pass afterward.
🪄 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: 031c58b8-7c51-4b2d-a559-f36e69a128d5
📒 Files selected for processing (3)
pkg/api/componentreadiness/component_report.gopkg/api/componentreadiness/test_details.gosippy-ng/src/component_readiness/CompSeverityIcon.js
| if (explanations != null && explanations.length > 0) { | ||
| toolTip = explanations.join(' ') | ||
| } |
There was a problem hiding this comment.
Add a regression test for the null explanations crash path.
The Line 22 guard is correct, but this bug fix should be locked with a regression test (e.g., null, undefined, and [] explanations render without throwing and tooltip fallback still works).
As per coding guidelines, “bug fixes should include a regression test that fails without the fix.”
🤖 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/CompSeverityIcon.js` around lines 22 - 24,
Add a regression test for CompSeverityIcon that verifies the component safely
handles explanations being null, undefined, and an empty array: render the
component with explanations=null, explanations=undefined, and explanations=[],
assert that rendering does not throw, that no exception occurs in the tooltip
generation code path (the logic around the explanations guard and toolTip), and
that the tooltip fallback/placeholder is shown when explanations is falsy or
empty; add the tests to the existing test suite for this component so they fail
before the guard fix and pass afterward.
Source: Coding guidelines
1 participant
Problem
Opening the regressed tests modal on the component readiness page crashes with:
in
CompSeverityIcon.Root Cause
TestComparisonstructs are created with a nilExplanationsslice incomponent_report.goandtest_details.go. Several code paths inassessComponentStatus/buildFisherExactTestStatsnever append toExplanations(e.g. the "no regression" branch), so it stays nil. Go serializes nil slices as JSONnull.On the frontend,
CompSeverityIcon.jscheckedexplanations !== undefinedwhich does not catchnull, sonull.lengththrows a TypeError and crashes the modal.Fix
Explanations: []string{}at bothTestComparisoncreation sites so Go serializes it as[]instead ofnull.!= nullcheck inCompSeverityIcon.js(JS loose equality!= nullcatches bothnullandundefined).Files Changed
pkg/api/componentreadiness/component_report.go— initializeExplanationsto empty slicepkg/api/componentreadiness/test_details.go— initializeExplanationsto empty slicesippy-ng/src/component_readiness/CompSeverityIcon.js— use!= nullinstead of!== undefinedCloses: TRT-2660
Summary by CodeRabbit