feat(test): PTY-based interactive CLI snapshot harness#2052
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
How to use the Graphite Merge QueueAdd the label auto-merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
|
||
| vp-specific additions with no `vtt` counterpart: `vpt json-edit <file> <dot-path> <value>` (the existing snap-tests `json-edit` helper for fixture manifest edits) and `vpt chmod`. | ||
|
|
||
| Reusing `vtt` itself was considered and rejected. Cargo git dependencies provide library code only, never a dependency's binaries, so obtaining the `vtt` executable would require an out-of-band `cargo install --git` pinned in lockstep with the other vite-task git deps across local dev, CI, and nextest archives. Reusing it as a library would mean depending on `vite_task_bin` and dragging the entire `vt` product tree (task engine, TUI, server, fspy) into the harness build for a handful of trivial helpers. And vp-specific subcommands would then need upstream PRs plus dep bumps before tests here could use them. If the duplication ever becomes a maintenance burden, the designated path is upstream extraction: vite-task moves the subcommands into a small library crate (as `pty_terminal` already is for the emulator) and `vtt`/`vpt` become thin bin wrappers over it. |
There was a problem hiding this comment.
@wan9chi Please help me see how to make vt reusable or inheritable/extendable by vpt, so that vpt only needs to add additional commands without duplicating the code implementation.
Implements rfcs/interactive-snapshot-tests.md: - crates/vite_cli_snapshots: libtest-mimic runner that executes every step in a real PTY (vt100 grid capture), synchronizes interactive input on OSC 8 milestones, and compares Markdown snapshots with real pass/fail semantics (UPDATE_SNAPSHOTS=1 to accept) - one fixture tree with per-case vp flavor (local, global, or both for parity), per-case VP_HOME/HOME isolation, and managed-runtime seeding - vpt test multitool (vtt-aligned subcommands plus json-edit, chmod, probe) - tool migrate-snap-tests: one-click conversion of old steps.json cases, validated by migrating the check-pass cases end to end - packages/prompts: milestone emission (VP_EMIT_MILESTONES=1) wired into select, confirm, and text renders - just snapshot-test recipe, pnpm snapshot-test wrapper, and a CI step on the Rust test job (global flavor only until the JS build joins during migration) Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
Full both-flavor snapshot coverage now runs in cli-snap-test, which builds packages/cli/dist for the local flavor and reuses the installed release binary for the global flavor via the new VP_SNAP_GLOBAL_VP override (no second vite_global_cli compile). The Rust test job keeps its fast global-only leg for early signal, with a comment explaining the split. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
The archive's package loop is kept in sync with the justfile test recipe, which already excluded vite_cli_snapshots; the archive copy was missed, so the relocated cli_snapshots binary panicked at nextest list time on the Windows runner (its compile-time CARGO_MANIFEST_DIR is a Linux path). Also prefer the runtime CARGO_MANIFEST_DIR over the compile-time value in the harness, which is what relocated nextest archives rewrite; this is the groundwork for the planned Windows legs. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
The e2e job's repo-wide vp check flagged the markdown table alignment. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
The fixtures under crates/vite_cli_snapshots/tests are workspaces under test, not repo code, same as the existing snap-tests excludes. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
Adds the harness reference README (case/step/interaction schema, vpt helpers, milestone conventions, env overrides, migration workflow) and points AGENTS.md and CONTRIBUTING.md at it, marking the legacy snap trees as migration-only so new cases land in the new harness. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
build-windows-tests now cross-compiles a dedicated -p vite_cli_snapshots archive (test binary + vpt), and the new cli-snapshot-test-windows job runs it on windows-latest with no Rust toolchain: prebuilt vp via VP_SNAP_GLOBAL_VP, JS CLI built on the runner for the local flavor, managed runtime prewarmed for seed-runtime. vpt resolution now prefers the runtime CARGO_BIN_EXE_vpt that nextest rewrites under --workspace-remap, matching the CARGO_MANIFEST_DIR handling. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
Reuse: milestone hex encoding via Buffer.toString('hex'); the migrator now
imports the legacy Steps schema from snap-test.ts instead of redeclaring it.
Simplification: shared makeTodo/redirect handling and a verbatim-vpt set in
the migrator; dead NewStep fields dropped; shared find_beside_test_exe and
manifest_dir helpers in the harness; the always-true separator guard from
the upstream port removed.
Efficiency: redaction regexes compiled once per run (LazyLock), diagnostic
sort skipped when no blocks exist, fixture staging filters harness metadata
instead of copy-then-delete, per-step env only cloned when a step overrides
it, and the prompts milestone flag is cached at module load (per-keystroke
path).
Suite output is unchanged: all 8 trials pass against existing snapshots and
the migrator reproduces byte-identical fixtures.
Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
fengmk2
force-pushed
the
rfc/interactive-snapshot-tests
branch
from
fefbe9d to
e6ed290
Compare
std's canonicalize returns a \\?\ verbatim path on Windows; CMD.EXE,
which runs the local flavor's .cmd shims, rejects verbatim/UNC working
directories ('UNC paths are not supported'), so every local-flavor case
failed instantly on the Windows snapshot job. Also run the Windows suite
with --no-fail-fast: on a snapshot suite every diff is diagnostic signal.
Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
The full both-flavor suite already runs in cli-snap-test (linux/mac) and cli-snapshot-test-windows; the extra leg only re-ran the global cases and cost a vite_global_cli build inside the Rust test job. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
On Windows ~/.vite-plus/bin/vp.exe is the trampoline, which re-execs %VP_HOME%/current/bin/vp.exe; the harness gives each case an isolated VP_HOME with no install inside, so vp_help::help::global failed with 'failed to execute ...current/bin/vp.exe'. Use current/bin/vp.exe (the real CLI) as VP_SNAP_GLOBAL_VP, matching what bin/vp is on Unix. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
Converted with tool migrate-snap-tests (15 help/version steps, zero hand conversions). The new snapshot keeps every legacy assertion and adds the banner line the old pipe capture missed; vp -V now records the isolated workspace state (tools Not found) since the new harness does not symlink the checkout node_modules into fixtures. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
…llisions Successfully converted case directories are now removed from the legacy tree automatically (git history keeps the originals; --keep-old defers). A case whose target fixture already exists is skipped and reported instead of clobbering it: the same name in both legacy trees means a hand merge. Also migrates packages/cli/snap-tests/cli-helper-message as the first such merge: the fixture gains a cli_helper_message_local case (vp -h / -V) next to the 15-step global one, sharing the legacy package.json; the global snapshot is unchanged by the added file. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
Extracted from the cli-snap-test matrix per review: a dedicated Linux/macOS job with no runner.os/shard filter conditions, mirroring the Windows job's structure (build-upstream for dist + release vp, bootstrap-cli:ci, runtime prewarm for seed-runtime, then cargo test). Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
build-upstream and the snapshot suite never touch docs/ (the Windows snapshot job already runs green without it); the step exists in cli-e2e-test for pnpm tsgo, which this job does not run. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
Converted with tool migrate-snap-tests (env-prefixed commands became step envs, win32 skip became skip-platforms; zero hand conversions; the old case dir was removed by the migrator). The task-cache flow asserts identically: cold miss, cache hit with replay trailer, and env-changed miss. Build sizes and the asset hash are now recorded concretely instead of masked; they are deterministic per vite version. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
The win32 skip was inherited from the legacy case (added in #544 with no stated reason). Its plausible causes are gone in the new harness: env prefixes are structured step envs instead of shell syntax, the task engine supports Windows, .gitattributes forces LF so the asset content hash is checkout-stable, and vite prints forward-slash paths on every OS. The Windows snapshot job is the arbiter; if it disagrees, the skip returns with a documented reason. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
- Backslash-to-slash normalization now runs unconditionally on Windows instead of only when an absolute-path redaction matched the screen; tools print OS-native separators for relative paths too. Debug-escaped separators collapse BEFORE conversion so https:// URLs survive. - Every rendered row is trimmed of trailing whitespace on all platforms: ConPTY repaints rows padded to the grid width when a second console client attaches (global-flavor vp spawning node). - Byte sizes and content-hash asset suffixes are now redacted (<size>, <hash>): emitted bundle bytes differ across OSes, as #2031's 0.10 vs 0.11 kB pack output showed. Aligns the implementation with the RFC's normalization list; build_vite_env re-recorded accordingly. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
A redaction fixture drives the guarantees end to end through the PTY (a new vpt print-native-path payload prints OS-native separators, so one snapshot proves normalization on every platform; sizes/hashes masked; lowercase stems and URLs survive). A redact_unit test target covers the edges fixtures cannot exercise deterministically: ConPTY row padding, Debug-escaped separator collapse, and URL survival, with the Windows-gated assertions running in the nextest-archive job. Runner entry points now run the whole package (both test targets). Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
<size> kB instead of <size>, matching the legacy <variable> kB convention: the unit only changes when content crosses a magnitude boundary, which is real signal. Durations stay fully masked because their unit flips with timing (999ms vs 1.00s). Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
The crate list gains pty_terminal_test, pty_terminal_test_client, and snapshot_test (and corrects vite_glob/vite_powershell drift; enumeration now via grep instead of a hardcoded count). Step 6 covers the PTY suite first (local UPDATE_SNAPSHOTS workflow, harness-compilation breakage class, OS-shared snapshots) with the legacy trees marked as migration-only. The commented local-dev patch section in Cargo.toml now mirrors all nine crates. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
How contributors create the PR is personal workflow, not repo policy. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
|
@codex review |
The dependency set changes over time; the Cargo.toml grep is the authoritative enumeration, per review. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9b7db0e78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| localRegistry: false, | ||
| }; | ||
|
|
||
| const newName = caseName.replaceAll('-', '_'); |
There was a problem hiding this comment.
The new harness rejects fixture and case names unless they match [A-Za-z0-9_]+, but the migrator only replaces hyphens. Migrating existing legacy cases such as packages/cli/snap-tests-global/migration-not-supported-npm8.2 produces migration_not_supported_npm8.2, which still contains ., so just snapshot-test panics in assert_identifier_like before the migrated case can run. Replace every invalid character or emit a TODO instead of writing an unusable fixture.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d9bd36: fixture names now normalize every character outside [A-Za-z0-9_] (migration-not-supported-npm8.2 -> migration_not_supported_npm8_2), with a rename note in the report. Covered by the new migrate-snap-tests spec.
| if (PASSTHROUGH_PROGRAMS.has(program)) { | ||
| step = { argv: tokens }; | ||
| } else if (program in COREUTILS_MAP) { | ||
| step = { argv: ['vpt', COREUTILS_MAP[program], ...args.filter((a) => !a.startsWith('-'))] }; |
There was a problem hiding this comment.
Existing legacy fixtures include symbolic chmod commands, e.g. chmod +x fix-baseurl.mjs; this branch converts them to vpt chmod +x fix-baseurl.mjs. The new vpt chmod helper expects an octal mode and parses the first argument with from_str_radix, so the migrated step exits with an invalid-digit error before the real scenario runs. Translate common symbolic modes or mark them as TODOs for hand conversion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d9bd36, both sides: vpt chmod now accepts +x (adds execute bits to the current mode; still a Windows no-op), and the migrator passes through only octal or +x forms, turning other symbolic modes into TODOs. Spec-covered.
| if args.is_empty() { | ||
| return Err("Usage: vpt touch-file <filename>".into()); | ||
| } | ||
| let _file = std::fs::OpenOptions::new().read(true).write(true).open(&args[0])?; |
There was a problem hiding this comment.
vpt touch-file is documented as the cross-platform replacement for touch, but this opens the path without create(true). Any fixture setup or migrated legacy touch foo step for a missing file will fail with ENOENT and record an unexpected nonzero exit instead of creating the file, so the helper cannot be used for its advertised purpose.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d9bd36: touch-file now opens with create(true) (append mode, no truncation), deliberately diverging from vtt since our README documents it as the touch replacement. The vpt_selftest snapshot now creates a missing file and stat-files it.
| output = DURATION_RE.replace_all(&output, "<duration>").into_owned(); | ||
|
|
||
| // Redact semver-shaped versions (bundled tool versions, Node versions). | ||
| output = VERSION_RE.replace_all(&output, "<version>").into_owned(); |
There was a problem hiding this comment.
This blanket semver replacement also rewrites package manifests and config files printed by migrated package-manager or migration cases, e.g. "vitest": "4.0.13" becomes "vitest": "<version>", so snapshots can no longer catch wrong dependency pins. Scope version masking to known tool banners or make it opt-in for steps where versions are nondeterministic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping the blanket masking deliberately: it matches the legacy harness convention (old snap.txt shows "" in printed manifests too) and keeps snapshots stable across upstream dep bumps, which churn on nearly every renovate cycle. The cost is real but was already accepted in the old suite; when a migrated case genuinely needs pin assertions, the right tool is a per-step redaction opt-out, which we will add at that point rather than speculatively.
| const paths = args.filter((a) => !a.startsWith('-')); | ||
| if (paths.length > 0 && args.every((a) => /^-[fde]$/.test(a) || !a.startsWith('-'))) { | ||
| step = { argv: ['vpt', 'stat-file', ...paths] }; |
There was a problem hiding this comment.
For legacy assertions like test ! -f .nvmrc, this path extraction treats ! as another filename and emits vpt stat-file ! .nvmrc, so the migrated snapshot asserts a bogus ! path and no longer faithfully represents the negated check. Many deletion assertions in the legacy trees use this form; handle ! as absence semantics or leave these commands as TODOs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d9bd36: ! tokens are dropped from the path list (never treated as a filename). stat-file prints the actual exists/missing state, so the snapshot asserts the same fact for both polarities. Spec-covered with test ! -f .nvmrc.
| if !data.is_empty() { | ||
| stdin.write_all(data.as_bytes())?; | ||
| } | ||
| stdin.write_all(b"\n")?; |
There was a problem hiding this comment.
vpt pipe-stdin documents that empty data provides an empty stdin, but this unconditional write sends a newline even when <data> is empty. A non-TTY fixture that uses vpt pipe-stdin "" -- <cmd> to assert EOF/no input will instead exercise a one-line input case, producing misleading snapshots.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d9bd36: empty data now means genuinely empty stdin (immediate EOF); the terminating newline only accompanies actual input. Note vtt upstream has the same doc/behavior mismatch. The vpt_selftest snapshot now asserts both the empty and non-empty forms.
| ls: 'list-dir', | ||
| touch: 'touch-file', | ||
| chmod: 'chmod', | ||
| 'json-edit': 'json-edit', |
There was a problem hiding this comment.
Legacy cases call the old repo json-edit helper with assignment expressions such as json-edit package.json '_.dependencies = {}', but this mapping sends that expression unchanged to vpt json-edit, whose implementation requires <file> <dot-path> <value>. Those migrated steps exit with the usage error instead of editing the manifest; translate the old expression syntax or leave these commands as TODOs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d9bd36: json-edit only passes through the strict <file> <dot-path> <value> shape (three args, no = or spaces in the path); the legacy expression syntax becomes a TODO. Spec-covered with _.dependencies = {}.
| env.insert("VP_CLI_TEST".into(), "1".into()); | ||
| env.insert("VP_EMIT_MILESTONES".into(), "1".into()); | ||
| env.insert("NODE_NO_WARNINGS".into(), "1".into()); |
There was a problem hiding this comment.
The legacy snap harness set VP_SKIP_INSTALL=1 for every case, so vp migrate --no-interactive fixtures skipped real dependency installs; this new baseline omits that variable and the migrator does not add it back. Migrated migration snapshots will now perform slow, network-dependent installs and exercise different behavior unless each case is hand-edited, so preserve the legacy skip env here or inject it during migration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d9bd36: VP_SKIP_INSTALL=1 joined the harness baseline env for legacy parity, so migrated vp migrate cases skip real installs by default; cases wanting real installs opt out via unset-env. Existing snapshots verified unchanged.
| while (tokens.length > 0 && /^[A-Za-z_][A-Za-z0-9_]*=/.test(tokens[0])) { | ||
| const [key, ...rest] = tokens.shift()!.split('='); | ||
| envs.push([key, rest.join('=')]); |
There was a problem hiding this comment.
The $ check runs after leading VAR=value tokens are removed, so legacy commands like NPM_CONFIG_PREFIX=$(pwd)/custom-prefix npm install -g ... or PATH=$(pwd)/bin:$PATH ... are migrated with literal $(pwd)/$PATH values. Those cases install into the wrong prefix or lose the intended PATH setup; detect shell expansion inside env assignments and require hand conversion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d9bd36: env assignment values containing $ or backticks turn the whole command into a hand-conversion TODO before any translation. Spec-covered with NPM_CONFIG_PREFIX=$(pwd)/prefix.
| for (const part of parts) { | ||
| const step = translateSimple(part, ctx); | ||
| if (step) { | ||
| steps.push(step); |
There was a problem hiding this comment.
Splitting every && part independently means a legacy step like cd packages/web && vp run build becomes a TODO print for cd followed by a real vp run build step still executed from the fixture root. That records and asserts the wrong scenario for subdirectory cases; either translate leading cd <dir> && into the new step cwd field or make the whole chain a TODO.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d9bd36: a leading cd <dir> && chain now translates into the step's cwd field (relative dirs compose; absolute or expansion-bearing cd makes the whole chain a TODO), so subdirectory scenarios keep their meaning. Spec-covered.
vpt: touch-file creates missing files, empty pipe-stdin data means empty stdin (both diverge deliberately from vtt), chmod accepts +x. Harness: VP_SKIP_INSTALL=1 joins the baseline env for legacy parity. Migrator: fixture names normalize every invalid character, chmod/json-edit forms are validated (legacy expression syntax and symbolic modes become TODOs), test's ! token is dropped (stat-file records actual state), globs on shell-less vpt verbs become TODOs, echo redirects keep their trailing newline while printf escapes become TODOs, env values needing expansion become TODOs, and leading cd chains translate into the step cwd field. Covered by a new migrate-snap-tests vitest spec and extended vpt_selftest snapshot steps. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d9bd36a53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (command.length === 0) { | ||
| return []; |
There was a problem hiding this comment.
When a legacy steps.json entry is only a comment (for example the existing packages/cli/snap-tests/vite-config-task/steps.json and run-task-command-conflict/steps.json entries that start with #), extractComment() returns that whole line as command, so this empty-command guard does not fire and the migrator emits a TODO(migrate) step that prints into the new snapshot. That changes a no-op documentation line into an executed step for those cases instead of preserving the old runner's behavior of stripping comment-only commands.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b12942: comment-only entries (leading #) are documentation, not commands; they now drop with a report note instead of becoming an executed TODO step. Spec-covered.
| match arg.as_str() { | ||
| "-r" | "-rf" | "-f" => recursive = true, | ||
| _ => paths.push(arg.as_str()), |
There was a problem hiding this comment.
rm -f force semantics
For migrated cleanup/setup steps that use rm -f or rm -rf on optional paths, this treats -f only as recursive and never records that missing paths should be ignored, so a missing target still reaches remove_file() and fails the step. Several legacy fixtures use rm -f/rm -rf cleanup commands; the new vpt rm helper should either ignore NotFound when -f is present or those migrated tests will fail when the optional artifact was not created.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b12942: vpt rm now tracks -f separately from -r (previously -f only implied recursive, a bug inherited from the vtt port) and ignores NotFound when forced, so optional-artifact cleanup stays deterministic. Exercised by a new vpt_selftest step.
| if (!keepOld && !report.skipped) { | ||
| fs.rmSync(caseDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
When a case contains unsupported commands, migrateCase() records report.todos but leaves report.skipped false, so this branch still deletes the old snap-test directory unless --keep-old was passed. Since those TODOs become passing vpt print TODO(migrate) steps and the migration report is gitignored, it is easy to commit a migration that removes the real legacy coverage while the new harness only snapshots a placeholder.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b12942: auto-removal now requires a TODO-free conversion; cases with TODOs keep their legacy dir (with a report note) until the hand conversion lands, so placeholder steps can never silently replace real coverage. README updated to match.
|
|
||
| match &termination_state { | ||
| TerminationState::TimedOut => { | ||
| doc.push_str("**Exit code:** timeout\n\n"); |
There was a problem hiding this comment.
When a step times out, the harness only writes **Exit code:** timeout into the snapshot and then continues to check_snapshot, so UPDATE_SNAPSHOTS=1 can record the timeout as a passing baseline and any committed timeout baseline will keep passing. A hung CLI command should make the trial fail independently of snapshot contents; otherwise this suite can bless hangs instead of catching them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b12942: a step timeout now returns an error from the trial before any snapshot comparison, in both compare and UPDATE_SNAPSHOTS modes, with the redacted partial output in the failure message. Hangs can no longer be recorded or blessed.
| return todo('unsupported chmod invocation'); | ||
| } | ||
| } else if (program in COREUTILS_MAP) { | ||
| step = { argv: ['vpt', COREUTILS_MAP[program], ...args.filter((a) => !a.startsWith('-'))] }; |
There was a problem hiding this comment.
ls <file> checks during migration
This maps every legacy ls command to vpt list-dir, but existing snap tests use ls on files as an existence assertion (for example ls vite-plus-application/package.json in new-create-vite and several monorepo create cases). Those migrated steps will call read_dir on a file and fail even though the original ls <file> succeeded, so file-target ls needs to become a file assertion such as stat-file or be left as a TODO.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b12942 at the tool level rather than in the migrator: vpt list-dir on a file target prints the path, matching ls semantics, so migrated ls <file> existence assertions keep working unchanged. Exercised by a new vpt_selftest step.
| paths.length > 0 && | ||
| args.every((a) => a === '!' || /^-[fde]$/.test(a) || !a.startsWith('-')) | ||
| ) { | ||
| step = { argv: ['vpt', 'stat-file', ...paths] }; |
There was a problem hiding this comment.
test -f and test -d type checks
Migrating test -f/test -d to plain vpt stat-file drops the predicate type and only records that some filesystem entry exists. Legacy cases such as the hook/no-git checks rely on distinguishing directories from files, so a regression that creates a file where a directory was required (or vice versa) would still snapshot as exists after migration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b12942 at the tool level: stat-file now reports the entry type (file/dir/missing) instead of a typeless exists, so migrated test -f vs test -d assertions keep predicate fidelity; a file-where-dir-was-expected regression changes the snapshot. Existing snapshots re-recorded.
vpt: rm -f ignores missing targets, list-dir on a file prints the path (ls parity for existence assertions), stat-file reports file/dir/missing so migrated test -f/-d checks keep predicate fidelity. Harness: a step timeout now fails the trial in both modes; hangs can never be recorded or blessed as baselines. Migrator: comment-only legacy commands drop with a report note instead of becoming TODO steps, and cases with TODOs keep their legacy dir so placeholders never silently replace real coverage. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b1294289f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,8 @@ | |||
| [[case]] | |||
| name = "build_vite_env" | |||
| vp = "local" | |||
There was a problem hiding this comment.
This migrated case lost the legacy ignoredPlatforms: ["win32"] from packages/cli/snap-tests/build-vite-env/steps.json. The new cli-snapshot-test-windows workflow runs the PTY fixtures, so without skip-platforms = ["windows"] this build/cache scenario is exercised on Windows even though the old suite explicitly excluded it; if the existing Windows incompatibility still applies, the new job will fail. Please carry the skip over or include the Windows-specific fix in the migration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Stale finding: the skip was removed deliberately in an earlier commit at the maintainer's request (see test(cli): let build_vite_env run on Windows), after establishing the legacy reasons no longer apply in this harness (structured envs instead of shell prefixes, eol=lf-stable content hashes). The Windows snapshot job has since run this case green, so the removal is empirically validated.
| (repo_str.as_str(), "<repo>"), | ||
| ], | ||
| ); | ||
| return Err(format!( |
There was a problem hiding this comment.
When a case defines after cleanup and any step times out, this early return exits run_case before the for step in &case.after cleanup block below. This matters for timeout/hanging-process scenarios—the same cases likely to need cleanup for background services or temporary state—because the cleanup is skipped and can leave state behind for later trials or CI; run the cleanup path before returning the timeout error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7ce35c1: the timeout error is now deferred until after the after cleanup block runs, so hung-process cases still tear down before the trial fails.
| static VERSION_RE: LazyLock<regex::Regex> = LazyLock::new(|| { | ||
| regex::Regex::new(r"\bv?\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?\b").unwrap() | ||
| }); |
There was a problem hiding this comment.
This regex masks every semver-looking token in captured output, including user-controlled values in package.json, dependency ranges, and pack filenames such as app-1.0.0.tgz. Package-manager and migration snapshots often assert those exact versions changed or were preserved, so the new harness would hide regressions by turning them all into <version>; keep the redaction scoped to tool/runtime versions instead of all semver literals.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7ce35c1, and thanks for the second pass on this one, the sharper examples changed the verdict: version masking now requires the v prefix. Tool and runtime banners (vite v7.3.2, vp v0.2.2, Node.js v24.18.0) stay masked and dep-bump-stable, while user-controlled bare semver (app-1.0.0.tgz, dependency pins, doc examples like "0.2.0") stays assertable. Unit-covered; corpus re-verified.
| // part of the workspace under test, so they are never copied in. | ||
| CopyOptions::new() | ||
| .filter(|path, _| Ok(path != Path::new("snapshots") && path != Path::new("snapshots.toml"))) | ||
| .copy_tree(fixture_path, &stage) | ||
| .unwrap(); |
There was a problem hiding this comment.
The staged workspace only copies the fixture files and excludes harness metadata, but it never recreates the legacy runner's node_modules/vite-plus links. Any migrated or new fixture with a normal vite.config.ts like import { defineConfig } from 'vite-plus' will fail config loading in the isolated temp workspace unless every fixture vendors dependencies itself; add the same repo package/module setup here or document and enforce a different fixture contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7ce35c1: the run root now provides node_modules links for bare vite-plus and @voidzero-dev/vite-plus-core (Node's upward walk finds them from any staged workspace; the linked packages' own deps resolve at their real location), matching the legacy runner's module contract. Everything else must be vendored in the fixture, documented in the README. Proven by the new config_import fixture, whose computed task command forces the JS config loader so the import genuinely resolves.
Cleanup steps now run before a timeout error propagates, so hung-process cases still tear down. Version masking requires the v prefix: tool and runtime banners stay masked while user-controlled semver (pack filenames, dependency pins, doc examples) stays assertable. Fixture configs can import bare vite-plus / @voidzero-dev/vite-plus-core through run-root node_modules links, matching the legacy runner's module contract; proven by a new config_import fixture whose computed task command forces the JS config loader. cli_helper_message re-recorded: vp -V regains the full tool-version table the legacy snapshot asserted. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ce35c142f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn path_variants(path: &str, label: &'static str) -> Vec<(String, &'static str)> { | ||
| use cow_utils::CowUtils as _; | ||
| let stripped = path.strip_prefix(r"\\?\").unwrap_or(path); | ||
| let mut variants = vec![(path.to_owned(), label)]; |
There was a problem hiding this comment.
When a Windows child process prints an absolute path with forward slashes — for example Node/Vite file URLs like file:///C:/... or other JS stack/config paths — none of the variants generated here match because only raw backslash and debug-escaped backslash forms are redacted before the later separator-normalization pass. That later \→/ conversion cannot redact an already-forward-slashed absolute path, so Windows snapshot runs can leak machine-specific <workspace>/<repo> paths and diverge from Unix; please include a slash-normalized Windows path variant in the replacement list.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 23a5d0b: path_variants now also generates forward-slash forms of each redaction pair (raw and verbatim-stripped), so file:// URLs and stack-frame paths printed with forward slashes on Windows redact to their labels before the separator pass. Unit-covered cross-platform.
| cargo build -p vite_global_cli | ||
| cargo test -p vite_cli_snapshots -- {{args}} |
There was a problem hiding this comment.
When a developer changes packages/cli/src and then runs the documented just snapshot-test entry point, this recipe only rebuilds the Rust global binary; local-flavor cases use packages/cli/bin and local_cli_bin_dir only checks that dist/bin.js exists. That means a stale packages/cli/dist can be silently tested instead of the current source (or a clean checkout fails missing dist), so local CLI snapshot results can be false positives unless the user remembers a separate build step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 23a5d0b via detection rather than rebuilding: just snapshot-test deliberately does not run the heavy JS build chain, but the harness now fails fast when packages/cli/dist is older than packages/cli/src (mirroring the legacy harness's mtime check for the global binary), with the pnpm build / VP_SNAP_SKIP_FLAVORS=local remedies in the message. Skipped in CI and under the explicit checkout override. Stale dist can no longer produce false positives.
| #[cfg(unix)] | ||
| let _ = std::os::unix::fs::symlink(target, link); | ||
| #[cfg(windows)] | ||
| let _ = std::os::windows::fs::symlink_dir(target, link); |
There was a problem hiding this comment.
On Windows machines without symlink privileges, this ignored symlink_dir failure leaves the run-root node_modules without the checkout vite-plus/@voidzero-dev/vite-plus-core links. Local-flavor fixtures such as config_import rely on bare imports resolving through those links, so the harness fails with module-resolution errors instead of falling back as the comment promises; please create a junction/copy fallback or report the failure before running the cases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 23a5d0b: link_dir falls back to a junction (no privileges required) when symlink_dir fails; both the run-root node_modules links and the VP_HOME runtime seeding go through it. If both mechanisms fail the affected case fails visibly on module resolution, which is the honest remaining behavior.
| *s = replaced; | ||
| } | ||
| } | ||
| if let Cow::Owned(replaced) = s.as_str().cow_replace('\\', "/") { |
There was a problem hiding this comment.
When a step enables formatted-snapshot, render_formatted_screen turns SGR bytes into literal strings like \x1b[31m, but this Windows-only normalization rewrites every backslash in the captured output to /. Any color/style snapshot added later will therefore record /x1b[...] on Windows while Unix records \x1b[...], making the documented formatted snapshot mode unusable across the shared CI matrix; skip separator normalization for formatted escape renderings or protect \xNN sequences first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 23a5d0b: separator normalization is now gated per capture and skipped for formatted-snapshot steps, so literal \x1b renderings survive byte for byte on Windows. Windows-gated unit test added; this closes the tradeoff the code comment had documented.
Redaction gains forward-slash variants for Windows absolute paths (file:// URLs, stack frames) and skips separator normalization for formatted-snapshot captures so literal escape renderings survive. The local flavor fails fast when packages/cli/dist is older than src, so just snapshot-test can never silently test stale JS. Directory links fall back to junctions on Windows when symlinks need privileges (run-root node_modules and runtime seeding both benefit). Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23a5d0b7f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
| if (old.localVitePlusPackages || ctx.localRegistry) { | ||
| lines.push('local-registry = true'); |
There was a problem hiding this comment.
When a legacy case uses localVitePlusPackages or node $SNAP_LOCAL_REGISTRY, this emits local-registry = true into the new fixture even though the new harness immediately errors out for every such case in run_case. Since migrateCase still writes the fixture and only keeps the old directory because a TODO was reported, the next just snapshot-test over that migrated batch fails before baselines can be recorded; skip or mark these generated fixtures ignored until local-registry support exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f046797: local-registry migrations now generate ignore = true alongside local-registry = true, so the case is registered but skipped by default and a migrated batch stays green; the TODO notes the flip-back when harness support lands (the legacy dir is also kept, from the earlier keep-on-TODO fix).
| "node" => Ok(self.node_dir.join(format!("node{}", std::env::consts::EXE_SUFFIX))), | ||
| "git" | "npm" | "pnpm" | "yarn" | "bun" => which::which(program) | ||
| .map_err(|e| format!("`{program}` not found on the harness PATH: {e}")), |
There was a problem hiding this comment.
For env/runtime or global-install cases that first create shims in the isolated VP_HOME, follow-up commands such as node -v, npm --version, or an installed package binary need to be resolved through the case's PATH (including $VP_HOME/bin). This resolver instead turns node into the harness Node path and npm/pnpm/yarn/bun into whatever is on the parent process PATH before spawning, so migrated shim tests exercise host tools or fail to find newly installed shims rather than testing Vite+'s shims.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f046797: node/git/npm/pnpm/yarn/bun now resolve via which_in against the CASE's PATH, and the per-case PATH leads with $VP_HOME/bin, so shims created by vp env setup or global installs take precedence over harness/host tools. The harness node stays reachable only as the PATH fallback.
| timeout: entry.timeout, | ||
| snapshot: entry.ignoreOutput === true ? false : undefined, | ||
| }; |
There was a problem hiding this comment.
For legacy commands with ignoreOutput: true, this converts the step to snapshot = false unconditionally. The old runner only suppressed output after a successful command, so if one of these setup/build steps regressed the captured stdout/stderr still appeared for diagnosis; after migration the failing step records only the exit code and omits the screen, making these migrated fixture failures much harder to debug and losing the old failure behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f046797 at harness depth: snapshot = false now has the legacy ignoreOutput semantics natively; output is suppressed only while the step succeeds, and a failing step always keeps its screen for diagnosis. The migrator mapping is therefore faithful without special-casing.
Real tools (node, git, package managers) now resolve through the case's PATH, which leads with the per-case VP_HOME/bin, so shim and global-install fixtures exercise Vite+'s shims rather than host tools. snapshot = false adopts the legacy ignoreOutput semantics: output is suppressed only while the step succeeds, failures always keep their screen. The migrator generates local-registry cases with ignore = true so a migrated batch stays green until harness support lands. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f046797698
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| libtest_mimic::run(&args, tests).exit(); |
There was a problem hiding this comment.
When any snapshot run reaches this line, Conclusion::exit() terminates the process, so the TempDir created at the start of main is never dropped. Because all staged workspaces and the run-root node_modules links live under that tempdir, every successful or failed snapshot run leaves a full run tree behind in the OS temp directory; repeated local/CI runs can fill disks. Store the conclusion, drop the tempdir (or return an exit code) before exiting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bfad895: the Conclusion is captured, the run TempDir is dropped, and only then does exit() run, so staged trees no longer accumulate. Verified locally: the OS temp dir is clean after a full run. (The pattern was inherited from the upstream vite-task harness, which has the same leak; worth an upstream fix too.)
| } | ||
| copy_dir_recursive(src, dst)?; | ||
| } else { | ||
| std::fs::copy(src, dst)?; |
There was a problem hiding this comment.
When a fixture uses vpt cp file existing-dir (the common cp form for setup commands), this passes the directory path directly to std::fs::copy, which treats it as the destination file and fails with an “is a directory” error instead of creating existing-dir/file. Since vpt cp is documented as the cross-platform cp replacement, file copies into existing directories will break during fixture setup; append src.file_name() when dst.is_dir().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bfad895: vpt cp file existing-dir now copies INTO the directory (dst.join(file_name)), matching real cp. Exercised by a new vpt_selftest step that copies into a subdirectory and lists it.
| for step in &case.steps { | ||
| let argv = step.argv(); | ||
| assert!(!argv.is_empty(), "step argv must not be empty"); | ||
| let program = runtime.resolve_program(&argv[0], &case_path)?; |
There was a problem hiding this comment.
For detailed steps that override PATH in envs and run an allowed real tool such as node, npm, or pnpm, this resolves argv[0] against the case-level PATH before the per-step environment is applied. Those steps will therefore run the wrong host tool or fail to find a shim even though the child process later receives the intended PATH; compute the step environment first and resolve against its PATH.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bfad895: argv[0] resolution moved below the per-step env merge and resolves against the step's effective PATH (step override, else case PATH), so a step that overrides PATH runs exactly the tool its child would see.
| needs: | ||
| - download-previous-rolldown-binaries |
There was a problem hiding this comment.
The new PTY jobs are only reachable through download-previous-rolldown-binaries, which is gated by the existing detect-changes filter that excludes **/*.md. Because the new harness stores its assertions as .md files under crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/**/snapshots/, a PR that only records or updates those baselines is treated as docs-only and skips these snapshot jobs entirely; add an exception for these snapshot paths so baseline-only changes are still checked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bfad895: detect-changes gained a snapshots filter (crates/vite_cli_snapshots/**) OR-ed into code-changed, so baseline-only .md updates still run the snapshot jobs. A plain positive pattern could not be appended because the code filter uses predicate-quantifier: every over negations.
| let _ = std::process::Command::new(program) | ||
| .args(&argv[1..]) | ||
| .env_clear() | ||
| .envs(&case_env) |
There was a problem hiding this comment.
If an after cleanup step is written as a detailed step with envs (which the shared Step schema allows, and the migrator can emit from leading VAR=value assignments), those variables are ignored here and the command runs with only the case environment. Cleanup that depends on a custom PATH, prefix, or target variable will silently fail because cleanup output/status is discarded, leaving files, shims, or background processes behind for later trials.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bfad895: after-cleanup steps now apply their per-step envs over the case env, and resolve argv[0] against that effective PATH, mirroring regular steps.
The run tempdir is dropped before Conclusion::exit so runs no longer leak staged trees. vpt cp copies into existing directories like real cp. Real-tool resolution happens after per-step envs apply, so a step PATH override selects the tool the child sees. after-cleanup steps get their per-step envs (and PATH-aware resolution) too. detect-changes ORs in a crates/vite_cli_snapshots filter so baseline-only .md changes still run the snapshot jobs. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfad895b80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # snapshots is OR-ed in because the PTY harness stores its assertions | ||
| # as .md files (fixtures/**/snapshots/*.md), which the code filter's | ||
| # markdown exclusion would otherwise classify as docs-only. | ||
| code-changed: ${{ steps.filter.outputs.code == 'true' || steps.filter.outputs.snapshots == 'true' }} |
There was a problem hiding this comment.
This only fixes the in-workflow docs/code filter after the workflow has started; on push to main, the top-level paths-ignore: '**/*.md' still prevents the workflow from running at all for changes that only update crates/vite_cli_snapshots/**/snapshots/*.md. That means a snapshot-baseline-only merge can bypass the new PTY snapshot CI entirely, even though this output now treats those Markdown files as code changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7660d05: the push trigger's paths-ignore is now a paths list with negations ('', '!/*.md', then re-include crates/vite_cli_snapshots/**), so a baseline-only merge to main still runs the workflow; the in-workflow filter fix from the previous round then routes it to the snapshot jobs.
| let name = entry.file_name().to_string_lossy().into_owned(); | ||
| if let Some(suffix) = ext | ||
| && !name.ends_with(suffix) | ||
| { | ||
| continue; | ||
| } | ||
| names.push(name); |
There was a problem hiding this comment.
Because std::fs::read_dir returns names starting with ., this helper includes hidden files for every migrated plain ls command; checked ls --help, where -a/--all is the option that stops ignoring dot entries. Existing legacy cases such as ls node_modules intentionally hide package-manager internals like .pnpm, so mapping bare ls to this output changes the assertion and can make migrated snapshots noisy or platform/package-manager dependent unless dot entries are skipped unless an explicit all flag is represented.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7660d05: list-dir hides dot entries by default like plain ls (keeping .pnpm and friends out of snapshots), with --all as the opt-in; the migrator additionally TODOs any ls invocation with flags, since -l and friends change semantics list-dir does not replicate. Spec-covered.
| case_path: &std::ffi::OsStr, | ||
| ) -> Result<PathBuf, String> { | ||
| match program { | ||
| "vp" | "vpr" | "vpx" | "vpt" | "oxfmt" | "oxlint" => { |
There was a problem hiding this comment.
When a case creates $VP_HOME/bin shims, baseline_env deliberately puts that directory before the harness bin, but this branch ignores case_path for vp, vpr, and vpx and always runs the harness-installed alias. A snapshot meant to validate vp env setup or a replaced vpr/vpx shim will therefore pass against the harness alias even if the generated shim is missing or broken; these commands need to resolve through the case PATH with the harness bin as the fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7660d05: vp/vpr/vpx (and oxfmt/oxlint) resolve through the case PATH first, so $VP_HOME/bin shims shadow the harness aliases; the direct bin-dir lookup remains only as a fallback for cases that override PATH entirely. vpt stays harness-resolved on purpose (it is the harness's own tool, never shimmed).
| // `cd <dir> && ...` scopes the rest of the chain to that directory (each | ||
| // legacy command line started fresh at the fixture root, so the cwd never | ||
| // leaks across lines). | ||
| let cwd: string | undefined; | ||
| for (const part of parts) { |
There was a problem hiding this comment.
For legacy commands like vp remove testnpm2 -D && cat package.json, this loop emits independent steps and the new harness continues after nonzero exits, so the cat step still runs even though a shell && chain would stop. That can bless follow-up output from a failed setup/action and changes the meaning of existing snap assertions; either preserve the chain semantics or leave nontrivial && chains as TODOs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7660d05 at harness depth: a failing step now stops the case by default (shell-like && semantics; the snapshot notes the skipped remainder), with continue-on-failure = true as the deliberate opt-out. The migrator restores the legacy model exactly: chain members keep the stopping default, and each legacy command line's FINAL step gets the opt-out because old lines were independent. Spec-covered both ways.
| static VERSION_RE: LazyLock<regex::Regex> = LazyLock::new(|| { | ||
| regex::Regex::new(r"\bv\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?\b").unwrap() |
There was a problem hiding this comment.
This only redacts v-prefixed semver, but vp create prints tool versions as bare values (Node ${process.versions.node} ${packageManager} ${packageManagerVersion} in packages/cli/src/create/bin.ts). Any new create-flow snapshot will therefore lock to the runner's Node/package-manager versions and churn across CI images or local runtime updates; those tool-version contexts need redaction even though other bare semver literals should remain assertable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7660d05: bare runtime-tool versions redact by tool-name context (Node/Node.js/npm/pnpm/yarn/bun/deno followed by bare semver, space or slash separated), covering vp create's banner and user-agent strings, while unrelated bare semver (pins, tarball names) stays assertable. Unit-covered.
The push trigger's paths-ignore becomes a paths list with negations so baseline-only .md pushes still run CI. list-dir hides dot entries like plain ls (--all opts in) and the migrator TODOs ls flags. vp/vpr/vpx resolve through the case PATH first so VP_HOME/bin shims shadow the harness aliases. A failing step now stops the case by default (shell-like && semantics) with continue-on-failure as the opt-out; the migrator restores legacy semantics exactly by marking each command line's final step. Bare runtime-tool versions (Node 24.x, pnpm 10.x from vp create) redact by tool-name context while user semver stays assertable. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7660d0533f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cargo build -p vite_global_cli | ||
| cargo test -p vite_cli_snapshots -- {{args}} |
There was a problem hiding this comment.
When a clean checkout runs the documented just snapshot-test, this recipe only builds the Rust global vp before invoking the harness. The new suite includes local-flavor cases, and local_cli_bin_dir() requires packages/cli/dist/bin.js; since dist is not committed, those cases fail before running unless the developer happened to run a separate JS build first. Please build packages/cli here or explicitly skip/document the local flavor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in df5abf7: absent dist stays a deliberate fail-fast (each local trial errors with the exact remedies: pnpm build or VP_SNAP_SKIP_FLAVORS=local), because auto-building would pull the heavy rolldown build chain into the recipe and auto-skipping would silently drop coverage. What was missing was a first-class entry point for that workflow: just snapshot-test-global (unix + windows recipes) runs the global flavor and vpt cases with no JS build, documented in the README's run commands.
…build Round-8 review follow-up: absent dist stays a deliberate fail-fast with remedies in the message (auto-building would drag the rolldown chain into the recipe; auto-skipping would hide coverage), and toolchain-only contributors get a first-class entry point instead. Claude-Session: https://claude.ai/code/session_01NRgjMi2Vus3iJctudGEWPT
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df5abf79ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,8 @@ | |||
| [[case]] | |||
| name = "build_vite_env" | |||
| vp = "local" | |||
There was a problem hiding this comment.
This migrated case will now run in the new cli-snapshot-test-windows job because there is no skip-platforms entry, but the legacy source case (packages/cli/snap-tests/build-vite-env/steps.json) had ignoredPlatforms: ["win32"], and the RFC mapping in this commit says those should become skip-platforms = ["windows"]. As written, Windows-only failures that were previously excluded from the legacy snap suite will now fail the PTY snapshot job; add the Windows skip here unless the fixture has been explicitly made Windows-safe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Repeat of an earlier-round finding, already resolved empirically: the skip was removed on purpose in test(cli): let build_vite_env run on Windows (the legacy win32 reasons do not apply in this harness: structured step envs instead of shell prefixes, eol=lf-stable content hashes, forward-slash vite output), and the CLI snapshot test (Windows) job has run this case green repeatedly since. That satisfies the condition stated here: the fixture has been explicitly made Windows-safe, with the rationale in the commit message. The RFC mapping applies to the automated migrator default; deliberately challenging an inherited skip and letting the Windows job arbitrate is the documented follow-up practice.
1 participant
Implements the RFC included in this PR as
rfcs/interactive-snapshot-tests.md.crates/vite_cli_snapshots: PTY snapshot harness (libtest-mimic). Every step runs in a real pseudo-terminal with vt100 grid capture; interactive steps script keystrokes synchronized on OSC 8 milestones; snapshots are Markdown compared with real pass/fail semantics (UPDATE_SNAPSHOTS=1to accept,.md.newplus unified diff on mismatch). Built on the pty_terminal/snapshot_test crates from vite-task at the already-pinned rev.vp = "local" | "global" | ["local", "global"]. The parity matrix already caught real drift between the two help outputs. Per-caseVP_HOME/HOME/npm-prefix isolation removesserialand the bootstrap byte-match requirement;seed-runtimesymlinks a provisioned managed runtime so cases do not download Node per case.vpttest multitool: vtt-aligned subcommands plusjson-edit,chmod, andprobe(interactive payload proving milestone round-trips before product commands are instrumented).tool migrate-snap-tests <dir> --vp <flavor> [filter]: one-click migration of oldsteps.jsoncases with a report; validated by migrating the fourcheck-pass*cases, which pass under the new harness with equivalent assertions.packages/prompts: milestone emission (select/confirm/text, gated onVP_EMIT_MILESTONES=1, byte-identical to the vite-task protocol, unit-tested; render output unchanged when disabled).just snapshot-test [filter],pnpm snapshot-test. CI runs the suite on the Rust test job withVP_SNAP_SKIP_FLAVORS=localuntil the JS build joins during migration.Validation: 8 snapshot trials pass in compare mode (including the interactive milestone case), prompts suite 10/10, clippy/rustfmt/
vp checkclean,just testexcludes the harness crate.Follow-ups per the RFC phasing:
local-registrycase support, remaining prompt components (multiselect, password, spinner), migration batches and old-harness removal.