Archived
docs(test): audit testing-guide compliance gaps for fix relay
This commit is contained in:
Federico Jaramillo Martinez
@@ -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 (`"<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.
|
||||
Reference in New Issue
Block a user