docs: docs & test hygiene remainder (SL-014, 028, 031, 032, 033)

[jackgranatowski]

PR2 from the technical-debt audit remediation plan (#469): the docs/test-hygiene remainder (SL-027/029/030 already landed in batch 1, PR #474). Targets the integration branch, same stacking model as PR1 (#475).

Findings addressed

• SL-014 — added the 12 previously-undocumented npm scripts to CLAUDE.md's Key scripts table, prioritizing the two CI gates the audit specifically called out (check:macros, check:registry), plus audit:check and lint:css.
• SL-028 — added tests/README.md documenting the *.spec.js (Playwright) vs *.test.js (node:test) split, using SL-027 as the concrete example of what happens when it's unenforced.
• SL-031 — docs/migration.md's header said "0.6.25 → next"; traced the change it documents (base-alias removal) via CHANGELOG.md to the release that actually shipped it (0.6.26) and retitled accordingly.
• SL-032 — documented why tier1-p2/p7/p8/p10 skip P1, P3-P6, P9 (they trace to the tier-1-color-fallback feature's original property enumeration — only a subset got dedicated automated tests) in tests/README.md, plus a one-line pointer in each of the 4 files. Not renumbered.
• SL-033 — extracted tests/render-helpers.js. Diffed all 8 page.setContent call sites individually first, per the plan's caution: 5 files (a11y-patterns, layout, macros, states-full, typography) had near-identical setup() functions differing only in viewport size / body margin / an extra transition-disabling stylesheet — now parameterized through one renderWithBundle() helper. layers.spec.js, print.spec.js, and container-queries.spec.js are genuinely differently-shaped (per-test inline content, or a container-wrapping variant) and were left otherwise untouched — they only now share the BUNDLE path constant instead of each redefining it.

Verification

Real browser execution, not just --list: this sandbox doesn't have the exact Playwright browser build the project pins pre-installed, so I ran the 7 touched spec files (150 tests total) under a temporary local executablePath override pointing at the closest available build — all 150 pass unchanged. That temp config was never committed.

• npm run lint:css, npm run build — clean
• npm run test:unit — 31/31 pass
• npm run check:llm-guide, npm run check:version — pass

---

Generated by Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5757ac2f-302c-413b-a589-8a100cedca56

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/audit-pr2-docs-test-hygiene

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Docs/test hygiene: document scripts & conventions; dedupe Playwright setup

📝 Documentation 🧪 Tests 🕐 40+ Minutes

AI Description

• Document previously-implicit npm scripts and test-runner conventions to reduce CI drift.
• Clarify migration versioning and tier1 test numbering gaps to prevent accidental renames.
• Extract shared Playwright render/setup helper to remove duplicated bundle/viewport boilerplate.

Diagram

graph TD
  Dev["Developer"] --> Docs["Docs (CLAUDE/tests)"] --> Scripts["npm scripts"]
  Scripts --> Runners["Test runners"] --> NodeTest["node:test *.test.js"]
  Runners --> Playwright["Playwright *.spec.js"] --> Helpers["tests/render-helpers.js"] --> Bundle["slashed.optimal.css"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use Playwright fixtures (test.extend) instead of a helper

• ➕ Centralizes setup via Playwright’s idioms (fixtures, test.use viewport)
• ➕ Can enforce consistent setup automatically across specs
• ➖ More indirection for simple suites; harder to parameterize per-file quirks like body margin vs extra style without more fixture plumbing
• ➖ May require reworking existing local setup functions anyway

2. Keep per-spec setup functions but share only the BUNDLE constant

• ➕ Minimal change footprint; avoids any behavior-coupling between specs
• ➖ Leaves duplicated setViewportSize/setContent boilerplate and increases drift risk across spec files

Recommendation: The chosen approach (a small, explicit renderWithBundle helper plus shared BUNDLE/NO_TRANSITIONS_STYLE constants) is a good middle ground: it removes high-duplication boilerplate while keeping per-spec intent readable and preserving the few meaningful differences via options.

Files changed (16) +118 / -46

Tests (13) +76 / -44

a11y-patterns.spec.jsUse shared render helper and shared no-transitions style +2/-7

Use shared render helper and shared no-transitions style

• Removes local bundle path + setup boilerplate and switches to 'renderWithBundle()' with explicit viewport sizing. Uses the shared 'NO_TRANSITIONS_STYLE' constant to keep computed-style reads stable.

tests/a11y-patterns.spec.js

container-queries.spec.jsReuse shared BUNDLE constant +1/-3

Reuse shared BUNDLE constant

• Drops per-file bundle path construction and imports the shared 'BUNDLE' constant. Keeps the file’s specialized container-wrapping setup logic intact.

tests/container-queries.spec.js

layers.spec.jsReuse shared BUNDLE constant for per-test style injection +7/-7

Reuse shared BUNDLE constant for per-test style injection

• Replaces repeated 'path.join(process.cwd(), ...)' bundle construction with an import from 'render-helpers.js'. Per-test inline 'page.setContent()' structure remains unchanged.

tests/layers.spec.js

layout.spec.jsDeduplicate Playwright setup via renderWithBundle +2/-6

Deduplicate Playwright setup via renderWithBundle

• Replaces the local 'setup()' boilerplate (viewport/content/bundle load) with a call to 'renderWithBundle()' using the same viewport dimensions.

tests/layout.spec.js

macros.spec.jsDeduplicate setup while preserving body margin behavior +4/-6

Deduplicate setup while preserving body margin behavior

• Switches to 'renderWithBundle()' and explicitly sets 'bodyMargin: null' to preserve the prior behavior of omitting 'margin:0' on '<body>'. Adds an inline comment explaining the intentional difference from other specs.

tests/macros.spec.js

print.spec.jsReuse shared BUNDLE constant for print smoke tests +4/-4

Reuse shared BUNDLE constant for print smoke tests

• Imports 'BUNDLE' from the shared helper module and uses it for 'page.addStyleTag()' calls. Test flow and print media emulation remain the same.

tests/print.spec.js

render-helpers.jsAdd shared Playwright renderWithBundle helper and constants +40/-0

Add shared Playwright renderWithBundle helper and constants

• Adds a shared helper module exporting the optimal CSS bundle path, a no-transitions stylesheet constant, and 'renderWithBundle()' to standardize viewport/content wrapper/bundle attachment across Playwright specs. Designed to support small per-spec differences via options (viewport, body margin, extra style, alternate bundle).

tests/render-helpers.js

states-full.spec.jsUse shared render helper and shared no-transitions style +2/-7

Use shared render helper and shared no-transitions style

• Replaces duplicated setup logic with 'renderWithBundle()' and uses the shared 'NO_TRANSITIONS_STYLE' constant to prevent flaky mid-transition computed-style reads.

tests/states-full.spec.js

tier1-p10-contrast.test.jsAdd pointer explaining tier1 numbering gaps +3/-0

Add pointer explaining tier1 numbering gaps

• Adds a short header comment directing readers to 'tests/README.md' for the rationale behind missing P1/P3–P6/P9 files and warning against renumbering.

tests/tier1-p10-contrast.test.js

tier1-p2-coverage.test.jsAdd pointer explaining tier1 numbering gaps +3/-0

Add pointer explaining tier1 numbering gaps

• Adds a header comment referencing 'tests/README.md' for the tier1 numbering-gap explanation and guidance not to renumber.

tests/tier1-p2-coverage.test.js

tier1-p7-oldengine.test.jsAdd pointer explaining tier1 numbering gaps +3/-0

Add pointer explaining tier1 numbering gaps

• Adds a brief header note pointing to 'tests/README.md' for the tier1 numbering-gap rationale and to prevent renumbering.

tests/tier1-p7-oldengine.test.js

tier1-p8-modern.spec.jsAdd pointer explaining tier1 numbering gaps +3/-0

Add pointer explaining tier1 numbering gaps

• Adds a header comment pointing to 'tests/README.md' for why the tier1 files are numbered P2/P7/P8/P10 and should not be renumbered.

tests/tier1-p8-modern.spec.js

typography.spec.jsDeduplicate Playwright setup via renderWithBundle +2/-4

Deduplicate Playwright setup via renderWithBundle

• Removes the local BUNDLE constant and switches the setup helper to call 'renderWithBundle()' while leaving the fixture URL and typography assertions unchanged.

tests/typography.spec.js

Documentation (3) +42 / -2

CLAUDE.mdDocument additional npm scripts (CI gates, docs, test commands) +13/-1

Document additional npm scripts (CI gates, docs, test commands)

• Expands the Key scripts table to include previously undocumented npm scripts, highlighting CI-gate checks (macros/registry/audit/css lint) and common workflows (watch, unit-only tests). Also clarifies that 'npm run audit' writes 'docs/registry.json'.

CLAUDE.md

migration.mdFix migration header version range for base-alias removal +1/-1

Fix migration header version range for base-alias removal

• Retitles the migration section from “0.6.25 → next” to “0.6.25 → 0.6.26” to match the release that actually shipped the documented change and align with the file’s versioned-header style.

docs/migration.md

README.mdAdd test-runner conventions and tier1 numbering rationale +28/-0

Add test-runner conventions and tier1 numbering rationale

• Introduces a tests README documenting the '*.spec.js' (Playwright) vs '*.test.js' (node:test) split and guidance for choosing the right runner. Also explains the historical tier1 P2/P7/P8/P10 numbering gaps and instructs contributors not to renumber.

tests/README.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 6 rules

1. Misstated CI gate ✓ Resolved 🐞 Bug ≡ Correctness

Description

CLAUDE.md labels npm run check:macros as a CI gate, but the CI workflow does not run
check:macros/scripts/check-macro-catalog.js, so macro-doc drift will not be caught by CI as
implied.

Code

CLAUDE.md[54]

+| `npm run check:macros` | Verify `.sf-*` macro classes match `docs/macros.md` (CI gate) |

Relevance

⭐⭐⭐ High

Team fixes doc/CI mismatches; added real CI gate + CLAUDE.md guidance in PR #383 (also doc accuracy
fixes in #68).

PR-#383
PR-#68

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The docs explicitly mark the script as a CI gate, but the CI workflow’s “Verify all generated
artifacts” job lists the checks it runs and does not include the macro catalog check.

CLAUDE.md[46-69]
.github/workflows/ci.yml[74-96]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`CLAUDE.md` says `npm run check:macros` is a CI gate, but `.github/workflows/ci.yml` does not run that check. This is misleading and can cause contributors to assume CI enforces macro-catalog correctness when it currently doesn't.

### Issue Context
You can fix this either by:
1) updating docs to remove/soften the “CI gate” claim, **or**
2) making it a real CI gate by adding the macro check to the CI workflow.

### Fix Focus Areas
- CLAUDE.md[54-54]
- .github/workflows/ci.yml[74-96]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. test:unit needs build ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

CLAUDE.md documents npm run test:unit without noting that some unit tests require build
artifacts in badges/ and will fail on a clean checkout unless npm run build (or CI’s badges
artifact download) ran first.

Code

CLAUDE.md[68]

+| `npm run test:unit` | Run the Node `--test` unit suite only, without the Playwright e2e build |

Relevance

⭐⭐ Medium

Similar concern (“test:unit” needs build output) was explicitly rejected in PR #421; docs-only
clarification might still be accepted.

PR-#421

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

A unit test explicitly fails if badges/slashed.full.css is missing and tells the user to run `npm
run build first, while test:unit` does not invoke build; CI works around this by downloading the
badges artifact before running test:unit.

CLAUDE.md[66-69]
tests/tier1-p7-oldengine.test.js[20-75]
package.json[34-57]
.github/workflows/ci.yml[139-161]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`npm run test:unit` runs tests that require built CSS artifacts under `badges/` (e.g., `badges/slashed.full.css`). The new CLAUDE.md entry can mislead contributors into thinking `test:unit` will work from a clean checkout without building first.

### Issue Context
`test:unit` does not run the build step itself; CI compensates by downloading the `badges/` artifact before running `test:unit`.

### Fix Focus Areas
- CLAUDE.md[68-69]
- package.json[54-57]
- tests/tier1-p7-oldengine.test.js[20-75]
- .github/workflows/ci.yml[139-160]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---