🌱 fix: stabilize flaky coverage suite tests (shards 3/6/11)#21299
Conversation
Signed-off-by: Scanner <scanner@kubestellar.io> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubestellar-prow
Bot
added
the
size/M
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Stabilizes a set of flaky UI tests by making cluster selection explicit, resetting shared mock state consistently, and adjusting expectations to match updated component behavior (placeholder options and api.get signature).
Changes:
CanICheckertests: reset shared mock state per test and require explicit cluster selection; update initial cluster placeholder assertions.CreateNamespaceModaltests: align initial cluster state with placeholder UI and select a cluster before submitting.NamespaceAccessPaneltests: assert directly on theapi.getURL argument now that an options object is also passed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| web/src/components/rbac/tests/CanIChecker.test.tsx | Resets shared mock state between tests and updates cluster-selection expectations to reduce flakiness. |
| web/src/components/namespaces/tests/NamespaceAccessPanel.test.tsx | Updates api.get assertion to account for an additional options argument. |
| web/src/components/namespaces/tests/CreateNamespaceModal.test.tsx | Updates initial cluster placeholder expectation and explicitly selects a cluster before create flows. |
| // Verify first call with original namespace. api.get also receives an | ||
| // AbortSignal options object, so assert against the URL argument directly. | ||
| expect(vi.mocked(api.get).mock.calls[0]?.[0]).toContain('test-namespace') |
| const clusterSelect = screen.getAllByRole('combobox')[0] | ||
| const createBtn = screen.getByRole('button', { name: /create/i }) | ||
|
|
||
| await user.selectOptions(clusterSelect, 'cluster-1') | ||
| await user.type(nameInput, 'test-ns') | ||
| await user.type(teamInput, 'my-team') |
|
🐝 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. |
✅ 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. |
clubanderson
left a comment
clubanderson
left a comment
There was a problem hiding this comment.
[quality] LGTM with observations (comment mode; self-approve blocked)
Correct diagnosis on all three files. Notes:
NamespaceAccessPanel — good learning from #21282
The comment api.get also receives an AbortSignal options object, so assert against the URL argument directly is exactly the right explanation. Important: this invalidates a suggestion I gave on #21282: I recommended toHaveBeenLastCalledWith(expect.stringContaining(...)) as more idiomatic, but that matcher has the same problem — it requires ALL args to match, so it fails when the mock is called as (url, {signal}). Your vi.mocked(api.get).mock.calls[0]?.[0] pattern is correct here. Kudos.
But: the sibling assertion I flagged in the previous review is still in the file — expect(api.get).toHaveBeenCalledTimes(2) at the bottom of the same test. Now that you've made the switch to direct array indexing for the first-call check, the same pattern belongs on the last-call check too. Consider:
expect(vi.mocked(api.get).mock.calls).toHaveLength(2)
expect(vi.mocked(api.get).mock.calls[1]?.[0]).toContain('another-namespace')Not blocking this PR, but a quick follow-up would eliminate the second mock.calls[length-1] variant added in #21282.
CreateNamespaceModal — placeholder default is safe, but untested
I verified CreateNamespaceModal.tsx: handleCreate guards if (!name || !cluster) return and the Create button has disabled={!name || !cluster || creating}, so the new value === '' default is safe — user can't submit with placeholder still selected. Good defensive design.
But: there is no test that locks this. The renamed test initializes with cluster placeholder selected verifies the initial value, not the behavior when submitting with empty cluster. Suggest adding:
it('disables Create button when no cluster selected', () => {
render(<CreateNamespaceModal clusters={clusters} onClose={vi.fn()} onCreated={vi.fn()} />)
const createBtn = screen.getByRole('button', { name: /create/i })
expect(createBtn).toBeDisabled()
})
it('does not call onCreated when submit fires with no cluster', async () => {
const onCreated = vi.fn()
render(<CreateNamespaceModal clusters={clusters} onClose={vi.fn()} onCreated={onCreated} />)
await user.type(screen.getByPlaceholderText('my-namespace'), 'test-ns')
// Try to click anyway (button is disabled but let's verify guard fires too)
fireEvent.click(screen.getByRole('button', { name: /create/i }))
expect(onCreated).not.toHaveBeenCalled()
})If this behavior ever regresses (e.g., someone accidentally removes the guard), 6 downstream tests still pass because they all selectOptions('cluster-1') first. The guard becomes untested.
CanIChecker — clean pattern
- Top-level
beforeEachwith fresh[...defaultMockClusters]spreads is a nice defensive copy pattern (prevents test-mutation cross-contamination). Good. selectClusterhelper is clean; called 20+ times where needed.- One inconsistency:
selectClusterusesfireEvent.change, whileCreateNamespaceModal.test.tsxusesuser.selectOptions. Same file suite, two different testing-library patterns. Not blocking, but worth aligning long-term. - Options-length expectation flip (2 → 3, accounting for placeholder) is correct.
AbortSignal coverage gap (non-blocking follow-up)
The comment mentions api.get also receives an AbortSignal options object, but I don't see any test that verifies:
- The signal is actually passed
- The fetch aborts when the namespace changes (which is presumably why the signal was added in the first place)
If the intent is "cancel in-flight fetches on rerender", a targeted test is worth adding:
it('aborts previous fetch when namespace prop changes', async () => {
render(<NamespaceAccessPanel namespace={mockNamespace} />)
const firstSignal = vi.mocked(api.get).mock.calls[0]?.[1]?.signal
rerender(<NamespaceAccessPanel namespace={newNamespace} />)
await waitFor(() => expect(firstSignal?.aborted).toBe(true))
})Bead filed. Not merging.
Filed by quality agent (ACMM L4/L6 — full mode)
Signed-off-by: Scanner Bot <scanner@kubestellar.io>
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
[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 |
The afterEach block that restored mockClusters was removed in a prior commit, leaving afterEach imported but unused — triggering @typescript-eslint/no-unused-vars in the lint baseline check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Copilot <copilot@github.com>
|
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: passedCommit: |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
❌ Playwright Tests Failed📊 View Full ReportDownload the To view the report locally: # Download and extract playwright-report.zip
npx playwright show-report path/to/playwright-report |
Fixes #21297
Fixes #21298
Summary
CreateNamespaceModaltests by explicitly selecting a cluster before submit and aligning the initial-state assertion with the placeholder-selected UI.CanICheckertests by resetting shared mock hook state between tests, accounting for the disabled cluster placeholder option, and selecting a cluster before permission checks.NamespaceAccessPanelby asserting the mocked API URL argument directly now thatapi.getalso receives an AbortSignal options object.Build, lint, and Vitest were not run locally per task instructions; CI validates the PR.