diff --git a/.claude/skills/writing-tests/SKILL.md b/.claude/skills/writing-tests/SKILL.md new file mode 100644 index 00000000..da89a9cc --- /dev/null +++ b/.claude/skills/writing-tests/SKILL.md @@ -0,0 +1,178 @@ +--- +name: writing-tests +description: Canonical rules for writing tests in the Shift codebase. Use whenever you add, rewrite, or review a `.test.ts` file — or any time you're about to mock, stub, or spy your way around a testing problem. This repo has deliberately swept mock-based testing out, and this skill is what keeps it out. +--- + +# /writing-tests — How tests are written in this codebase + +The goal is tests that survive a full rewrite of the implementation, as long as the behavior stays the same. If your test breaks when a private method is renamed or a call count changes, you're testing the wrong thing. + +## The rule + +**Assert on observable state, not on mock calls.** + +Drive the code through its user-facing surface (`editor.copy()`, `editor.click()`, `toolManager.handleKeyDown(...)`) and assert on what a user would see — glyph contours, selection, viewport pan, tool state, command history, returned values. Never on "was method X called N times." + +Everything else here is a consequence of that rule. + +## If you're stuck, research — don't invent a mock + +The moment you think "I'll just mock this out" is the moment to stop and look for prior art. Real Electron / font-editor / reactive-signal codebases have solved the same class of problem: VSCode, Obsidian, Signal Desktop, Bitwarden, Fontra, tldraw. Their patterns are on GitHub. + +The `SystemClipboard` adapter in this repo came from a short WebSearch pass through VSCode's `IClipboardService` + `TestClipboardService` — not from inventing one locally. Ten minutes of searching usually beats an hour of mock wrangling, and the resulting test survives refactors because it mirrors a pattern that works in production code. + +Rule of thumb: if the repo doesn't already show you a clean way to test this, the answer is probably "inject a boundary and test through it." Go find how someone else injected the boundary before reaching for `vi.mock`. + +## When to stop and rethink + +If you catch yourself doing any of the following, stop. You're about to write a test this codebase has deliberately deleted. + +| Reaching for… | What it means | Do instead | +| ----------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| **Vitest mock primitives** — `vi.fn`, `vi.spyOn`, `toHaveBeenCalled`, `mock.calls.length`, `.mock.calls`, execution-order arrays (`applied.push(...)`) | Asserting on "was this called" instead of "what did the user see" | Find the user-facing consequence (state change, return value, emitted payload). If there isn't one, the behavior isn't worth a unit test. Exception: when you're testing a primitive whose **contract is the invocation count** (reactive library fire counts, pub/sub dispatch), a plain closure counter (`let n = 0; ...; n++`) is fine — don't reach for vitest mocks | +| **Hand-built events, states, or snapshots** — constructing `ToolEvent`s, `Coordinates`, `GlyphSnapshot`s inline so you can pass them straight into a method | Recreating the pipeline instead of using it | Drive through `TestEditor` — real gestures, real events, real state | +| **Global stubs** — `vi.stubGlobal`, monkey-patching `window`, `requestAnimationFrame`, `electronAPI` | The code has an unmanaged global dependency; stubbing papers over it | Inject the boundary. For rAF: `toolManager.flushPointerMoves()`. For IPC: `SystemClipboard`-style adapter. If the boundary doesn't exist yet, add one | +| **Parallel-world test harnesses** — mock engines, mock renderers, mock command contexts, raw `new Editor(...)` in a `.test.ts` | Reimplementing production in TypeScript so the test can "look inside." Drifts from real behavior; the mock/prod divergence is the whole pain the repo swept out | Use `TestEditor`. If you can't, extract the logic into a pure function and test that. `new Editor(...)` in tests is enforced against by `oxlint no-raw-editor-in-tests` | + +## Test categories + +| Category | Use when | Template in repo | +| -------------------- | -------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------- | +| **Tool test** | User-triggerable behavior of a tool (pointer, keyboard, selection, cursor) | `lib/tools/pen/Pen.test.ts`, `lib/tools/hand/Hand.test.ts`, `lib/tools/shape/Shape.test.ts` | +| **Command test** | A `Command`'s execute/undo/redo round-trip | `lib/commands/primitives/PointCommands.test.ts` | +| **Pure module test** | Stateless class or function with no `Editor` dependency | `lib/tools/text/TextRunController.test.ts`, `lib/editor/hit/boundingBox.test.ts` | +| **Bridge test** | `NativeBridge` against the real Rust engine | `bridge/NativeBridge.test.ts` | + +If your target doesn't fit one of these, stop and ask — don't invent a new shape. + +### Decision: which one + +1. Can a user trigger it with a click, drag, or key? → **Tool test** via `TestEditor`. +2. Is it a single command's undo/redo contract? → **Command test**. +3. Is it a pure function or pure class with no `Editor`? → **Pure module test**. +4. Is it the NAPI boundary? → **Bridge test**. + +## Templates + +### Tool test + +```ts +import { describe, it, expect, beforeEach } from "vitest"; +import { TestEditor } from "@/testing/TestEditor"; + +describe("Hand tool", () => { + let editor: TestEditor; + + beforeEach(() => { + editor = new TestEditor(); + editor.startSession(); + editor.selectTool("hand"); + }); + + it("drag pans the viewport by the screen delta", () => { + const startPan = editor.pan; + + editor.pointerDown(0, 0); + editor.pointerMove(50, 30); // crosses drag threshold + editor.pointerMove(120, 80); + editor.pointerUp(120, 80); + + expect(editor.pan.x).toBe(startPan.x + 120); + expect(editor.pan.y).toBe(startPan.y + 80); + }); +}); +``` + +### Pure module test + +```ts +import { describe, it, expect } from "vitest"; +import { SnapPipelineRunner } from "./SnapPipelineRunner"; + +describe("SnapPipelineRunner", () => { + const runner = new SnapPipelineRunner(); + + it("point-to-point wins over a closer metrics candidate", () => { + const result = runner.runPointPipeline( + [ + pointStep("p2p", hit("pointToPoint", { x: 110, y: 110 })), + pointStep("metrics", hit("metrics", { x: 101, y: 101 })), + ], + pointArgs, + ); + + expect(result.source).toBe("pointToPoint"); + expect(result.point).toEqual({ x: 110, y: 110 }); + }); +}); +``` + +Stub _inputs_ (a `PointSnapStep` here is an interface — build a minimal one). Never stub the class under test. + +### Bridge test + +```ts +import { describe, it, expect, beforeEach } from "vitest"; +import { NativeBridge } from "./NativeBridge"; +import { createBridge } from "@/testing/engine"; + +describe("NativeBridge session lifecycle", () => { + let bridge: NativeBridge; + + beforeEach(() => { + bridge = createBridge(); // real Rust engine via shift-node + }); + + it("startEditSession populates $glyph", () => { + bridge.startEditSession("A"); + expect(bridge.hasSession()).toBe(true); + expect(bridge.$glyph.peek()).not.toBe(null); + }); +}); +``` + +## The fake-test checklist + +Before committing a test, run through these. Any miss means the test is wrong. + +1. **Mental deletion.** Replace the method body under test with `throw new Error("unimplemented")`. Does your test fail? If it still passes, you wrote a tautology — usually because you asserted on a value you constructed rather than on a consequence. +2. **User-facing surface.** You drove through a method a real user can trigger (pointer, keyboard, command, menu action), not a `#private` field or a tool-internal method. +3. **Specific assertions.** `expect(editor.pointCount).toBe(4)` — yes. `expect(result).toBeDefined()` or `expect(spy).toHaveBeenCalled()` — no, those survive any implementation. +4. **Correct code path.** If the real production path goes through command history + bridge + signals, your test goes through those too. You didn't reach behind the facade. +5. **Under ~15 lines.** If a single `it()` body exceeds ~15 lines, it's testing too much at once, or its setup belongs in `beforeEach`. + +## Setup discipline + +- `beforeEach` is ≤ 5 lines: `new TestEditor()` + `startSession()` + `selectTool()` + optional fixture. +- No wrapper factories around `TestEditor`. If you need a reusable helper, it belongs as a method on `TestEditor` itself (see `pointerMove`, `click`, `escape`). +- If your test needs a pre-drawn shape, draw it with the pen/shape tool in `beforeEach` — don't construct glyph snapshots inline. + +## When testing is genuinely hard + +Some code resists clean unit testing — DOM event handlers, focus management, IME composition, React effect lifecycle. + +**Default move: extract the non-DOM logic into a pure function and test that.** Example: `HiddenTextInput` keyboard handling → `lib/tools/text/textInput.ts → handleTextKeyDown(event, editor)`. The component becomes a thin adapter; the extracted function is a normal tool test via `TestEditor`. The part that genuinely can't be tested cleanly shrinks to ~20 lines that rely on manual QA. + +If after extraction and research you're still reaching for jsdom + `@testing-library/react` + IPC mocks, pause and ask the user before introducing that infrastructure. + +## Why these rules exist + +The codebase went through a deliberate sweep that deleted thousands of lines of mock-based tests. See commits `5f2f503`, `876e542`, `642ec99`, `bfea0b5`, `3f3a6d6`, `fa8a829`, `bd07e9d`, `3f3a6d6 Delete MockFontEngine`. The motivating pain was real: + +- Mocks drifted from the real Rust behavior they pretended to simulate. Tests passed; production broke. +- Spy-count tests locked in implementation details; every refactor broke tests that weren't guarding any behavior. +- Global stubs (`vi.stubGlobal("window", ...)`) leaked across test files and hid unmanaged global dependencies. +- Mock context builders (`services.ts`, 1,795 lines) became their own maintenance burden. + +The replacement — real `Editor`, real Rust via NAPI, fake only at the outermost boundary (`SystemClipboard`, `NativeBridge`) — catches regressions mocks silently missed. Don't reintroduce what was deleted. + +## Running + +```bash +pnpm test # full vitest suite, runs against real Rust +pnpm test:watch # watch mode +pnpm typecheck # tsgo across all packages +pnpm lint:check # oxlint, includes no-raw-editor-in-tests and no-mock-call-assertions +``` + +All three must pass before committing. If `no-mock-call-assertions` flags your test, it's caught you reaching for the banned pattern — fix the test, don't disable the rule. diff --git a/Claude.md b/Claude.md index 39937f4d..45acaab6 100644 --- a/Claude.md +++ b/Claude.md @@ -16,23 +16,7 @@ When completing a feature, check ROADMAP.md and check any box if we have complet ## Testing -### TestEditor pattern - -Tests use `TestEditor` from `@/testing/TestEditor` (real Editor + real NAPI). Assert on state, not mock calls. No mock context builders, no mock renderers. Enforced by oxlint `no-raw-editor-in-tests`. - -```typescript -const editor = new TestEditor(); -editor.startSession(); -editor.selectTool("pen"); -editor.click(100, 200); -expect(editor.pointCount).toBe(1); -``` - -### Keep tests lean - -- 5-15 lines per test -- beforeEach under 5 lines (`new TestEditor()` + `startSession()` + `selectTool()`) -- No wrapper factories around TestEditor +Tests use `TestEditor` from `@/testing/TestEditor` (real Editor + real NAPI). Assert on state, not mock calls. See `/writing-tests` skill for canonical rules, templates, banned patterns, and the fake-test checklist — trigger it any time you add, rewrite, or review a `.test.ts` file. ## Frontend diff --git a/apps/desktop/.oxlintrc.json b/apps/desktop/.oxlintrc.json index b7d00dce..e312cbbf 100644 --- a/apps/desktop/.oxlintrc.json +++ b/apps/desktop/.oxlintrc.json @@ -45,7 +45,9 @@ "shift/no-repeated-optional-chain": "warn", "shift/no-section-divider-comments": "error", "shift/no-bridge-import": "error", - "shift/no-raw-editor-in-tests": "error" + "shift/no-raw-editor-in-tests": "error", + "shift/no-vi-stub-global-in-tests": "error", + "shift/no-vitest-mock-primitives": "error" }, "overrides": [ { diff --git a/apps/desktop/src/renderer/src/lib/editor/lifecycle.test.ts b/apps/desktop/src/renderer/src/lib/editor/lifecycle.test.ts index 770b7238..1391a8f3 100644 --- a/apps/desktop/src/renderer/src/lib/editor/lifecycle.test.ts +++ b/apps/desktop/src/renderer/src/lib/editor/lifecycle.test.ts @@ -1,76 +1,75 @@ -import { describe, it, expect, vi } from "vitest"; +import { describe, it, expect } from "vitest"; import { EventEmitter } from "./lifecycle"; describe("EventEmitter", () => { - it("calls handler when event is emitted", () => { + it("delivers the emitted payload to a registered handler", () => { const lifecycle = new EventEmitter(); - const handler = vi.fn(); + const payloads: unknown[] = []; - lifecycle.on("fontLoaded", handler); + lifecycle.on("fontLoaded", (p) => payloads.push(p)); lifecycle.emit("fontLoaded", { font: {} as never }); - expect(handler).toHaveBeenCalledOnce(); - expect(handler).toHaveBeenCalledWith({ font: {} }); + expect(payloads).toEqual([{ font: {} }]); }); - it("calls multiple handlers for the same event", () => { + it("delivers to every handler registered for the same event", () => { const lifecycle = new EventEmitter(); - const a = vi.fn(); - const b = vi.fn(); + const aPayloads: unknown[] = []; + const bPayloads: unknown[] = []; - lifecycle.on("fontLoaded", a); - lifecycle.on("fontLoaded", b); + lifecycle.on("fontLoaded", (p) => aPayloads.push(p)); + lifecycle.on("fontLoaded", (p) => bPayloads.push(p)); lifecycle.emit("fontLoaded", { font: {} as never }); - expect(a).toHaveBeenCalledOnce(); - expect(b).toHaveBeenCalledOnce(); + expect(aPayloads).toEqual([{ font: {} }]); + expect(bPayloads).toEqual([{ font: {} }]); }); - it("does not call handlers for other events", () => { + it("routes by event name so unrelated handlers do not fire", () => { const lifecycle = new EventEmitter(); - const handler = vi.fn(); + const saved: unknown[] = []; - lifecycle.on("fontSaved", handler); + lifecycle.on("fontSaved", (p) => saved.push(p)); lifecycle.emit("fontLoaded", { font: {} as never }); - expect(handler).not.toHaveBeenCalled(); + expect(saved).toEqual([]); }); - it("unsubscribes via returned disposer", () => { + it("unsubscribes via the returned disposer", () => { const lifecycle = new EventEmitter(); - const handler = vi.fn(); + const payloads: unknown[] = []; - const off = lifecycle.on("fontLoaded", handler); + const off = lifecycle.on("fontLoaded", (p) => payloads.push(p)); off(); lifecycle.emit("fontLoaded", { font: {} as never }); - expect(handler).not.toHaveBeenCalled(); + expect(payloads).toEqual([]); }); - it("handles events with no payload", () => { + it("delivers no-payload events", () => { const lifecycle = new EventEmitter(); - const handler = vi.fn(); + let fires = 0; - lifecycle.on("destroying", handler); + lifecycle.on("destroying", () => fires++); lifecycle.emit("destroying"); - expect(handler).toHaveBeenCalledOnce(); + expect(fires).toBe(1); }); - it("dispose removes all listeners", () => { + it("dispose clears every listener across every event", () => { const lifecycle = new EventEmitter(); - const a = vi.fn(); - const b = vi.fn(); + const loaded: unknown[] = []; + const destroying: unknown[] = []; - lifecycle.on("fontLoaded", a); - lifecycle.on("destroying", b); + lifecycle.on("fontLoaded", (p) => loaded.push(p)); + lifecycle.on("destroying", () => destroying.push(null)); lifecycle.dispose(); lifecycle.emit("fontLoaded", { font: {} as never }); lifecycle.emit("destroying"); - expect(a).not.toHaveBeenCalled(); - expect(b).not.toHaveBeenCalled(); + expect(loaded).toEqual([]); + expect(destroying).toEqual([]); }); it("emitting with no listeners does not throw", () => { diff --git a/apps/desktop/src/renderer/src/lib/editor/managers/EdgePanManager.test.ts b/apps/desktop/src/renderer/src/lib/editor/managers/EdgePanManager.test.ts index 023b97e4..65cca626 100644 --- a/apps/desktop/src/renderer/src/lib/editor/managers/EdgePanManager.test.ts +++ b/apps/desktop/src/renderer/src/lib/editor/managers/EdgePanManager.test.ts @@ -1,39 +1,45 @@ -import { describe, it, expect, beforeEach, vi } from "vitest"; -import { EdgePanManager } from "./EdgePanManager"; +import { describe, it, expect, beforeEach } from "vitest"; +import { TestEditor } from "@/testing/TestEditor"; import type { Rect2D } from "@shift/types"; +const canvasBounds: Rect2D = { + x: 0, + y: 0, + width: 800, + height: 600, + left: 0, + top: 0, + right: 800, + bottom: 600, +}; + describe("EdgePanManager", () => { - const canvasBounds: Rect2D = { - x: 0, - y: 0, - width: 800, - height: 600, - left: 0, - top: 0, - right: 800, - bottom: 600, - }; + let editor: TestEditor; beforeEach(() => { - vi.stubGlobal("requestAnimationFrame", (_cb: () => void) => 0); + editor = new TestEditor(); + editor.startSession(); + editor.selectTool("hand"); }); - it("calls handlePointerMove with force: true when ticking so tool feedback moves with pan", () => { - const handlePointerMove = vi.fn(); - const editor: ConstructorParameters[0] = { - toolManager: { handlePointerMove, isDragging: true }, - pan: { x: 0, y: 0 }, - setPan: vi.fn(), - requestRedraw: vi.fn(), - }; - - const manager = new EdgePanManager(editor, { marginSize: 50 }); - manager.update({ x: 10, y: 300 }, canvasBounds); - - expect(handlePointerMove).toHaveBeenCalledWith( - { x: 10, y: 300 }, - { shiftKey: false, altKey: false }, - { force: true }, - ); + it("pans the viewport when the cursor is near the edge during a drag", () => { + // Start a hand-tool drag so toolManager.isDragging is true. + editor.pointerDown(400, 300); + editor.pointerMove(420, 300); // crosses drag threshold, starts dragging + const panBeforeEdgePan = { ...editor.pan }; + + // Cursor near the left edge while still dragging. The viewport should + // scroll to reveal content to the left, which means pan.x increases. + editor.updateEdgePan({ x: 10, y: 300 }, canvasBounds); + + expect(editor.pan.x).toBeGreaterThan(panBeforeEdgePan.x); + }); + + it("does not pan when not dragging", () => { + const startPan = { ...editor.pan }; + + editor.updateEdgePan({ x: 10, y: 300 }, canvasBounds); + + expect(editor.pan).toEqual(startPan); }); }); diff --git a/apps/desktop/src/renderer/src/lib/keyboard/KeyboardRouter.test.ts b/apps/desktop/src/renderer/src/lib/keyboard/KeyboardRouter.test.ts index 4437c380..ed552abf 100644 --- a/apps/desktop/src/renderer/src/lib/keyboard/KeyboardRouter.test.ts +++ b/apps/desktop/src/renderer/src/lib/keyboard/KeyboardRouter.test.ts @@ -1,7 +1,6 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it } from "vitest"; import { KeyboardRouter } from "./KeyboardRouter"; -import type { KeyboardEditorActions, KeyboardToolManagerActions } from "./types"; -import type { ToolName } from "@/lib/tools/core"; +import { TestEditor } from "@/testing"; type KeyboardEventOptions = Partial< Pick @@ -26,8 +25,8 @@ function createKeyboardEvent(options: KeyboardEventOptions = {}): KeyboardEvent ctrlKey: options.ctrlKey ?? false, shiftKey: options.shiftKey ?? false, altKey: options.altKey ?? false, - preventDefault: vi.fn(), - stopPropagation: vi.fn(), + preventDefault: () => {}, + stopPropagation: () => {}, target: options.target ?? null, }; @@ -35,216 +34,188 @@ function createKeyboardEvent(options: KeyboardEventOptions = {}): KeyboardEvent } describe("KeyboardRouter", () => { - let activeTool: ToolName; + let editor: TestEditor; let canvasActive: boolean; - let editor: KeyboardEditorActions; - let toolManager: KeyboardToolManagerActions; let router: KeyboardRouter; beforeEach(() => { - activeTool = "select"; + editor = new TestEditor(); + editor.startSession(); + // setActiveTool short-circuits if the target matches the current $activeTool, + // and $activeTool defaults to "select" without actually activating a tool + // instance. Force activation directly so primaryTool is populated. + editor.toolManager.activate("select"); canvasActive = true; - Object.defineProperty(globalThis, "navigator", { - value: { clipboard: { readText: vi.fn(() => Promise.resolve("")) } }, - writable: true, - configurable: true, - }); - - editor = { - zoomIn: vi.fn(), - zoomOut: vi.fn(), - requestRedraw: vi.fn(), - copy: vi.fn(), - cut: vi.fn(), - paste: vi.fn(), - undo: vi.fn(), - redo: vi.fn(), - selectAll: vi.fn(), - deleteSelectedPoints: vi.fn(), - setActiveTool: vi.fn(), - getToolShortcuts: vi.fn(() => [ - { toolId: "select", shortcut: "v" }, - { toolId: "pen", shortcut: "p" }, - { toolId: "hand", shortcut: "h" }, - { toolId: "shape", shortcut: "s" }, - { toolId: "text", shortcut: "t" }, - ]), - requestTemporaryTool: vi.fn((_tool, options) => options?.onActivate?.()), - returnFromTemporaryTool: vi.fn(), - previewMode: false, - setPreviewMode: vi.fn(), - openGlyphFinder: vi.fn(), - }; - - toolManager = { - handleKeyDown: vi.fn(() => false), - handleKeyUp: vi.fn(() => false), - }; - router = new KeyboardRouter(() => ({ canvasActive, - activeTool, + activeTool: editor.getActiveTool(), editor, - toolManager, + toolManager: editor.toolManager, })); }); - it("runs global paste shortcut even when canvas is inactive", () => { - canvasActive = false; - const e = createKeyboardEvent({ key: "v", ctrlKey: true }); + describe("zoom shortcuts", () => { + it("zooms in on command/control + equal without shift", () => { + const zoomBefore = editor.zoom; + const e = createKeyboardEvent({ key: "=", code: "Equal", metaKey: true }); - const handled = router.handleKeyDown(e); + const handled = router.handleKeyDown(e); - expect(handled).toBe(true); - expect(editor.paste).toHaveBeenCalledTimes(1); - expect(toolManager.handleKeyDown).not.toHaveBeenCalled(); - expect(e.preventDefault).toHaveBeenCalledTimes(1); - }); + expect(handled).toBe(true); + expect(editor.zoom).toBeGreaterThan(zoomBefore); + }); - it("applies canvas zoom in for command/control + equal without shift", () => { - const e = createKeyboardEvent({ key: "=", code: "Equal", metaKey: true, shiftKey: false }); + it("zooms out on command/control + minus without shift", () => { + const zoomBefore = editor.zoom; + const e = createKeyboardEvent({ key: "-", code: "Minus", ctrlKey: true }); - const handled = router.handleKeyDown(e); + const handled = router.handleKeyDown(e); - expect(handled).toBe(true); - expect(editor.zoomIn).toHaveBeenCalledTimes(1); - expect(editor.requestRedraw).toHaveBeenCalledTimes(1); - expect(e.preventDefault).toHaveBeenCalledTimes(1); - }); + expect(handled).toBe(true); + expect(editor.zoom).toBeLessThan(zoomBefore); + }); - it("applies canvas zoom out for command/control + minus without shift", () => { - const e = createKeyboardEvent({ key: "-", code: "Minus", ctrlKey: true, shiftKey: false }); + it("does not intercept shift+equal (leaves it for native UI zoom)", () => { + canvasActive = false; + const zoomBefore = editor.zoom; + const e = createKeyboardEvent({ key: "+", code: "Equal", metaKey: true, shiftKey: true }); - const handled = router.handleKeyDown(e); + const handled = router.handleKeyDown(e); - expect(handled).toBe(true); - expect(editor.zoomOut).toHaveBeenCalledTimes(1); - expect(editor.requestRedraw).toHaveBeenCalledTimes(1); - expect(e.preventDefault).toHaveBeenCalledTimes(1); - }); + expect(handled).toBe(false); + expect(editor.zoom).toBe(zoomBefore); + }); - it("does not intercept command/control + shift + equal (allows native UI zoom)", () => { - canvasActive = false; - const e = createKeyboardEvent({ key: "+", code: "Equal", metaKey: true, shiftKey: true }); + it("does not intercept shift+minus (leaves it for native UI zoom)", () => { + canvasActive = false; + const zoomBefore = editor.zoom; + const e = createKeyboardEvent({ key: "_", code: "Minus", ctrlKey: true, shiftKey: true }); - const handled = router.handleKeyDown(e); + const handled = router.handleKeyDown(e); - expect(handled).toBe(false); - expect(editor.zoomIn).not.toHaveBeenCalled(); - expect(editor.requestRedraw).not.toHaveBeenCalled(); - expect(e.preventDefault).not.toHaveBeenCalled(); + expect(handled).toBe(false); + expect(editor.zoom).toBe(zoomBefore); + }); }); - it("does not intercept command/control + shift + minus (allows native UI zoom)", () => { - canvasActive = false; - const e = createKeyboardEvent({ key: "_", code: "Minus", ctrlKey: true, shiftKey: true }); + describe("tool shortcuts", () => { + it("switches tools from canvas shortcuts for non-text tools", () => { + const e = createKeyboardEvent({ key: "s" }); - const handled = router.handleKeyDown(e); + const handled = router.handleKeyDown(e); - expect(handled).toBe(false); - expect(editor.zoomOut).not.toHaveBeenCalled(); - expect(editor.requestRedraw).not.toHaveBeenCalled(); - expect(e.preventDefault).not.toHaveBeenCalled(); - }); + expect(handled).toBe(true); + expect(editor.getActiveTool()).toBe("shape"); + }); - it("does not run tool shortcuts outside canvas context", () => { - canvasActive = false; - const e = createKeyboardEvent({ key: "s" }); + it("does not run tool shortcuts outside the canvas context", () => { + canvasActive = false; + const e = createKeyboardEvent({ key: "s" }); - const handled = router.handleKeyDown(e); + const handled = router.handleKeyDown(e); - expect(handled).toBe(false); - expect(editor.setActiveTool).not.toHaveBeenCalled(); - expect(toolManager.handleKeyDown).not.toHaveBeenCalled(); - }); + expect(handled).toBe(false); + expect(editor.getActiveTool()).toBe("select"); + }); - it("switches tools from canvas shortcuts for non-text tools", () => { - activeTool = "select"; - const e = createKeyboardEvent({ key: "s" }); + it("does not intercept plain typing while the text tool is active", () => { + editor.selectTool("text"); + const e = createKeyboardEvent({ key: "s" }); - const handled = router.handleKeyDown(e); + const handled = router.handleKeyDown(e); - expect(handled).toBe(true); - expect(editor.setActiveTool).toHaveBeenCalledWith("shape"); - expect(editor.requestRedraw).toHaveBeenCalledTimes(1); - expect(toolManager.handleKeyDown).not.toHaveBeenCalled(); + expect(handled).toBe(false); + expect(editor.getActiveTool()).toBe("text"); + }); }); - it("does not intercept plain typing in text mode (textarea handles it)", () => { - activeTool = "text"; - const e = createKeyboardEvent({ key: "s" }); - - const handled = router.handleKeyDown(e); - - expect(handled).toBe(false); - expect(editor.setActiveTool).not.toHaveBeenCalled(); - }); + describe("clipboard shortcuts", () => { + beforeEach(() => { + editor.selectTool("pen"); + editor.click(100, 100); + editor.click(200, 100); + editor.click(200, 200); + editor.click(100, 200); + editor.selectTool("select"); + editor.selectAll(); + }); - it("does not intercept paste in text mode (textarea handles it)", () => { - activeTool = "text"; - const e = createKeyboardEvent({ key: "v", metaKey: true }); + it("runs paste even when canvas is inactive", async () => { + await editor.copy(); + const pointsBefore = editor.pointCount; + canvasActive = false; + const e = createKeyboardEvent({ key: "v", ctrlKey: true }); - router.handleKeyDown(e); + const handled = router.handleKeyDown(e); + // Clipboard paste is async; wait for the microtask to settle. + await Promise.resolve(); + await Promise.resolve(); - expect(editor.paste).not.toHaveBeenCalled(); - }); + expect(handled).toBe(true); + expect(editor.pointCount).toBeGreaterThan(pointsBefore); + }); - it("does not intercept copy in text mode (textarea handles it)", () => { - activeTool = "text"; - const e = createKeyboardEvent({ key: "c", metaKey: true }); + it("does not intercept paste while the text tool is active", async () => { + await editor.copy(); + editor.selectTool("text"); + const pointsBefore = editor.pointCount; + const e = createKeyboardEvent({ key: "v", metaKey: true }); - router.handleKeyDown(e); + router.handleKeyDown(e); + await Promise.resolve(); - expect(editor.copy).not.toHaveBeenCalled(); - }); + expect(editor.pointCount).toBe(pointsBefore); + }); - it("inserts space in text mode instead of activating temporary hand", () => { - activeTool = "text"; - const e = createKeyboardEvent({ key: " ", code: "Space" }); - (toolManager.handleKeyDown as ReturnType).mockReturnValue(true); + it("does not intercept copy while the text tool is active", async () => { + editor.selectTool("text"); + const bufferBefore = editor.clipboardBuffer; + const e = createKeyboardEvent({ key: "c", metaKey: true }); - const handled = router.handleKeyDown(e); + router.handleKeyDown(e); + await Promise.resolve(); - expect(handled).toBe(true); - expect(editor.requestTemporaryTool).not.toHaveBeenCalled(); - expect(toolManager.handleKeyDown).toHaveBeenCalledWith(e); + expect(editor.clipboardBuffer).toBe(bufferBefore); + }); }); - it("activates temporary hand on space and releases it on keyup", () => { - activeTool = "select"; - const down = createKeyboardEvent({ key: " ", code: "Space" }); - const up = createKeyboardEvent({ key: " ", code: "Space" }); - - const downHandled = router.handleKeyDown(down); - const upHandled = router.handleKeyUp(up); - - expect(downHandled).toBe(true); - expect(editor.requestTemporaryTool).toHaveBeenCalledTimes(1); - expect(editor.setPreviewMode).toHaveBeenCalledWith(true); - expect(upHandled).toBe(true); - expect(editor.returnFromTemporaryTool).toHaveBeenCalledTimes(1); - }); + describe("temporary hand tool (space)", () => { + it("activates the hand tool on space and returns to the previous tool on keyup", () => { + const down = createKeyboardEvent({ key: " ", code: "Space" }); + const up = createKeyboardEvent({ key: " ", code: "Space" }); + + router.handleKeyDown(down); + // Temporary tools are tracked as toolManager overrides; activeToolId + // reflects the override while primaryToolId stays on the base tool. + expect(editor.toolManager.activeToolId).toBe("hand"); + expect(editor.toolManager.primaryToolId).toBe("select"); + expect(editor.previewMode).toBe(true); + + router.handleKeyUp(up); + expect(editor.toolManager.activeToolId).toBe("select"); + expect(editor.previewMode).toBe(false); + }); - it("falls back to tool manager when no keymap matches in canvas", () => { - const e = createKeyboardEvent({ key: "q" }); - (toolManager.handleKeyDown as ReturnType).mockReturnValue(true); + it("does not activate the hand tool on space while the text tool is active", () => { + editor.selectTool("text"); + const e = createKeyboardEvent({ key: " ", code: "Space" }); - const handled = router.handleKeyDown(e); + router.handleKeyDown(e); - expect(handled).toBe(true); - expect(toolManager.handleKeyDown).toHaveBeenCalledWith(e); + expect(editor.getActiveTool()).toBe("text"); + }); }); - it("does not intercept shortcuts when an editable input has focus", () => { - const input = { tagName: "INPUT" } as EventTarget & { tagName: string }; - const e = createKeyboardEvent({ key: "a", metaKey: true, target: input }); + describe("focus handling", () => { + it("does not intercept shortcuts while an editable input has focus", () => { + const input = { tagName: "INPUT" } as EventTarget & { tagName: string }; + const e = createKeyboardEvent({ key: "a", metaKey: true, target: input }); - const handled = router.handleKeyDown(e); + const handled = router.handleKeyDown(e); - expect(handled).toBe(false); - expect(editor.selectAll).not.toHaveBeenCalled(); - expect(toolManager.handleKeyDown).not.toHaveBeenCalled(); - expect(e.preventDefault).not.toHaveBeenCalled(); + expect(handled).toBe(false); + expect(editor.selection.pointIds.size).toBe(0); + }); }); }); diff --git a/apps/desktop/src/renderer/src/lib/reactive/signal.test.ts b/apps/desktop/src/renderer/src/lib/reactive/signal.test.ts index 59e63fd6..7b6472bc 100644 --- a/apps/desktop/src/renderer/src/lib/reactive/signal.test.ts +++ b/apps/desktop/src/renderer/src/lib/reactive/signal.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from "vitest"; +import { describe, it, expect } from "vitest"; import { signal, computed, effect, batch, untracked, isTracking } from "./signal"; describe("signal", () => { @@ -27,9 +27,7 @@ describe("signal", () => { it("should return value without tracking via peek()", () => { const s = signal(10); - const fn = vi.fn(() => s.peek()); - - const c = computed(fn); + const c = computed(() => s.peek()); expect(c.value).toBe(10); s.value = 20; @@ -39,35 +37,35 @@ describe("signal", () => { it("should not notify if value is the same (Object.is)", () => { const s = signal(1); - const fn = vi.fn(); + let fires = 0; effect(() => { s.value; - fn(); + fires++; }); - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); s.value = 1; // Same value - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); s.value = 2; // Different value - expect(fn).toHaveBeenCalledTimes(2); + expect(fires).toBe(2); }); it("should handle NaN correctly", () => { const s = signal(NaN); - const fn = vi.fn(); + let fires = 0; effect(() => { s.value; - fn(); + fires++; }); - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); s.value = NaN; // NaN === NaN via Object.is - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); }); }); @@ -91,17 +89,20 @@ describe("computed", () => { }); it("should be lazy (only compute when accessed)", () => { - const fn = vi.fn(() => 42); - const c = computed(fn); + let computes = 0; + const c = computed(() => { + computes++; + return 42; + }); - expect(fn).not.toHaveBeenCalled(); + expect(computes).toBe(0); c.value; - expect(fn).toHaveBeenCalledTimes(1); + expect(computes).toBe(1); // Accessing again should not recompute (not dirty) c.value; - expect(fn).toHaveBeenCalledTimes(1); + expect(computes).toBe(1); }); it("should chain computed values", () => { @@ -131,41 +132,40 @@ describe("computed", () => { const a = signal(1); const b = signal(2); - const fn = vi.fn(); + let computes = 0; const c = computed(() => { - fn(); + computes++; return condition.value ? a.value : b.value; }); c.value; // Initial compute, depends on condition + a - expect(fn).toHaveBeenCalledTimes(1); + expect(computes).toBe(1); a.value = 10; // Should trigger recompute c.value; - expect(fn).toHaveBeenCalledTimes(2); + expect(computes).toBe(2); b.value = 20; // Should NOT trigger recompute (not a dependency) c.value; - expect(fn).toHaveBeenCalledTimes(2); + expect(computes).toBe(2); // Switch condition condition.value = false; c.value; - expect(fn).toHaveBeenCalledTimes(3); + expect(computes).toBe(3); // Now a should NOT trigger, but b should a.value = 100; c.value; - expect(fn).toHaveBeenCalledTimes(3); + expect(computes).toBe(3); b.value = 200; c.value; - expect(fn).toHaveBeenCalledTimes(4); + expect(computes).toBe(4); }); it("should support invalidate()", () => { - const fn = vi.fn(() => Math.random()); - const c = computed(fn); + const c = computed(() => Math.random()); const v1 = c.value; const v2 = c.value; @@ -179,84 +179,90 @@ describe("computed", () => { describe("effect", () => { it("should run immediately", () => { - const fn = vi.fn(); - effect(fn); - expect(fn).toHaveBeenCalledTimes(1); + let fires = 0; + effect(() => { + fires++; + }); + expect(fires).toBe(1); }); it("should re-run when dependencies change", () => { const s = signal(1); - const fn = vi.fn(); + let fires = 0; effect(() => { s.value; - fn(); + fires++; }); - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); s.value = 2; - expect(fn).toHaveBeenCalledTimes(2); + expect(fires).toBe(2); s.value = 3; - expect(fn).toHaveBeenCalledTimes(3); + expect(fires).toBe(3); }); it("should stop running after dispose()", () => { const s = signal(1); - const fn = vi.fn(); + let fires = 0; const fx = effect(() => { s.value; - fn(); + fires++; }); - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); fx.dispose(); s.value = 2; - expect(fn).toHaveBeenCalledTimes(1); // No additional calls + expect(fires).toBe(1); // No additional calls }); it("should run cleanup function before re-execution", () => { const s = signal(1); - const cleanup = vi.fn(); - const executed = vi.fn(); + let cleaned = 0; + let executed = 0; effect(() => { s.value; // Subscribe to signal - executed(); - return cleanup; + executed++; + return () => { + cleaned++; + }; }); - expect(executed).toHaveBeenCalledTimes(1); - expect(cleanup).not.toHaveBeenCalled(); // No cleanup on first run + expect(executed).toBe(1); + expect(cleaned).toBe(0); // No cleanup on first run s.value = 2; - expect(executed).toHaveBeenCalledTimes(2); - expect(cleanup).toHaveBeenCalledTimes(1); // Cleanup called before re-execution + expect(executed).toBe(2); + expect(cleaned).toBe(1); // Cleanup called before re-execution s.value = 3; - expect(executed).toHaveBeenCalledTimes(3); - expect(cleanup).toHaveBeenCalledTimes(2); + expect(executed).toBe(3); + expect(cleaned).toBe(2); }); it("should run cleanup on dispose", () => { - const cleanup = vi.fn(); - const fx = effect(() => cleanup); + let cleaned = 0; + const fx = effect(() => () => { + cleaned++; + }); - expect(cleanup).not.toHaveBeenCalled(); + expect(cleaned).toBe(0); fx.dispose(); - expect(cleanup).toHaveBeenCalledTimes(1); + expect(cleaned).toBe(1); }); it("should update dependencies on re-run", () => { const condition = signal(true); const a = signal(1); const b = signal(2); - const fn = vi.fn(); + let fires = 0; effect(() => { if (condition.value) { @@ -264,45 +270,45 @@ describe("effect", () => { } else { b.value; } - fn(); + fires++; }); - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); // a is a dependency a.value = 10; - expect(fn).toHaveBeenCalledTimes(2); + expect(fires).toBe(2); // b is NOT a dependency yet b.value = 20; - expect(fn).toHaveBeenCalledTimes(2); + expect(fires).toBe(2); // Switch condition condition.value = false; - expect(fn).toHaveBeenCalledTimes(3); + expect(fires).toBe(3); // Now b IS a dependency, a is NOT b.value = 30; - expect(fn).toHaveBeenCalledTimes(4); + expect(fires).toBe(4); a.value = 100; - expect(fn).toHaveBeenCalledTimes(4); + expect(fires).toBe(4); }); it("should react to computed dependencies", () => { const s = signal(2); const doubled = computed(() => s.value * 2); - const fn = vi.fn(); + let fires = 0; effect(() => { doubled.value; - fn(); + fires++; }); - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); s.value = 3; - expect(fn).toHaveBeenCalledTimes(2); + expect(fires).toBe(2); }); }); @@ -310,15 +316,15 @@ describe("batch", () => { it("should defer effect execution until batch completes", () => { const a = signal(1); const b = signal(2); - const fn = vi.fn(); + let fires = 0; effect(() => { a.value; b.value; - fn(); + fires++; }); - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); batch(() => { a.value = 10; @@ -326,26 +332,24 @@ describe("batch", () => { }); // Effect should only run once, not twice - expect(fn).toHaveBeenCalledTimes(2); + expect(fires).toBe(2); }); it("should return the value from the batch function", () => { - const result = batch(() => { - return 42; - }); + const result = batch(() => 42); expect(result).toBe(42); }); it("should handle nested batches", () => { const s = signal(0); - const fn = vi.fn(); + let fires = 0; effect(() => { s.value; - fn(); + fires++; }); - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); batch(() => { s.value = 1; @@ -357,7 +361,7 @@ describe("batch", () => { }); // Should only run once after all batches complete - expect(fn).toHaveBeenCalledTimes(2); + expect(fires).toBe(2); expect(s.value).toBe(4); }); @@ -381,24 +385,29 @@ describe("untracked", () => { it("should read signals without creating dependencies", () => { const a = signal(1); const b = signal(2); - const fn = vi.fn(); + const calls: Array<[number, number]> = []; effect(() => { const aVal = a.value; // Tracked const bVal = untracked(() => b.value); // NOT tracked - fn(aVal, bVal); + calls.push([aVal, bVal]); }); - expect(fn).toHaveBeenCalledTimes(1); - expect(fn).toHaveBeenCalledWith(1, 2); + expect(calls).toEqual([[1, 2]]); // Changing a should trigger effect a.value = 10; - expect(fn).toHaveBeenCalledTimes(2); + expect(calls).toEqual([ + [1, 2], + [10, 2], + ]); // Changing b should NOT trigger effect b.value = 20; - expect(fn).toHaveBeenCalledTimes(2); + expect(calls).toEqual([ + [1, 2], + [10, 2], + ]); }); it("should return the value from the function", () => { @@ -457,49 +466,49 @@ describe("edge cases", () => { it("should handle object values", () => { const obj = signal({ count: 0 }); - const fn = vi.fn(); + let fires = 0; effect(() => { obj.value.count; - fn(); + fires++; }); - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); // Mutating the object doesn't trigger (same reference) obj.value.count = 1; - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); // Replacing the object triggers obj.value = { count: 2 }; - expect(fn).toHaveBeenCalledTimes(2); + expect(fires).toBe(2); }); it("should handle multiple effects on same signal", () => { const s = signal(0); - const fn1 = vi.fn(); - const fn2 = vi.fn(); + let fires1 = 0; + let fires2 = 0; effect(() => { s.value; - fn1(); + fires1++; }); effect(() => { s.value; - fn2(); + fires2++; }); - expect(fn1).toHaveBeenCalledTimes(1); - expect(fn2).toHaveBeenCalledTimes(1); + expect(fires1).toBe(1); + expect(fires2).toBe(1); s.value = 1; - expect(fn1).toHaveBeenCalledTimes(2); - expect(fn2).toHaveBeenCalledTimes(2); + expect(fires1).toBe(2); + expect(fires2).toBe(2); }); it("should handle effect disposal during another effect execution", () => { const s = signal(0); - const fn = vi.fn(); + let fires = 0; let fx2: ReturnType | null = null; @@ -512,17 +521,17 @@ describe("edge cases", () => { fx2 = effect(() => { s.value; - fn(); + fires++; }); - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); // This should dispose fx2 via fx1 s.value = 1; // fx2 should not run again after being disposed s.value = 2; - expect(fn).toHaveBeenCalledTimes(1); + expect(fires).toBe(1); fx1.dispose(); }); diff --git a/apps/desktop/src/renderer/src/lib/tools/core/BaseTool.contract.test.ts b/apps/desktop/src/renderer/src/lib/tools/core/BaseTool.test.ts similarity index 61% rename from apps/desktop/src/renderer/src/lib/tools/core/BaseTool.contract.test.ts rename to apps/desktop/src/renderer/src/lib/tools/core/BaseTool.test.ts index b7c7fc5f..031d789f 100644 --- a/apps/desktop/src/renderer/src/lib/tools/core/BaseTool.contract.test.ts +++ b/apps/desktop/src/renderer/src/lib/tools/core/BaseTool.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, vi } from "vitest"; +import { describe, it, expect, beforeEach } from "vitest"; import { BaseTool } from "./BaseTool"; import type { ToolEvent } from "./GestureDetector"; import { makeTestCoordinates, TestEditor } from "@/testing"; @@ -15,10 +15,19 @@ const ClickBehavior: Behavior = { }, }; +/** + * Captures every (prev, next, event) triple passed to onStateChange so + * tests can assert on the full lifecycle payload — what the BaseTool + * contract promises to downstream tool implementations. + */ class ContractTestTool extends BaseTool { readonly id: ToolName = "select"; readonly behaviors: Behavior[] = [ClickBehavior]; - onStateChangeSpy = vi.fn(); + readonly stateChanges: Array<{ + prev: ContractState; + next: ContractState; + event: ToolEvent; + }> = []; initialState(): ContractState { return { type: "idle" }; @@ -33,26 +42,23 @@ class ContractTestTool extends BaseTool { next: ContractState, event: ToolEvent, ): void { - this.onStateChangeSpy(prev, next, event); + this.stateChanges.push({ prev, next, event }); } } describe("BaseTool contract", () => { let tool: ContractTestTool; let editor: TestEditor; - let setActiveToolStateSpy: ReturnType; beforeEach(() => { editor = new TestEditor(); - setActiveToolStateSpy = vi.spyOn(editor, "setActiveToolState"); tool = new ContractTestTool(editor); tool.activate(); - setActiveToolStateSpy.mockClear(); - tool.onStateChangeSpy.mockClear(); + tool.stateChanges.length = 0; }); - describe("lifecycle when state changes", () => { - it("calls setActiveToolState with next state and onStateChange with prev, next, event", () => { + describe("when state changes", () => { + it("advances tool state and fires onStateChange with prev/next/event", () => { const clickEvent: ToolEvent = { type: "click", point: { x: 10, y: 10 }, @@ -63,20 +69,16 @@ describe("BaseTool contract", () => { tool.handleEvent(clickEvent); - expect(setActiveToolStateSpy).toHaveBeenCalledTimes(1); - expect(setActiveToolStateSpy).toHaveBeenCalledWith({ type: "clicked" }); - expect(tool.onStateChangeSpy).toHaveBeenCalledTimes(1); - expect(tool.onStateChangeSpy).toHaveBeenCalledWith( - { type: "ready" }, - { type: "clicked" }, - clickEvent, - ); expect(tool.getState()).toEqual({ type: "clicked" }); + expect(editor.getActiveToolState()).toEqual({ type: "clicked" }); + expect(tool.stateChanges).toEqual([ + { prev: { type: "ready" }, next: { type: "clicked" }, event: clickEvent }, + ]); }); }); - describe("lifecycle when state is unchanged (same reference)", () => { - it("does not call onStateChange when no behavior matches", () => { + describe("when state is unchanged (same reference)", () => { + it("does not fire onStateChange when no behavior matches", () => { const moveEvent: ToolEvent = { type: "pointerMove", point: { x: 10, y: 10 }, @@ -85,12 +87,11 @@ describe("BaseTool contract", () => { tool.handleEvent(moveEvent); - expect(setActiveToolStateSpy).not.toHaveBeenCalled(); - expect(tool.onStateChangeSpy).not.toHaveBeenCalled(); expect(tool.getState()).toEqual({ type: "ready" }); + expect(tool.stateChanges).toEqual([]); }); - it("does not call onStateChange when transition returns same state after clicked", () => { + it("does not fire onStateChange when transition returns same state after clicked", () => { tool.handleEvent({ type: "click", point: { x: 0, y: 0 }, @@ -98,8 +99,7 @@ describe("BaseTool contract", () => { shiftKey: false, altKey: false, }); - setActiveToolStateSpy.mockClear(); - tool.onStateChangeSpy.mockClear(); + tool.stateChanges.length = 0; tool.handleEvent({ type: "pointerMove", @@ -107,8 +107,7 @@ describe("BaseTool contract", () => { coords: makeTestCoordinates({ x: 5, y: 5 }), }); - expect(setActiveToolStateSpy).not.toHaveBeenCalled(); - expect(tool.onStateChangeSpy).not.toHaveBeenCalled(); + expect(tool.stateChanges).toEqual([]); }); }); }); diff --git a/apps/desktop/src/renderer/src/lib/tools/text/layout.test.ts b/apps/desktop/src/renderer/src/lib/tools/text/layout.test.ts index a504c64b..3aca904c 100644 --- a/apps/desktop/src/renderer/src/lib/tools/text/layout.test.ts +++ b/apps/desktop/src/renderer/src/lib/tools/text/layout.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from "vitest"; +import { describe, it, expect } from "vitest"; import type { Font } from "@/lib/model/Font"; import type { Bounds } from "@shift/geo"; import { computeTextLayout, hitTestTextSlot, hitTestTextCaret, type GlyphRef } from "./layout"; @@ -19,6 +19,11 @@ function createMockFont( glyphNameMap[u] = `uni${u.toString(16).toUpperCase()}`; } + // NOTE: this parallel-world Font mock is the exact pattern the + // /writing-tests skill bans — tracked for proper rewrite in + // projects/shift/text-layout-rethink.md. Kept narrow (only the methods + // computeTextLayout + hitTestTextSlot/Caret actually call) until the + // layout API is reshaped. return { getMetrics: () => ({ unitsPerEm: 1000, @@ -31,22 +36,6 @@ function createMockFont( underlinePosition: null, underlineThickness: null, }), - getMetadata: () => ({ - familyName: "Test", - styleName: null, - versionMajor: null, - versionMinor: null, - copyright: null, - trademark: null, - designer: null, - designerUrl: null, - manufacturer: null, - manufacturerUrl: null, - license: null, - licenseUrl: null, - description: null, - note: null, - }), nameForUnicode: (unicode: number) => glyphNameMap[unicode] ?? null, getPath: (name: string) => { const unicode = Object.entries(glyphNameMap).find(([, n]) => n === name)?.[0]; @@ -69,7 +58,7 @@ function createMockFont( if (!unicode) return null; return svgPaths[Number(unicode)] ?? null; }, - }; + } as unknown as Font; } function toGlyphs(unicodes: number[]): GlyphRef[] { @@ -183,8 +172,15 @@ describe("hitTestTextSlot", () => { }); it("shape hit test should reject points outside glyph bbox before path hit", () => { + // Bbox pre-check is an optimization contract: we must not invoke the + // expensive path hit tester when the point is clearly outside the glyph + // bbox. Count invocations to verify the optimization holds. + let hitPathCalls = 0; const pathHitTester = { - hitPath: vi.fn(() => true), + hitPath: () => { + hitPathCalls++; + return true; + }, }; const font = createMockFont( @@ -207,7 +203,7 @@ describe("hitTestTextSlot", () => { }); expect(result).toBeNull(); - expect(pathHitTester.hitPath).not.toHaveBeenCalled(); + expect(hitPathCalls).toBe(0); }); it("shape hit test should allow zero-advance combining marks by glyph bounds", () => { @@ -215,11 +211,12 @@ describe("hitTestTextSlot", () => { const letterA = 0x0061; const spacingAcute = 0x00b4; + let hitPathCalls = 0; const pathHitTester = { - hitPath: vi.fn( - (_path: Path2D, x: number, y: number, _strokeWidth: number, _fill: boolean) => - x >= 40 && x <= 120 && y >= 0 && y <= 100, - ), + hitPath: (_path: Path2D, x: number, y: number, _strokeWidth: number, _fill: boolean) => { + hitPathCalls++; + return x >= 40 && x <= 120 && y >= 0 && y <= 100; + }, }; const font = createMockFont( @@ -253,7 +250,7 @@ describe("hitTestTextSlot", () => { }), ).toBe(0); - expect(pathHitTester.hitPath).toHaveBeenCalled(); + expect(hitPathCalls).toBeGreaterThan(0); }); it("should hit slots on the second line", () => { @@ -287,12 +284,12 @@ describe("hitTestTextSlot", () => { }); it("shape hit test should return slot only when path hit succeeds", () => { + let hitPathCalls = 0; const pathHitTester = { - hitPath: vi.fn( - (_path: Path2D, x: number, y: number, _strokeWidth: number, _fill: boolean) => { - return x >= 20 && x <= 80 && y >= 10 && y <= 90; - }, - ), + hitPath: (_path: Path2D, x: number, y: number, _strokeWidth: number, _fill: boolean) => { + hitPathCalls++; + return x >= 20 && x <= 80 && y >= 10 && y <= 90; + }, }; const font = createMockFont( @@ -325,7 +322,7 @@ describe("hitTestTextSlot", () => { }), ).toBe(0); - expect(pathHitTester.hitPath).toHaveBeenCalled(); + expect(hitPathCalls).toBeGreaterThan(0); }); }); diff --git a/apps/desktop/src/renderer/src/lib/editor/managers/SelectionManager.test.ts b/apps/desktop/src/renderer/src/types/selection.test.ts similarity index 100% rename from apps/desktop/src/renderer/src/lib/editor/managers/SelectionManager.test.ts rename to apps/desktop/src/renderer/src/types/selection.test.ts diff --git a/scripts/oxlint/shift-plugin.mjs b/scripts/oxlint/shift-plugin.mjs index d132a4ec..65b25856 100644 --- a/scripts/oxlint/shift-plugin.mjs +++ b/scripts/oxlint/shift-plugin.mjs @@ -866,5 +866,137 @@ export default { }; }, }, + + /** + * Ban vitest mock primitives in test files. + * + * `vi.fn`, `vi.spyOn`, `toHaveBeenCalled*`, and `.mock.calls` lead to + * tests that assert on invocations ("was method X called N times") rather + * than observable state ("did the user's glyph change"). The codebase + * has deliberately swept this pattern out — see commits 5f2f503, + * fa8a829, bd07e9d. + * + * Exception: when you're testing a primitive whose contract IS the + * invocation count (reactive fire counts, pub/sub dispatch), use a + * plain closure counter (`let n = 0; ...; n++`) — that pattern is + * legitimate and not flagged by this rule. + * + * See the /writing-tests skill for the full rationale and migration + * patterns. + */ + "no-vitest-mock-primitives": { + meta: { + type: "suggestion", + messages: { + noViFn: + "vi.fn() builds a spy whose only use is counting invocations. Capture the observable effect in a closure (payload array, flag, counter) or drive through TestEditor. See /writing-tests skill.", + noViSpyOn: + "vi.spyOn is for asserting on call counts — the banned pattern. Drive the real code through its user-facing surface and assert on observable state. See /writing-tests skill.", + noToHaveBeenCalled: + "Asserting on whether a function was called couples the test to implementation. Assert on the observable consequence (state change, return value, emitted payload). See /writing-tests skill.", + noMockCalls: + "Inspecting .mock.calls is the banned pattern. Assert on the outcome the code produces, not the invocations it made. See /writing-tests skill.", + }, + schema: [], + }, + create(context) { + const filename = context.getFilename(); + if (!filename.includes(".test.")) return {}; + + return { + CallExpression(node) { + const callee = node.callee; + if ( + callee && + callee.type === "MemberExpression" && + callee.object && + callee.object.type === "Identifier" && + callee.object.name === "vi" && + callee.property && + callee.property.type === "Identifier" + ) { + if (callee.property.name === "fn") { + context.report({ node, messageId: "noViFn" }); + } else if (callee.property.name === "spyOn") { + context.report({ node, messageId: "noViSpyOn" }); + } + } + + if ( + callee && + callee.type === "MemberExpression" && + callee.property && + callee.property.type === "Identifier" && + typeof callee.property.name === "string" && + callee.property.name.startsWith("toHaveBeenCalled") + ) { + context.report({ node, messageId: "noToHaveBeenCalled" }); + } + }, + MemberExpression(node) { + if ( + node.property && + node.property.type === "Identifier" && + node.property.name === "calls" && + node.object && + node.object.type === "MemberExpression" && + node.object.property && + node.object.property.type === "Identifier" && + node.object.property.name === "mock" + ) { + context.report({ node, messageId: "noMockCalls" }); + } + }, + }; + }, + }, + + /** + * Ban `vi.stubGlobal(...)` in test files. + * + * Global stubbing (`vi.stubGlobal("window", ...)`, `vi.stubGlobal("requestAnimationFrame", ...)`) + * papers over unmanaged global dependencies. Leaks across tests if unstub + * is forgotten, order-sensitive, and hides that the production code has + * a hard-coded global that should be an injected boundary. + * + * - For rAF-backed pipelines, use `toolManager.flushPointerMoves()` + * (TestEditor.pointerMove already does this). + * - For `window.electronAPI` / IPC, inject an adapter through the + * constructor (see `SystemClipboard` in `lib/clipboard/`). + * + * See `/writing-tests` skill for the full rationale. + */ + "no-vi-stub-global-in-tests": { + meta: { + type: "suggestion", + messages: { + noStubGlobal: + "Do not use vi.stubGlobal in tests. Inject the boundary via a constructor adapter (SystemClipboard pattern) or use the synchronous seam (toolManager.flushPointerMoves). See /writing-tests skill.", + }, + schema: [], + }, + create(context) { + const filename = context.getFilename(); + if (!filename.includes(".test.")) return {}; + + return { + CallExpression(node) { + const callee = node.callee; + if ( + callee && + callee.type === "MemberExpression" && + callee.object && + callee.object.type === "Identifier" && + callee.object.name === "vi" && + callee.property && + callee.property.type === "Identifier" && + callee.property.name === "stubGlobal" + ) { + context.report({ node, messageId: "noStubGlobal" }); + } + }, + }; + }, + }, }, };