diff --git a/docs/testing-skill-compliance-audit.md b/docs/testing-skill-compliance-audit.md new file mode 100644 index 0000000..b6de12e --- /dev/null +++ b/docs/testing-skill-compliance-audit.md @@ -0,0 +1,232 @@ +# Testing-guide compliance audit (relay input) + +## Purpose + +Audit tests added/modified while the `testing-guide` skill was silently +non-loading, and identify concrete gaps for a relay to fix. This document is the +input to that relay: each finding is a discrete, actionable work item. + +## Why the skill was broken, and the exact window + +The skill failed to load because its YAML frontmatter `description` contained an +unquoted colon: + +``` +description: Repository-specific testing guide. Use for any test work: planning ... +``` + +A bare `:` inside an unquoted YAML scalar breaks parsing, so the skill silently +did not load. It was quoted (fixed) later. + +- **Broken by:** `a266541` — `docs: shorten testing guide skill description` (2026-07-04) +- **Fixed by:** `aedcbf8` — `feat(sessions): surface live session startup warnings in the web UI` (2026-07-17 19:16), which quoted the description string. + +**Audit window:** commits `a266541..aedcbf8` (exclusive of the break commit, +inclusive of the fix commit). Any test authored in this window was written +without the skill in context. + +- New test files in window: **61** +- Modified test files in window: **72** + +The dominant, systemic compliance problem is concentrated in **Lit client +component tests** that use TemplateResult / private-member reflection. That is +the focus below. Server/helper tests sampled in the window were largely +compliant; the two timing exceptions are listed in Finding 4. + +--- + +## Skill rules most relevant to the findings + +From `.agents/skills/testing-guide/SKILL.md`: + +- **Component tests** should prefer public/component boundaries: instantiate and + set properties, dispatch events against rendered DOM, assert user-visible + rendered state or controller calls. +- **TemplateResult event-handler extraction** (calling `render()`, inspecting + `strings`/`values`, invoking a handler directly) is an **escape hatch**, usable + only when all of: (1) specifically verifying Lit template event wiring; (2) a + DOM harness would add disproportionate setup/flakiness; (3) the assertion + checks observable component/controller effects, not Lit internals; (4) the + lookup anchors to stable semantic markup/labels; (5) the test stays narrow. +- **Do NOT** use TemplateResult extraction for: general content assertions; + styling/layout/focus/keyboard/accessibility; broad user flows; scenarios with + an existing public seam; or copying an ad hoc helper variant into another file + without considering a shared helper/DOM harness. +- When the escape hatch is used: **add a short comment** explaining why it is + proportionate; keep the helper **small, type-guarded, and file-local unless + reuse is justified**; anchor to stable semantic markers; assert the caused + behavior. +- **Test helpers:** keep local until reuse is clear; if a pattern appears in + multiple files, **consolidate deliberately rather than copy-pasting variants**. +- **Determinism:** prefer controllable promises / fake timers / injected deps + over sleeps or timing guesses. + +Note on environment: `vitest.config.ts` sets **no DOM environment** (node +default). So real shadow-DOM click harnesses are not available today. This is +important context — it partially justifies handler extraction, but per the skill +it makes deliberate helper consolidation and clear scoping *more* important, not +less. The relay should treat "introduce a shared, type-guarded template-inspection +helper (and/or a jsdom/happy-dom harness option)" as the strategic fix rather +than blessing per-file copies. + +--- + +## Finding 1 — Duplicated TemplateResult-inspection helpers across many files (HIGH) + +The same private helper cluster (`templateStrings`, `templateValues`, +`isTemplateResult`, `isStringArray`, plus per-file variants like +`templateStaticMarkup`, `collectTemplateStrings`, `flattenTemplateContent`, +`templateValuesAfterMarker`, `findTemplateClickHandlerForText`) is copy-pasted +across at least these window files: + +| File | Local helper defs | Escape-hatch comment? | +| --- | --- | --- | +| `src/client/src/components/ChatView.image.test.ts` | 6 | partial | +| `src/client/src/components/ChatView.test.ts` | 6 | partial | +| `src/client/src/components/PiWebApp.clearQueue.test.ts` | 3 | **none** | +| `src/client/src/components/settings/SettingsSessiondPanel.test.ts` | 5 | **none** | +| `src/client/src/components/WorkspaceFilesPanel.test.ts` | 5 | partial | +| `src/client/src/promptAttachmentCapture.test.ts` | 4 | partial | +| `src/client/src/components/SettingsDialog.testSupport.ts` | 5 | **none** | + +Also carrying the same duplicated helpers (pre-window, but same cluster — verify +before consolidating): `settings/SettingsGeneralPanel.test.ts`, +`SettingsPluginsPanel.test.ts`, `SettingsShortcutsPanel.test.ts`, +`SettingsPackagesPanel.test.ts`, `SettingsPanelFrame.test.ts`. + +**Violation:** "If a pattern appears in multiple files, consolidate deliberately +rather than copy-pasting variants." This is now a maintenance liability (a second +mini-application of Lit-internal reflection). + +**Relay action:** Create one shared, strictly-typed, type-guarded template +inspection helper module for tests (e.g. under a test-support path), covering: +static-markup flattening, value-after-marker lookup, and event-handler-after-marker +lookup. Migrate the files above to it. Delete the per-file copies. Keep the API +minimal and documented as an escape hatch. Confirm each call site still anchors to +stable semantic markers. + +--- + +## Finding 2 — TemplateResult inspection used for general content assertions (HIGH) + +The skill explicitly forbids TemplateResult inspection for "general content +assertions" and "styling/layout" — it is only for verifying event wiring with +observable effects. + +Offending patterns: + +- **`src/client/src/components/settings/SettingsSessiondPanel.test.ts`** — uses + `flattenTemplateContent(...)` + `expectTextOrder(...)` / `countOccurrences(...)` + to assert rendered *text content and ordering* (labels like "Session daemon", + "Config file", "Companion CLI command"). This is general content/layout + assertion via Lit internals, not event-wiring verification. No escape-hatch + comment. +- **`src/client/src/components/SettingsDialog.general.test.ts`** (and the + `.packages`, `.plugins`, `.sessiond` siblings) — use + `collectTemplateStrings(dialog.render())` to assert presence/absence of markup + substrings (`" setTimeout(resolve, 5))` after emitting + `compaction_end` / `agent_start` to let the async prompt drain. Flake-prone. +- **`src/server/sessions/piSessionService.spawnSubsession.test.ts`** — ~14 sites + using `setTimeout(resolve, 20)` (and one `40`) to await async custom-message + delivery after emitting `agent_end`. Comment at line ~797 confirms these wait on + "the async custom-message path." + +Both files already have access to controllable fakes (the service is constructed +with injected collaborators). `piSessionService.lifecycle.test.ts` in the same +window uses `useFakeTimers`, showing the deterministic pattern is available here. + +**Relay action:** Replace fixed sleeps with a deterministic wait: expose/await the +service's drain promise, use an injected controllable clock / `vi.useFakeTimers()` ++ advance, or poll a condition via `vi.waitFor`. Do not merely shorten the delays. + +--- + +## Out of scope / verified clean + +- No `any`, `@ts-ignore`, or `@ts-expect-error` in window test files. +- No `.skip`/`.only`/`xit`/`xdescribe` (the `xhrs.only()` hits are a test-helper + method, not Vitest focus). +- Server/helper/route tests sampled (nativeServices, machines, workspaces file + content, sessionRoutes, parsers) follow the layer guidance and use injected + fakes; no additional violations noted beyond Finding 4. + +## Suggested relay sequencing + +1. **Slice A (strategic seam):** Add the shared, type-guarded template-inspection + test helper (Finding 1). Decide whether to also enable a jsdom/happy-dom + environment option for genuine DOM assertions. +2. **Slice B (client component tests):** Migrate each window component test to the + shared helper; reclassify assertions (Finding 2); move content/layout checks to + public seams or the render harness; add escape-hatch comments (Finding 3). + Suggested per-file order: `SettingsSessiondPanel.test.ts`, the four + `SettingsDialog.*` files + `SettingsDialog.testSupport.ts`, `PiWebApp.clearQueue.test.ts`, + `ChatView.test.ts`, `ChatView.image.test.ts`, `WorkspaceFilesPanel.test.ts`, + `promptAttachmentCapture.test.ts`. +3. **Slice C (determinism):** Replace sleeps in `piSessionService.promptQueue.test.ts` + and `piSessionService.spawnSubsession.test.ts` (Finding 4). +4. **Verify:** `npm test -- --run `, then `npm run typecheck` and + `npx eslint `; `npm run verify` before final review. + +## Checks used to produce this report + +- Window: `git log a266541..aedcbf8 -- '*.test.ts'` (added vs modified via + `--diff-filter`). +- Pattern scans over existing window files for: `Reflect.(set|get)`, + `templateStrings`/`templateValues`/`isTemplateResult`, `collectTemplateStrings`, + `flattenTemplateContent`, `setTimeout`/`sleep`, `any`, `@ts-ignore`, + `.skip/.only`, and escape-hatch comment keywords. +- Read of the representative offenders listed under each finding.