Skip to content

test(replay-test): cover failFast, retry exhaustion, plan-prep failure, JUnit escaping - #1550

Merged
thymikee merged 1 commit into
mainfrom
claude/peaceful-wu-3cb903
Aug 2, 2026
Merged

test(replay-test): cover failFast, retry exhaustion, plan-prep failure, JUnit escaping#1550
thymikee merged 1 commit into
mainfrom
claude/peaceful-wu-3cb903

Conversation

@thymikee

@thymikee thymikee commented Aug 2, 2026

Copy link
Copy Markdown
Member

Summary

Closes four untested failure paths in the replay test harness, flagged by the 2026-08-01 test-strength audit of packages/replay-test:

  1. failFast — no test anywhere set request.failFast=true. Added a test with 3 explicit entries where the first fails: asserts the suite invokes only the first entry, and notRun accounts for the other two (shouldStopReplayTestExecution in packages/replay-test/src/internal/session-test.ts).
  2. Retry exhaustion — every existing retry test ends in a pass. Added a case with retries: 2 where every attempt fails: asserts attempts === 3 exactly, guarding the attemptIndex <= params.retries loop bound in packages/replay-test/src/internal/session-test-attempt.ts.
  3. Suite-level plan-prep failure — fed an unparseable .ad source (env BROKEN — missing =) through discovery so readReplayScriptMetadata throws before any entry is runnable. Asserts the suite-level try/catch (packages/replay-test/src/internal/session-test.ts ~L73-132) converts the thrown AppError into {ok: false, error: {code: 'INVALID_ARGS', ...}} instead of letting it escape.
  4. JUnit XML escapingsrc/replay/test/reporters/junit.ts had no direct test, only reporter-spec parsing. Added src/replay/test/reporters/__tests__/junit.test.ts: writes a suite with a failing case whose title/message contain <, &, ", and a newline, then parses the written file back with @agent-device/xml's parseXmlDocumentSync and asserts the values round-trip.

All four tests were verified red per docs/agents/testing.md's counterfactual rule — the production condition was temporarily broken, the test observed failing, then restored:

  • failFast: disabled the failFast branch in shouldStopReplayTestExecutioninvoked.length went from expected 1 to actual 3 (notRun from 2 to 0).
  • retry exhaustion: changed the loop bound from attemptIndex <= params.retries to attemptIndex < params.retriesattemptCount went from expected 3 to actual 2.
  • plan-prep failure: moved prepareReplayTestSuitePlan outside the try/catch → the AppError escaped uncaught instead of the daemon returning {ok: false}.
  • JUnit escaping: dropped escapeXmlTextAndAttribute on the testcase name attribute → parseXmlDocumentSync threw Missing value for XML attribute "quoted". on the tricky title.

Design question for @thymikee

While writing the retry-exhaustion test I hit a contract asymmetry in packages/contracts/src/replay.ts: ReplaySuiteTestPassed carries attemptFailures (so a flaky-then-passed test reports its earlier failed attempts), but ReplaySuiteTestFailed does not. So when every attempt fails, only the last attempt's error survives in the result — the earlier attempts' failure messages are silently dropped. I did not change this (out of scope, and it's an API-widening decision), but flagging it: should ReplaySuiteTestFailed also carry attemptFailures for the attempts prior to the final one?

Test plan

  • npx vitest run packages/replay-test src/daemon/handlers/__tests__/session-test-suite.test.ts src/replay/test — 75 tests passed
  • pnpm typecheck
  • pnpm lint
  • pnpm format:check
  • All four new tests independently verified red before the fix/restore (see above)

…e, and JUnit escaping

Closes four untested failure paths flagged by the 2026-08-01 test-strength
audit of packages/replay-test:

- request.failFast=true now has a test proving the suite stops after the
  first failure and leaves the rest in the notRun bucket.
- Retry exhaustion (every attempt fails) is pinned to attempts === maxAttempts,
  guarding the attemptIndex <= retries loop bound.
- A malformed .ad source (bad env directive) is fed through discovery to
  prove the suite-level try/catch in session-test.ts converts the thrown
  AppError into a {status:'failed'} response instead of escaping uncaught.
- The JUnit reporter's escaping is exercised directly for the first time,
  round-tripping a title/message containing <, &, ", and a newline through
  parseXmlDocumentSync.

Each test was verified red: the production condition was temporarily broken,
the test observed failing, then the code was restored (see PR body for the
four before/after runs).
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.93 MB 1.92 MB -8.4 kB
JS gzip 617.9 kB 615.6 kB -2.4 kB
npm tarball 736.3 kB 733.7 kB -2.6 kB
npm unpacked 2.58 MB 2.57 MB -9.2 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.4 ms 25.8 ms -0.5 ms
CLI --help 63.8 ms 55.6 ms -8.2 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/selector-vocabulary.js -4.2 kB -1.3 kB
dist/src/registry.js -2.6 kB -600 B
dist/src/cli.js -745 B -198 B
dist/src/session.js -176 B -69 B
dist/src/selector-runtime.js 0 B +2 B

@thymikee

thymikee commented Aug 2, 2026

Copy link
Copy Markdown
Member Author

Reviewed exact head 5fc3bee6. Clean: the fail-fast, retry-exhaustion, plan-preparation failure, and JUnit escaping tests exercise the real handler/reporter paths and each has a meaningful counterfactual. No code findings; all exact-head checks are green. Ready for human review/merge.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 2, 2026
@thymikee
thymikee merged commit 634073a into main Aug 2, 2026
30 checks passed
@thymikee
thymikee deleted the claude/peaceful-wu-3cb903 branch August 2, 2026 06:41
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-02 06:41 UTC

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

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant