🌱 fix: coverage suite test failure#21282
Conversation
Signed-off-by: GitHub Copilot <223556219+Copilot@users.noreply.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. |
|
[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 |
|
🐝 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. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
kubestellar-prow
Bot
added
the
size/S
✅ 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 updates the NamespaceAccessPanel unit test to correctly validate that changing the namespace prop triggers a second access fetch (fixing a Coverage Suite failure for this test).
Changes:
- Adds explicit verification that
api.getis invoked again after rerendering with a different namespace. - Adds assertions intended to distinguish the initial fetch (original namespace) from the subsequent fetch (new namespace).
| // Verify first call with original namespace | ||
| expect(api.get).toHaveBeenCalledWith( | ||
| expect.stringContaining('test-namespace') | ||
| ) | ||
|
|
clubanderson
left a comment
clubanderson
left a comment
There was a problem hiding this comment.
[quality] Review — assertions are better but the PR body misdiagnoses the root cause (comment mode; self-approve blocked)
The new assertions are strictly more informative than the old ones — no complaint about the direction. But the "Root Cause" section is wrong in a way worth calling out because it could lead to the same class of mis-fix on the next flaky test.
Root cause claim is incorrect
The PR body says:
The test was using
toHaveBeenCalledWithwhich checks if ANY call matches, but didn't verify that a SECOND call was made after the namespace changed. This could lead to false positives if the test passed based on the first call alone.
The first call used 'test-namespace' (line 290: getByText('alice') waits for that render). The assertion under waitFor was expect(api.get).toHaveBeenCalledWith(expect.stringContaining('another-namespace')). The first call cannot make that assertion pass — 'test-namespace' does not contain 'another-namespace'. If the component didn't refetch, waitFor would time out, not silently pass.
So the old test was not a false-positive on refetch. Whatever #21280 actually caught, it wasn't "the assertion passed via the first call." More likely candidates:
- A race between the initial render's fetch and the
rerender()—waitForfinds the 'another-namespace' call before the initial has fully settled, and something else the test doesn't assert on is broken. - The component actually stopped refetching (regression) and
waitFortimed out — in which case the old test was correctly catching the bug, and the new test does too. - Flake from Vitest worker-parallelism (see #21083 / #21283) unrelated to the assertion shape.
Concrete concerns with the new assertions
-
toHaveBeenCalledTimes(2)is stricter than before. IfNamespaceAccessPanelmakes any otherapi.getcall (bindings prefetch, RBAC lookup, cluster-role list, feature flag), or React StrictMode double-invokes an effect in test env, this test flakes for reasons unrelated to the fix. Quick check:rg -n "api.get\(" web/src/components/namespaces/NamespaceAccessPanel.tsx— if there's more than one call per render, this assertion is fragile. -
vi.mocked(api.get).mock.calls[length - 1]is verbose. Idiomatic Vitest:expect(api.get).toHaveBeenLastCalledWith(expect.stringContaining('another-namespace'))
Same guarantee, tolerates future refactors, no
length-1off-by-one risk. -
No
afterEach(() => vi.clearAllMocks())visible in the diff. If a prior test in this file calledapi.get, the call-count assertion will fail. Worth checking the top of the file.
Non-blocking suggestion — assert on the arg
The last-call check verifies the URL substring contains another-namespace but doesn't verify the fetch shape (GET /api/v1/namespaces/another-namespace/access or whatever). If a future refactor changes the URL template, this test still passes on a broken call. Consider asserting the full URL pattern.
Recommendation
- Fix the "Root Cause" section in the PR body — currently misleading.
- Swap
mock.calls[length-1]fortoHaveBeenLastCalledWith. - Verify
api.getisn't called more than twice per test lifecycle (or droptoHaveBeenCalledTimes(2)in favor oftoHaveBeenNthCalledWith(2, ...)).
Bead filed. Not merging.
Filed by quality agent (ACMM L4/L6 — full mode)
|
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 Verification: cancelledCommit: |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
Fixes #21280
Summary
Fixed failing test in
NamespaceAccessPanel.test.tsxthat was checking if the component refetches access entries after namespace changes.Root Cause
The test was using
toHaveBeenCalledWithwhich checks if ANY call matches, but didn't verify that a SECOND call was made after the namespace changed. This could lead to false positives if the test passed based on the first call alone.Changes
api.getis called twice (once for each namespace)Testing
The test now properly verifies: