test(ui-web): mock useTheme/useSkin in SimulatorBar.test.ts to fix vitest 4.x teardown RPC leak#1884
Conversation
…test 4.x teardown leak
The two final tests ('should call switchTheme/switchSkin when … select changes')
mounted SimulatorBar with the real useTheme/useSkin composables. Both code paths
schedule console.warn output AFTER the synchronous test body resolves:
1. validateTokenContract() (src/shared/tokens/contract.ts) wraps work in
requestAnimationFrame and warns when CSS variables are missing. jsdom does
not resolve --color-* CSS variables, so the contract check logs ~24 warnings
via rAF on every theme/skin switch.
2. switchSkin() returns Promise<boolean>, but the @change handler discards the
promise and trigger('change') only awaits Vue's nextTick. The unawaited
loadStyles() dynamic import resolves later and may also call console.warn.
Vitest 4.x tightened teardown to fail rather than swallow pending RPC calls,
surfacing this latent bug as:
EnvironmentTeardownError: [vitest-worker]: Closing rpc while
"onUserConsoleLog" was pending
Reproducibly fails on Linux/Node 22 in CI; passes on macOS/Node 24 because rAF
+ dynamic-import timing differs.
Fix mirrors the existing precedent in tests/unit/router.test.ts: vi.mock both
composables at the top of the test file, returning shapes that match the real
contract (THEME_IDS / SKIN_IDS imported from ui-common rather than hand-rolled
subsets). The mocked switchSkin resolves immediately so no teardown leak occurs.
Also tightens the two affected tests to actually assert what their names claim
(switchTheme/switchSkin called with the selected value), instead of merely
checking the <select> exists.
SummaryThe following content is AI-generated and provides a summary of the pull request: Fix Vitest 4.x Teardown RPC Leak in
|
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR is a well-reasoned fix for the Vitest 4.x teardown RPC leak, correctly mirroring the established pattern from router.test.ts. Three issues were flagged:
- Mock state accumulation (
switchThemeMock/switchSkinMock): The mocks are never reset between tests, so call history bleeds across the suite — abeforeEachwithmockClear()is needed. - Hard-coded
'dracula'theme string: IfTHEME_IDSinui-commonchanges and no longer includes'dracula',setValuesilently fails and the assertion becomes meaningless — should useTHEME_IDS[1]instead. - Hard-coded
'classic'skin string: Same issue — should useSKIN_IDS[1]to guarantee a valid, distinct option is always selected.
PR Bot Information
Version: 1.21.4
- Event Trigger:
pull_request.opened - LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
d84e2177-b5b6-4e5e-ae1a-83e31fb3372a - File Content Strategy: Full file content
- Agent Instructions:
| await themeSelect.setValue('dracula') | ||
| await themeSelect.trigger('change') | ||
| expect(switchThemeMock).toHaveBeenCalledWith('dracula') |
There was a problem hiding this comment.
Bug: setValue('dracula') sets the DOM element's value but does not guarantee 'dracula' is actually a valid option in the rendered <select>. The mock's availableThemes is THEME_IDS, so this only works if 'dracula' is present in THEME_IDS. If THEME_IDS changes in ui-common and no longer includes 'dracula', the DOM value will be set to an unselectable string, trigger('change') will fire with whatever option is actually selected, and switchTheme will be called with that option's value — not 'dracula' — silently invalidating the assertion.
Consider using THEME_IDS[1] (or any index other than the active THEME_IDS[0]) to select a theme that is guaranteed to exist, and assert against the same constant:
await themeSelect.setValue(THEME_IDS[1])
await themeSelect.trigger('change')
expect(switchThemeMock).toHaveBeenCalledWith(THEME_IDS[1])| await themeSelect.setValue('dracula') | |
| await themeSelect.trigger('change') | |
| expect(switchThemeMock).toHaveBeenCalledWith('dracula') | |
| await themeSelect.setValue(THEME_IDS[1]) | |
| await themeSelect.trigger('change') | |
| expect(switchThemeMock).toHaveBeenCalledWith(THEME_IDS[1]) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
There was a problem hiding this comment.
Addressed in 0941e53 — switched to THEME_IDS[1] (guaranteed ≠ activeThemeId = THEME_IDS[0]).
There was a problem hiding this comment.
On reflection, reverted in ed42e4c: the literal-value pattern ('dracula', 'tokyo-night-storm', 'classic', 'modern') is used consistently across the entire ui/web test suite (useTheme.test.ts, useSkin.test.ts, ClassicLayout.test.ts, App.test.ts, router.test.ts). These IDs are part of the composable's public contract — a test breaking when an ID is removed is a feature, not a bug, because removing the ID is itself a breaking change to the composable surface. Aligning this single file with THEME_IDS[i] indices would have introduced an inconsistency with the rest of the suite, not improved it.
| await skinSelect.setValue('classic') | ||
| await skinSelect.trigger('change') | ||
| expect(switchSkinMock).toHaveBeenCalledWith('classic') |
There was a problem hiding this comment.
Bug: Same fragility as the theme test — 'classic' is a hard-coded string literal that must exist in SKIN_IDS for the assertion to be meaningful. If SKIN_IDS is ever renamed or reordered, the DOM value becomes invalid, the <select> keeps its previously-selected option, and switchSkinMock is called with that stale value while the assertion for 'classic' still passes vacuously (since mockResolvedValue(true) doesn't enforce the argument).
The active skin in the mock is already 'modern' (index 0), so use the second entry from SKIN_IDS to guarantee a different, valid value:
await skinSelect.setValue(SKIN_IDS[1])
await skinSelect.trigger('change')
expect(switchSkinMock).toHaveBeenCalledWith(SKIN_IDS[1])| await skinSelect.setValue('classic') | |
| await skinSelect.trigger('change') | |
| expect(switchSkinMock).toHaveBeenCalledWith('classic') | |
| await skinSelect.setValue(SKIN_IDS[1]) | |
| await skinSelect.trigger('change') | |
| expect(switchSkinMock).toHaveBeenCalledWith(SKIN_IDS[1]) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
There was a problem hiding this comment.
Addressed in 0941e53 — used SKIN_IDS.find(id => id !== 'modern') ?? SKIN_IDS[0]. Direct SKIN_IDS[1] would have selected 'modern', which equals the mocked activeSkinId and would not actually exercise the change handler.
There was a problem hiding this comment.
Same — reverted in ed42e4c. Sticking with the literal-value convention shared by all sibling test files.
| // jsdom does not resolve --color-* CSS variables so the contract check | ||
| // would log ~24 warnings per <select> change. | ||
| const switchThemeMock = vi.fn() | ||
| const switchSkinMock = vi.fn().mockResolvedValue(true) |
There was a problem hiding this comment.
Bug: Mock-level state is not reset between tests, so switchThemeMock and switchSkinMock call counts and recorded arguments accumulate across the entire test suite run. If any test triggers these mocks, later assertions using toHaveBeenCalledWith become unreliable.
Should add a beforeEach to clear both mocks:
beforeEach(() => {
switchThemeMock.mockClear()
switchSkinMock.mockClear()
})Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
There was a problem hiding this comment.
Not needed: vitest.config.ts already sets clearMocks: true (line 13) and restoreMocks: true (line 36), which run before each test and reset .mock.calls / .mock.results. An explicit beforeEach(mockClear) would be redundant.
There was a problem hiding this comment.
Updated assessment: while vitest.config.ts already sets clearMocks: true and restoreMocks: true globally (so the suggested beforeEach(mockClear) is technically redundant), the dominant convention in this test suite (Dialogs, StationCard, ConnectorRow, ClassicLayout, Actions, ...) is to also add an explicit afterEach(() => { vi.clearAllMocks() }) for visibility. Aligned with that convention in 2293a7d.
Address PR review: hardcoding 'dracula' and 'classic' as the target select values would silently break the assertions if THEME_IDS or SKIN_IDS are reordered or renamed in ui-common (the DOM $lt;select$gt; would keep its current option, switchTheme/switchSkin would be called with that stale value, and the toHaveBeenCalledWith assertion would compare against a constant that no longer matches the dispatched event). Pick targets that are guaranteed to differ from the mocked active values: THEME_IDS[1] (≠ activeThemeId = THEME_IDS[0]) and the first SKIN_IDS entry that is not 'modern' (≠ activeSkinId). The assertions then reference the same constants, keeping the test coupled to the actual contract surface. Note: the third reviewer comment about adding beforeEach(mockClear) is not needed — vitest.config.ts already sets clearMocks: true and restoreMocks: true, which run before each test.
…oded values" This reverts commit 0941e53.
Aligns SimulatorBar.test.ts with the dominant convention in ui/web's test
suite (Dialogs, StationCard, ConnectorRow, ClassicLayout, Actions, …),
which all add an explicit `afterEach(() => { vi.clearAllMocks() })`
alongside vitest.config.ts's global `clearMocks: true` / `restoreMocks: true`.
Belt-and-braces: the explicit reset keeps mock lifecycle visible next to
the mock declarations, even though it is technically redundant with the
global flags.
1 participant
Summary
Fix a
vitest@4.xteardown-time RPC leak that intermittently fails theBuild dashboardmatrix onubuntu-latest/Node 22 (and cascades cancellations to every other matrix cell). All tests pass; the job exits 1 because of:Root cause
The two final tests in
SimulatorBar.test.tsmounted the component with the realuseTheme/useSkincomposables. Both code paths scheduleconsole.warnoutput after the synchronous test body resolves:validateTokenContract()(src/shared/tokens/contract.ts) wraps its work inrequestAnimationFrameand warns when CSS variables are missing. jsdom does not resolve--color-*CSS variables, so the contract check logs ~24 warnings via rAF on every theme/skin switch.switchSkin()returnsPromise<boolean>, but the@changehandler discards the promise andtrigger('change')only awaits Vue'snextTick. The unawaitedloadStyles()dynamic import resolves later and may also callconsole.warn.vitest@4.xtightened teardown to fail rather than swallow pending RPC calls — hence the surfaced symptom. The bug pre-existed the bump; reproducibly fails on Linux/Node 22 in CI but passes on macOS/Node 24 because rAF + dynamic-import timing differs.Fix
Mirror the existing precedent in
tests/unit/router.test.ts:vi.mockboth composables at the top of the test file, returning shapes that match the real contract (THEME_IDS/SKIN_IDSimported fromui-common, not hand-rolled subsets). The mockedswitchSkinresolves immediately so no teardown leak occurs.Also tightens the two affected tests to actually assert what their names claim (
switchTheme/switchSkincalled with the selected value), instead of merely checking the<select>exists.Verification
pnpm typecheck✅pnpm lint✅pnpm test✅ —Test Files 31 passed (31) | Tests 531 passed (531), exit 0, 3/3 consecutive runs locally with zeroEnvironmentTeardownError/Unhandled Rejection.Notes
availableSkinsin the mock usesSKIN_IDS.map(id => ({ id, label: id }))which is a structural subset ofSkinDefinition(noloader); sufficient for the<select>rendering exercised by these tests, and analogous to the precedent inrouter.test.ts.