Archived
233 lines
12 KiB
Markdown
233 lines
12 KiB
Markdown
# 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 (`"<settings-general-panel"`, `"scope-note"`, `"This tab edits:"`).
|
||
General content assertions through template-string reflection. No comment.
|
||
- **`src/client/src/components/ChatView.image.test.ts` / `ChatView.test.ts`** —
|
||
mix legitimate handler-extraction (the `@load`/`@click`/Clear-queue wiring) with
|
||
static-markup content assertions (`templateStaticMarkup(...)` checking
|
||
`loading="lazy"`, `role="button"`, `class="msg tool-image-output"`, titles).
|
||
The wiring assertions may be justified; the markup/attribute assertions are the
|
||
kind of content/styling checks the skill steers away from.
|
||
|
||
**Relay action:** For each, decide per assertion:
|
||
- If it verifies **event wiring** with an observable effect → keep, but route
|
||
through the shared helper (Finding 1) and add the required comment (Finding 3).
|
||
- If it is **content/text/attribute/ordering** → move to a public boundary. Where
|
||
a public seam already exists, prefer it. Options: assert the public
|
||
method/controller output (e.g. `chatQueuedMessageSections` is already tested
|
||
this way in `ChatView.test.ts` — extend that pattern), set public properties and
|
||
assert observable state, or add a minimal jsdom/happy-dom render check.
|
||
- If no lower/public seam exists and content coverage is genuinely needed,
|
||
flag for a small render-harness (see strategic note) rather than string-scraping
|
||
Lit internals.
|
||
|
||
---
|
||
|
||
## Finding 3 — Missing required escape-hatch comments (MEDIUM)
|
||
|
||
When the extraction escape hatch is used, the skill requires a short comment
|
||
explaining why it is proportionate. Window files using extraction with **no such
|
||
comment**:
|
||
|
||
- `src/client/src/components/PiWebApp.clearQueue.test.ts`
|
||
- `src/client/src/components/settings/SettingsSessiondPanel.test.ts`
|
||
- `src/client/src/components/SettingsDialog.general.test.ts`
|
||
- `src/client/src/components/SettingsDialog.packages.test.ts`
|
||
- `src/client/src/components/SettingsDialog.plugins.test.ts`
|
||
- `src/client/src/components/SettingsDialog.sessiond.test.ts`
|
||
- `src/client/src/components/SettingsDialog.testSupport.ts` (shared helper module
|
||
— should carry the canonical explanation once consolidated)
|
||
|
||
Files that already include a proportionality comment (use as the style
|
||
reference): `ChatView.image.test.ts`, `ChatView.test.ts`,
|
||
`WorkspaceFilesPanel.test.ts`, `promptAttachmentCapture.test.ts`.
|
||
|
||
**Relay action:** After Findings 1–2 resolve which tests legitimately keep
|
||
extraction, add a concise comment at each remaining extraction site (or once on
|
||
the shared helper) explaining why a DOM harness is disproportionate here.
|
||
|
||
---
|
||
|
||
## Finding 4 — Sleep-based timing instead of deterministic waits (MEDIUM)
|
||
|
||
The skill: "prefer controllable promises, fake timers, and explicit injected
|
||
dependencies over sleeps or timing guesses." Real wall-clock sleeps found:
|
||
|
||
- **`src/server/sessions/piSessionService.promptQueue.test.ts`** — lines ~201,
|
||
~211: `await new Promise((resolve) => 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 <changed files>`, then `npm run typecheck` and
|
||
`npx eslint <changed files>`; `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.
|