test(replay): split session-replay-vars.test.ts by actual subject (#1460) - #1468
Merged
Conversation
) session-replay-vars.test.ts contained zero references to buildReplayBuiltinVars (the module it was named after) — it actually held tests of replay/vars.ts, replay/script.ts, and runReplayScriptFile. Redistribute each test to where its subject actually lives: - vars.ts unit tests move to new src/replay/__tests__/vars.test.ts - script.ts env-directive/actionLines tests move into script.test.ts (none were duplicates of existing coverage there) - runReplayScriptFile tests merge into a new sibling, session-replay-runtime-maestro.test.ts, since they depend on a vi.mock('../../../core/dispatch.ts') configuration incompatible with the one already in session-replay-runtime.test.ts - one genuine duplicate (`--update` env-directive regression test) is deleted in favor of the equivalent already in session-replay-runtime.test.ts No production code changed.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
The Fallow Code Quality gate flagged the "dispatches resolved literals with file env overridden by CLI" test in the new session-replay-runtime-maestro.test.ts for exceeding the cyclomatic complexity threshold, since fallow audits it as newly-added code in this file. Extract the unresolved-interpolation check into a helper and destructure the captured calls to cut branching, with identical assertions.
Member
Author
|
Reviewed clean at The split keeps each replay test with its actual subject: vars and script coverage is co-located, and the runtime/Maestro sibling preserves the necessary isolated dispatch mock. The 66-block disposition is accounted for (one genuine duplicate removed); all required checks are green. Ready for human review. |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1460.
session-replay-vars.test.tscontained zero references tobuildReplayBuiltinVars(the module it was named after — that module's real test issession-replay-builtin-vars.test.ts, untouched by this PR). It actually held tests ofsrc/replay/vars.ts,src/replay/script.ts, andrunReplayScriptFile(src/daemon/handlers/session-replay-runtime.ts). This PR redistributes each of the file's 66 test blocks to where its subject actually lives, and deletes the original file. No production code was changed.Discrepancy vs. the issue's estimate
The issue guessed "~15 tests...are pure duplication" against
script.test.ts. That guess was checked per-test and found incorrect:script.test.tshad zero coverage of theenvdirective feature or ofparseReplayScriptDetailed'sactionLines, so all 6 script.ts-subject tests moved as genuinely new coverage, none deleted.Across the whole file, only 1 test total was deleted, as a genuine duplicate of existing
session-replay-runtime.test.tscoverage.Disposition tally
test.each) → moved to newsrc/replay/__tests__/vars.test.tssrc/replay/__tests__/script.test.tssrc/daemon/handlers/__tests__/session-replay-runtime-maestro.test.tsWhy a new sibling file instead of merging into
session-replay-runtime.test.tsThe 43
runReplayScriptFiletests all depend on arunReplayFixturehelper that requiresvi.mock('../../../core/dispatch.ts', ...)mockingresolveTargetDevice(realistic android/ios device objects) anddispatchCommand(rejecting, to keep post-failure screen-digest-capture paths deterministic).session-replay-runtime.test.tsalready has its own, differently-shapedvi.mockof the same module (dispatchCommandresolves{},resolveTargetDeviceis a barevi.fn()). Vitest allows only onevi.mockper module per file, so merging would require reconciling two incompatible mock configurations — out of scope for a pure test-file split per the issue's "keep the move minimal and mechanical" instruction. The new sibling file carries therunReplayFixturehelper and itsvi.mockover verbatim.That sibling file also carries a handful of generic (non-Maestro)
.adrunReplayScriptFiletests alongside the Maestro-heavy majority, since they share the same fixture helper; this is called out in a comment at the top of the file.Per-test itemized disposition
Moved to
src/replay/__tests__/vars.test.ts(16 blocks / 19 cases)resolveReplayString substitutes variablesresolveReplayString supports fallback with :-defaultresolveReplayString prefers scope value over fallbackresolveReplayString fallback preserves embedded braces via escapesresolveReplayString throws on unresolved variable with file:lineresolveReplayString is case-sensitiveresolveReplayString substitutes multiple vars on one linebuildReplayVarScope precedence: cli > shell > file > builtincollectReplayShellEnv strips AD_VAR_ prefix and ignores other varscollectReplayShellEnv skips keys that land in reserved AD_* namespace after stripparseReplayCliEnvEntries splits KEY=VALUE and rejects invalid keysresolveReplayAction walks positionals and string flagsresolveReplayAction walks runtime hintsresolveReplayAction produces dispatch-ready literals for a realistic fixturetest.eachblockrejects AD_* as reserved namespace in $name(4 cases: file env viareadReplayScriptMetadata, CLI viaparseReplayCliEnvEntries,buildReplayVarScope.fileEnv,buildReplayVarScope.cliEnv) — kept together as one cross-cutting invariant rather than split across filesparseReplayCliEnvEntries error wording is user-friendly for invalid keysMoved to
src/replay/__tests__/script.test.ts(6 blocks — verified NOT duplicates;script.test.tshad no priorenv/actionLinescoverage)parseReplayScriptDetailed tracks line numbers(theactionLinesarray)readReplayScriptMetadata parses env KEY=VALUE directivesreadReplayScriptMetadata accepts env before contextreadReplayScriptMetadata parses quoted env values with spacesreadReplayScriptMetadata rejects invalid env keyreadReplayScriptMetadata rejects duplicate env keyDeleted as duplicate (1 block)
--update no longer rejects scripts with env directives (the guard existed only for rewrite safety)— deleted as duplicate ofsession-replay-runtime.test.ts's--update no longer refuses env directives (the guard existed only for rewrite safety)(same guard-removal regression, same ADR 0012 migration-step-6 context). The one extra assertion in the deleted test (that variable substitution completed,calls[0].positionals === ['settings']) is not distinct coverage — substitution itself is thoroughly covered by the vars.ts unit tests moved tovars.test.ts.Moved to new
src/daemon/handlers/__tests__/session-replay-runtime-maestro.test.ts(43 blocks — all new coverage, zero duplicates of the existing 17-test/18-call-sitesession-replay-runtime.test.ts)--update no longer rejects Maestro compat flow controls (the guard existed only for rewrite safety)runReplayScriptFile dispatches resolved literals with file env overridden by CLI.ad replay normalizes resolved gesture and swipe syntax into structured daemon inputrunReplayScriptFile reports snapshot diagnostics from per-action session samplesrunReplayScriptFile reports snapshot diagnostics on replay failurerunReplayScriptFile applies CLI env overrides before Maestro compat mappingrunReplayScriptFile runs Maestro runScript in replay order and exposes output variablesrunReplayScriptFile supports successful Maestro runScript http.post callsrunReplayScriptFile strips prototype pollution keys from runScript json()runReplayScriptFile reports Maestro runScript failures at the runScript steprunReplayScriptFile reports iOS Maestro openLink setup failures before assertionsrunReplayScriptFile explains empty Maestro runScript JSON bodiesrunReplayScriptFile rejects Maestro runScript output keys containing dotsrunReplayScriptFile retries Maestro scrollUntilVisible with scroll probesrunReplayScriptFile uses semantic iOS dispatch for an exact text tapOnrunReplayScriptFile uses matched Android id geometry for tapOnrunReplayScriptFile captures fresh geometry for tapOn after assertVisiblerunReplayScriptFile scopes duplicate tap targets after native Maestro assertVisiblerunReplayScriptFile treats absent Maestro assertNotVisible targets as passingrunReplayScriptFile propagates Maestro assertNotVisible infrastructure failuresrunReplayScriptFile waits briefly for Maestro assertNotVisible to stabilizerunReplayScriptFile treats absent Maestro extendedWaitUntil.notVisible targets as passingrunReplayScriptFile resolves Maestro percentage point taps from the direct viewportrunReplayScriptFile retries Maestro id tapOn through snapshot coordinatesrunReplayScriptFile resolves Maestro tapOn index and childOf from snapshotsrunReplayScriptFile lets snapshot id tap handle Maestro one-point edge controlsrunReplayScriptFile resolves a text-entry target once before typingrunReplayScriptFile resolves Maestro swipe.label from a labeled element rectrunReplayScriptFile keeps Maestro swipe.label anchored to the matched label rectrunReplayScriptFile resolves Maestro screen swipes from the direct viewportrunReplayScriptFile delegates Android directional swipes and preserves percentage pointsrunReplayScriptFile maps Maestro enter to keyboard enterrunReplayScriptFile waits for Maestro animation screenshots to stabilizerunReplayScriptFile propagates unsupported keyboard enter dispatchrunReplayScriptFile retries Maestro retry commands until they passrunReplayScriptFile propagates Maestro runFlow.when runtime errorsrunReplayScriptFile runs Maestro runFlow.when.visible commands when presentrunReplayScriptFile runs nested Maestro runtime commands inside runFlow.whenrunReplayScriptFile resolves nested Maestro runFlow.when command variables once at executionrunReplayScriptFile reads shell env from request (client-collected), not daemon process.envrunReplayScriptFile falls back to process.env when request omits replayShellEnvrunReplayScriptFile writes per-action timing events to active traceAD_ARTIFACTS resolves to per-attempt dir when artifactsDir flag is set by the test runnerVerification
pnpm test:unit— 524 test files, 4659 tests passed, 4 skipped, no failures.pnpm check:tooling— lint, typecheck, layering, depgraph, production-exports, mcp-metadata, build, bundle-owner-files all pass.session-replay-builtin-vars.test.ts(the correctly-namedbuildReplayBuiltinVarstest) is untouched.Per the issue: this is a mechanical move only. A later PR (#1451) will restructure this area further.
Generated by Claude Code