test(pwa): migrate keyboard-shortcut logic from e2e to unit tests#247
Merged
Conversation
…low-up) First migration under the unit-first test-placement policy. The keyboard shortcuts e2e paid for a full WebGL image load in every test just to assert that a keypress flips a signal - pure handler logic that needs no GPU. - Add useKeyboardShortcuts.test.tsx: mounts the hook and dispatches real KeyboardEvents in jsdom, asserting every key -> handler mapping (1-6, v, c, arrows, r, i, b, x, m, z, s, u, Shift+U/D, Ctrl+L, Ctrl+Shift+O/H), exactly-one routing (modifier disambiguation, e.g. u vs Shift+U), the input/textarea/ contentEditable focus guard, the enabled flag, listener cleanup on unmount, and missing-handler no-ops. The test bodies run in ~80ms. - Extend keyboardShortcuts.test.ts: formatShortcut, plus a real-registry disambiguation check for the shared "u" key. - Slim keyboard-shortcuts.spec.ts from 16 WebGL-loading specs to a single end-to-end smoke: a real keypress drives sliceType, and focus in a text input suppresses it. That is the one claim a unit test cannot make (the hook is actually mounted and wired in the running app); the logic itself is now covered far faster and more precisely by the unit tests above. Net: ~15 real WebGL loads removed from the serial e2e lane; comprehensive keyboard coverage moved to deterministic millisecond-scale unit tests. Verified: @niivue/react 120 unit tests pass; lint + type-check green; the e2e smoke passes on SwiftShader. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
🚀 PWA Preview DeploymentYour PWA preview has been deployed! Preview URL: https://niivue.github.io/niivue-vscode/pr-247/ This preview will be updated automatically when you push new commits to this PR. |
Contributor
Coverage ReportOverall line coverage: 40.7% (+2.2) vs
|
…-up) Addresses critical-review feedback on the keyboard migration: - The hook test asserts key->handler, and the e2e smoke only checked 1->AXIAL, so the handler->value wiring (4->RENDER, 5->MULTIPLANAR, the cycleViewMode %5 wrap, cycleUIVisibility 3->2->0->3) was asserted nowhere. Add Menu.test.tsx cases that dispatch keydowns at a rendered <Menu> and assert the resulting sliceType / hideUI values - no WebGL load. - Fix an off-by-one in the smoke comment (the replaced matrix was 15 specs). @niivue/react: 123 unit tests pass; type-check green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve the conflict in apps/pwa/tests/keyboard-shortcuts.spec.ts. Keep the e2e->unit migration (the single wiring smoke test) and preserve #242's #224 clip-plane regression as an integration test: that NiiVue's built-in clipPlaneHotKey is disabled so one 'c' press is one step is a claim the jsdom hook unit test cannot make (it has no real NiiVue canvas). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
🧹 PWA Preview CleanupThe preview deployment for this PR has been removed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First migration under the unit-first test-placement policy from #241 (and documented in #245). The keyboard-shortcuts e2e was the clearest offender: 16 specs, each paying for a full WebGL image load in
beforeEach, just to assert that a keypress flips a signal - pure handler logic that needs no GPU.What moved
To unit tests (
packages/niivue-react/src/test/):useKeyboardShortcuts.test.tsx(new): mounts the hook and dispatches realKeyboardEvents in jsdom, assertingv,c, arrows,r,i,b,x,m,z,s,u, Shift+U/D, Ctrl+L, Ctrl+Shift+O/H);u→ hide UI vs Shift+u→ crosshair superior);enabledflag, listener cleanup on unmount, and missing-handler no-ops.keyboardShortcuts.test.ts(extended):formatShortcut, plus a real-registry disambiguation check for the sharedukey.Kept in e2e (
apps/pwa/tests/keyboard-shortcuts.spec.ts): a single end-to-end smoke - a real keypress drivessliceType, and focus in a text input suppresses it. That is the one claim a unit test can't make: the hook is actually mounted and wired in the running app.Why
Per the placement policy: the key→action mapping and the focus guard are pure/component-local logic; only "the hook is wired in the real app" is genuine integration. The smell test in the README literally named this file.
Impact
Verification
@niivue/react: 120 unit tests pass (10 files).turbo lint type-check: green.Relationship to other PRs
🤖 Generated with Claude Code