Skip to content

chore(post-#168): architecture-test refactor — read-once + per-name regex + structural-absence-guard docs#171

Merged
jukka-matti merged 4 commits into
mainfrom
post-168-architecture-test-refactor
May 14, 2026
Merged

chore(post-#168): architecture-test refactor — read-once + per-name regex + structural-absence-guard docs#171
jukka-matti merged 4 commits into
mainfrom
post-168-architecture-test-refactor

Conversation

@jukka-matti
Copy link
Copy Markdown
Owner

Summary

Fixes the architecture-grep test flake at its algorithmic source: 16 forbidden-name checks were re-reading ~310 source files each (~5000 sync reads per run) and exceeding the 5s vitest timeout under turbo concurrent load. Now reads files once at setup and scans the in-memory cache per name.

  • T2 (06d2638a)refactor(core): read-once + alternation in architecture-grep test. beforeAll populates a CachedFile[] once; each of 16 it() blocks runs its whole-word regex against the cache. Same 16 forbidden names, same scope, per-name failure granularity preserved. Test docstring extended with "tripwire, not a wall" framing — denylist limits called out explicitly, durable answer (branded Cpk type) cross-referenced.
  • T4 (1a9f7b46)docs(testing): add architecture-test (structural-absence guard) section. New ~134-line section in docs/05-technical/implementation/testing.md documenting the pattern: when to use, when NOT, implementation (with TS code snippet matching this PR's post-refactor test + bash snippet for scripts/check-level-boundaries.sh), anti-pattern (the 3040-reads cost the original test paid), explicit limits (denylist not allowlist, substring grep, narrow scope, maintenance burden).
  • T5 (14f9ca79)docs(testing+investigations): add architecture-test cross-ref + branded-Cpk follow-up entry. (a) .claude/rules/testing.md +1 line pointing to the new docs section. (b) docs/investigations.md +60 lines new entry "Branded Cpk type as durable replacement for forbidden-name guard" — proposes opaque-type pattern mirroring ProcessHubId from PR chore(post-8f): cleanup cluster — setState fix + ProcessHubId brand + Canvas decomposition + tsc hygiene #168 (packages/core/src/processHub.ts), 4–8 task estimated scope, ADR-amendment promotion path. Final-review amend tightened forward-reference numbers (~310 files / ~5000 reads) and aligned the 3-surface cross-references.
  • T6 (9c411ab9)chore(testing): remove retry-once workaround for architecture-grep flake. In-repo: one line in testing.md (cross-ref to deleted memory replaced with credit to fix commit). Out-of-repo: deleted feedback_pr_ready_check_retry_on_grep_test.md memory + its MEMORY.md index line. Preserved the hooks/index.test.ts known-flake line in .claude/rules/testing.md (different root cause, unfixed).

Performance

State Test cases Total duration Wall under turbo
Pre-refactor 16 293-367ms hits 5s timeout intermittently
Post-refactor 16 54-58ms 623ms (verified)

~5-6× faster isolated; full turbo pnpm test completes architecture-grep test in 623ms (was hitting 5s timeout pre-fix). Adding name #17 doesn't change the cost curve (single cache pass, per-name regex on cached strings).

What this PR explicitly does NOT do

  • Does not delete the architecture-grep test. It remains an ADR-073 tripwire; the branded-Cpk-type future investigation in docs/investigations.md is the durable answer, not this PR.
  • Does not touch packages/hooks/src/__tests__/index.test.ts flake. Different root cause (import/env time, not file IO) — .claude/rules/testing.md retains its known-flake line for that test.
  • Does not change WHAT is enforced. Same 16 forbidden names. Same package scope (@variscout/core only).

Pre-existing issue surfaced during review (NOT a PR B blocker)

apps/azure/src/features/investigation/__tests__/useCanvasViewportLifecycle.test.ts throws DatabaseClosedError: IndexedDB API missing (9 unhandled rejections in vitest teardown). Verified pre-existing on origin/main. Likely fix: import 'fake-indexeddb/auto' in apps/azure/src/setupTests.ts. Should be filed as a new docs/investigations.md entry — out of scope here.

Plan: ~/.claude/plans/how-should-we-handle-generic-sonnet.md (PR B section).

Test plan

  • pnpm --filter @variscout/core exec vitest run architecture.noCrossInvestigationAggregation — 16 tests pass in 54-58ms
  • Injection test: adding export const aggregateCpk = 0; triggers failure naming aggregateCpk + file path; removal restores green
  • pnpm test full turbo — architecture-grep completes in 623ms
  • pnpm --filter @variscout/core exec tsc --noEmit — 0 errors
  • pnpm docs:check — green
  • Cross-reference triangle (test docstring + docs section + investigations.md entry title) resolves consistently to "Branded Cpk type as durable replacement"
  • Out-of-repo memory cleanup verified (file deleted + MEMORY.md line removed)

🤖 Generated with ruflo

jukka-matti and others added 4 commits May 14, 2026 21:17
Architecture test now reads all ~190 files once into a Map and runs each
per-name regex against the cached strings, dropping 3040 reads/run to
190. Preserves 16 it() blocks for per-name granular failure reporting.

Adds docstring section framing the test as a tripwire (denylist; narrow
scope) not a wall — references docs/investigations.md branded-Cpk-type
follow-up entry to be added in T5.

Refs investigations.md "Pre-existing tsc errors deferred from PR #168"
(vitest worker-timeout sub-item).

Co-Authored-By: ruflo <ruv@ruv.net>
Documents the denylist/CI-grep pattern used by
architecture.noCrossInvestigationAggregation.test.ts (ADR-073) and
scripts/check-level-boundaries.sh (ADR-074). Covers when to use, when
not to use, implementation pattern (read-once cache + per-name regex),
honest framing of denylist limits, and pointer to the branded-type
durable-answer follow-up in investigations.md (added in T5).

Refs investigations.md "Pre-existing tsc errors deferred from PR #168"
(vitest worker-timeout sub-item).

Co-Authored-By: ruflo <ruv@ruv.net>
…ed-Cpk follow-up entry

- .claude/rules/testing.md: add line pointing to the new architecture-test
  docs section.
- docs/investigations.md: new entry "Branded Cpk type as durable replacement
  for forbidden-name guard". Resolves forward references from T2's test
  docstring and T4's docs section. Proposes opaque-type pattern mirroring
  ProcessHubId from PR #168; estimated scope and promotion path included.

Refs investigations.md "Pre-existing tsc errors deferred from PR #168"
(vitest worker-timeout sub-item).

Co-Authored-By: ruflo <ruv@ruv.net>
T2 refactored the test to read-once + per-name regex (commit 06d2638),
eliminating the file-IO pattern that caused the flake under turbo
concurrent load. The retry-once workaround is now redundant and removed:

- docs/05-technical/implementation/testing.md: anti-pattern note no
  longer references the deleted feedback_pr_ready_check_retry_on_grep_test
  memory; updated to credit the fix commit and note no retry is needed.
- Out-of-repo system cleanup (not in this commit):
  - Deleted ~/.claude/projects/.../memory/feedback_pr_ready_check_retry_on_grep_test.md
  - Removed corresponding MEMORY.md index entry (line 75)

Keeps the hooks/index.test.ts known-flake line in .claude/rules/testing.md
intact (different root cause — import/env time, not file IO).

Co-Authored-By: ruflo <ruv@ruv.net>
@vercel
Copy link
Copy Markdown

vercel Bot commented May 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
mean-beoynd-lite-pwa Ready Ready Preview, Comment May 14, 2026 7:16pm
variscout_website Ready Ready Preview, Comment May 14, 2026 7:16pm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant