test(explanations): add coverage for ExplanationGenerator branches#77
Conversation
Closes #76. Adds 30 tests covering all public methods (generate, formatForUser, formatForAudit) and every branch of the private helpers (buildSummary, buildConfidenceReasoning, buildActionRationale, buildCorrectionGuidance, gatherEvidenceReferences, gatherPreferenceReferences). Includes an in-memory and a rejecting ExplanationRepositoryPort fake, plus reusable fixture builders. Pins current behavior — no implementation changes. Catches the escalationRationale-undefined → autoExecuted=true derivation in formatForAudit so future intent changes are explicit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds comprehensive Vitest coverage for @skytwin/explanations’s ExplanationGenerator, locking in current behavior across generation, formatting, and helper-branch logic without changing implementation code.
Changes:
- Introduces fixture builders for
DecisionObject,DecisionContext,DecisionOutcome,Preference,RiskAssessment, and repository fakes. - Adds a 30-test suite covering
generate,formatForUser,formatForAudit, and all listed branch paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/explanations/src/__tests__/fixtures.ts |
Adds reusable builders and in-memory/rejecting repository fakes used by the new test suite. |
packages/explanations/src/__tests__/explanation-generator.test.ts |
Adds full branch-coverage tests for ExplanationGenerator generation + user/audit formatting outputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| async getByUserId(userId: string, limit?: number): Promise<ExplanationRecord[]> { | ||
| const matches = this.saved.filter((r) => r.userId === userId); | ||
| return limit ? matches.slice(0, limit) : matches; |
There was a problem hiding this comment.
InMemoryExplanationRepo.getByUserId treats limit=0 as “no limit” because it uses a truthiness check (limit ? ...). If any caller passes 0 (e.g., to request none), this will incorrectly return all matches. Use an explicit undefined check (or a default parameter) so 0 is handled correctly.
| return limit ? matches.slice(0, limit) : matches; | |
| return limit === undefined ? matches : matches.slice(0, limit); |
| const record = await gen.generate(makeDecision(), makeOutcome({ autoExecute: true }), makeContext()); | ||
|
|
There was a problem hiding this comment.
Several tests call gen.generate(makeDecision(), ..., makeContext()) without wiring the same decision instance into context.decision. Since generate() uses both the decision argument (summary) and context.decision (evidence references), this can accidentally create internally inconsistent records and may mask bugs if the fixtures ever diverge. Prefer constructing one decision and passing it to both generate() and makeContext({ decision }) for clarity and correctness.
…h work (#87) Captures eight PRs (#77, #78, #79, #81, #82, #83, #84, #85, #86) under an [Unreleased] section. No code changes — pure documentation. Bumping VERSION + cutting a tag is left as a separate decision so the release moment stays explicit. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Summary
@skytwin/explanationscovering every branch ofExplanationGenerator— public surface (generate,formatForUser,formatForAudit) plus all six private helpers__tests__/fixtures.ts) with builders forDecisionObject,DecisionContext,DecisionOutcome,Preference,RiskAssessment,CandidateAction, plus in-memory and rejectingExplanationRepositoryPortfakesformatForAuditautoExecuted = !escalationRationalederivation so future intent changes have to be explicitTest plan
Closes #76
🤖 Generated with Claude Code