[scanner] fix: stabilize failing coverage suite test#21313
Conversation
The test 'refetches access entries after namespace changes' was using toHaveBeenCalledWith without accounting for the second argument (AbortSignal options object). The api.get mock is called with both a URL string and an options object, so the assertion must either check both arguments or access mock.calls directly to just verify the URL. This fixes the flaky test by reverting to the approach from the previous stabilization commit that directly accesses mock.calls[0]?.[0] to check only the URL argument. Signed-off-by: Copilot <copilot@github.com>
kubestellar-prow
Bot
added
the
dco-signoff: yes
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
kubestellar-prow
Bot
added
the
size/XS
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
|
🐝 Hi @clubanderson! I'm Trusted users — org members and contributors with write access — can mention Automation may take a moment to start, and follow-up happens through workflow activity rather than chat replies. |
✅ Test Coverage CheckAll new source files in this PR have corresponding test files. Checked |
♿ Accessibility Audit (WCAG 2.1 AA)✅ No WCAG 2.1 AA violations detected in audited routes. Powered by axe-core. Target: WCAG 2.1 AA compliance. |
There was a problem hiding this comment.
Pull request overview
This PR stabilizes the NamespaceAccessPanel test that verifies access entries are refetched when the selected namespace changes, aligning the assertion with the actual api.get(url, { signal }) call signature used by the component.
Changes:
- Updates the test assertion to validate only the URL argument from
api.getmock calls, avoiding brittleness around the AbortSignal options object.
clubanderson
left a comment
clubanderson
left a comment
There was a problem hiding this comment.
[quality] 🚨 Coordination issue: this PR is a duplicate + conflicts with #21299/#21302 (comment mode; self-approve blocked)
Duplicate finding
The NamespaceAccessPanel.test.tsx fix in this PR is byte-identical to the same change already in #21299. Both PRs open the same 3-line diff on the same file:
- expect(api.get).toHaveBeenCalledWith(
- expect.stringContaining('test-namespace')
- )
+ expect(vi.mocked(api.get).mock.calls[0]?.[0]).toContain('test-namespace')Whichever merges second becomes a trivial no-op. Not harmful, but wastes a review cycle and confuses the timeline.
Bigger conflict: PR#21318 reverses PR#21299's whole strategy for the SAME failing tests
While reviewing this batch I noticed:
- #21299 (this PR's sibling) fixes
CreateNamespaceModal+CanICheckerflakes by changing tests to expect empty-default:expect(comboboxes[0].value).toBe(''), renamed test to'initializes with cluster placeholder selected', etc. - #21318 fixes the same components by changing the components to pre-select the first cluster (
useState(clusters[0] ?? '')anduseEffect(dispatch SET_FIELD/cluster)) and removing the disabled placeholder<option>.
These are incompatible strategies aimed at the same bug (#21083 vs #21297/#21298 which is the same failure mode). One or the other must be chosen — merging both will:
- If #21299 merges first: #21318 will silently revert the test alignment, so tests once again expect
.toBe('')but component sets to'cluster-1'→ 22 tests fail again. - If #21318 merges first: #21299's renamed test
'initializes with cluster placeholder selected'(asserting.toBe('')) will fail because component now pre-selects'cluster-1'.
Recommendation: pick one. My preference is #21318 (fix at the source — the UX behavior of "auto-select the only reasonable default" is better than "force a click before submit"), close #21299 (or reduce #21299 to just this single-file NamespaceAccessPanel fix, which itself duplicates this PR).
If #21318 is the winner:
- Close this PR (the NamespaceAccessPanel fix is fine but redundant with what will remain in #21299 or gets absorbed into #21318's follow-up).
- Or keep this PR as the single-file NamespaceAccessPanel change and close #21299 entirely.
On the fix itself
Correct pattern (matches what I've now seen 3 times: mock.calls[0]?.[0] when the mock receives extra args like {signal}). Prior concern still stands: the sibling toHaveBeenCalledTimes(2) + mock.calls[length-1] from #21282 is still in this file, unchanged, and would benefit from the same explicit-length + explicit-index pattern.
Bead filed. Not merging.
Filed by quality agent (ACMM L4/L6 — full mode)
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
Fixes #21308
The test 'refetches access entries after namespace changes' was using
toHaveBeenCalledWithwithout accounting for the second argument (AbortSignal options object). Theapi.getmock is called with both a URL string and an options object, so the assertion must either check both arguments or accessmock.callsdirectly to just verify the URL.This fixes the flaky test by reverting to the approach from the previous stabilization commit that directly accesses
mock.calls[0]?.[0]to check only the URL argument.