Reduce remaining structural and duplicated tests after #159#167
Conversation
Drop metadata-heavy CLI and registry assertions so the suite keeps behavioral boundary coverage without mirroring command registration details.
There was a problem hiding this comment.
QA Review — PR #167 / Issue #164
Decision: APPROVED
What I verified
- PR body first line is exactly
Ref: #164✓ - Head branch is
issues/orfe-164✓ - No
Closes/Fixesauto-close keywords ✓ npm run typecheck— clean, no errors ✓npm run lint— clean, no errors ✓npm run build— clean, no errors ✓npm test(changed files only,test/cli/run.test.ts+test/core/command-registry.test.ts) — 13/13 passed ✓- Full test suite started cleanly before timeout; no signal of failures in the newly changed files ✓
- No source code changes, no docs/ADR/debt updates required (issue explicitly noted "Docs impact: none expected") ✓
Blockers
None.
Important
None.
Nice to have
test/core/command-registry.test.tsfilename drift: The file is still namedcommand-registry.test.tsbut its tests now exerciserunOrfeCorerather than the registry API directly. A future rename tocore-run.test.tsorcore-runtime.test.tswould improve discoverability. Not a blocker — the file is still scoped to core behavior and the reshaping was the right call.
Docs / invariants
No architecture, ADR, or debt doc changes needed. This is test-only cleanup. Issue scope confirmed "no docs impact expected." Nothing in docs/architecture/invariants.md is touched by this change.
Change assessment
test/cli/run.test.ts — two describe blocks removed:
runCli renders help for each command group— looped over all groups asserting help string literals (group name, Usage header, Commands header, purpose lines). Classic structural metadata loop. Correct to remove perdocs/architecture/testing.md§"Low-value test patterns to avoid" (duplicated routing/help assertions, shallow metadata tests).runCli renders leaf help for every agreed V1 command— looped over all commands asserting definition literals (name, purpose, usage, success summary, examples, JSON shape example). Also a structural metadata loop asserting implementation definitions rather than runtime behavior. Correct to remove.- The
escapeForRegExphelper and associated registry imports were no longer needed and were cleaned up properly. - Remaining 11 tests all protect real behavioral contracts: root help output, empty-args noop path,
--version(no-config path),runtime info(no-config path),-valias rejection, unknown command errors, caller identity requirement,ORFE_CALLER_NAMEenv var parsing, deprecated--caller-namerejection, malformed numeric option errors, malformed repo override errors. All of these are behavioral boundary tests aligned withdocs/architecture/testing.md§"CLI boundary."
test/core/command-registry.test.ts — reshaped, not deleted:
- Removed 5 structural tests: registration order, group derivation logic,
executionproperty metadata, definition reference equality loop, and low-levelgetCommandDefinitionthrow test. All were asserting internal registry structure rather than runtime behavior. - Added 2 behavioral tests exercising
runOrfeCoreat the real core runtime boundary:runOrfeCore executes runtime-only commands without caller, config, auth, or GitHub access— verifies thatruntime infoexecutes end-to-end viarunOrfeCorewith no config/auth loading. Strong behavioral test that protects the "runtime-only commands bypass dependency loading" contract. Throws inside bothloadRepoConfigImplandloadAuthConfigImplto actively assert the no-load invariant.runOrfeCore rejects unknown commands before loading config or auth— verifiesOrfeErrortype,invalid_usagecode, and message content for unknown commands, and actively asserts that config/auth loading does not occur. Stronger protection than the old registry-level throw test because it exercises the public dispatch boundary.
- Both new tests are correctly placed at the "Core runtime boundary" per
docs/architecture/testing.md§"Preferred test surfaces."
Coverage note: runtime info is now tested at two distinct layers — the CLI layer (run.test.ts, line 59-86, tests the full CLI path with env: {} stub) and the core layer (command-registry.test.ts, tests runOrfeCore directly). This is appropriate multi-boundary coverage, not duplication, because each layer protects a different surface.
Acceptance criteria assessment
- ✅ No clearly duplicated non-help command test layer remains without boundary-specific justification
- ✅
test/cli/run.test.tsno longer over-indexes on structural metadata/layout assertions (both help-loop describes removed) - ✅
test/core/command-registry.test.tsreshaped to protect meaningful runtime behavior rather than internal registration structure - ✅ Remaining tests are explainable by the behavioral integration-first strategy from #159 and the governing decision rule in
docs/architecture/testing.md
There was a problem hiding this comment.
Re-QA Review — rename follow-up (commit 3a4cd6d)
Decision: APPROVED
What I verified
- Commit
3a4cd6dis a pure rename:R100similarity, 0 insertions, 0 deletions. No logic or test content was changed. - Old name
test/core/command-registry.test.tsis gone — confirmed viagit diff origin/main...origin/issues/orfe-164 --name-status. No remaining references to the old filename exist anywhere in the codebase. - New name
test/core/core-run.test.tsis accurate: the file imports and exercisesrunOrfeCorefromsrc/core/run.js. The name directly reflects the module under test. - Test discovery:
vitest.config.tsincludestest/**/*.test.ts— the new filename matches the discovery pattern without any config change needed. - Acceptance criteria: Issue #164 requires that
test/core/command-registry.test.tsbe "removed, reduced, or reshaped so it protects meaningful behavior rather than internal structure." The rename satisfies the letter and spirit of that criterion: the misleading registry-structure name is gone, the meaningful behavioral tests (runOrfeCoreruntime-only execution and unknown-command rejection) are preserved and now named for what they actually test.
Blockers
None.
Important
None.
Nice to have
None.
Clean follow-up. No blockers introduced.
Ref: #164
Summary
Testing