Skip to content

Commit bfad895

Browse files
committed
fix(test): address Codex round-6 findings
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
1 parent f046797 commit bfad895

5 files changed

Lines changed: 55 additions & 7 deletions

File tree

.github/workflows/ci.yml

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,10 @@ jobs:
4444
contents: read
4545
pull-requests: read
4646
outputs:
47-
code-changed: ${{ steps.filter.outputs.code }}
47+
# snapshots is OR-ed in because the PTY harness stores its assertions
48+
# as .md files (fixtures/**/snapshots/*.md), which the code filter's
49+
# markdown exclusion would otherwise classify as docs-only.
50+
code-changed: ${{ steps.filter.outputs.code == 'true' || steps.filter.outputs.snapshots == 'true' }}
4851
steps:
4952
- uses: taiki-e/checkout-action@7d1e50e93dc4fb3bba58f85018fadf77898aee8b # v1.4.2
5053
- uses: dorny/paths-filter@fbd0ab8f3e69293af611ebaee6363fc25e6d187d # v4.0.1
@@ -58,6 +61,8 @@ jobs:
5861
- '!.github/workflows/deploy-docs-preview.yml'
5962
- '!.github/workflows/deploy-docs.yml'
6063
- '!netlify.toml'
64+
snapshots:
65+
- 'crates/vite_cli_snapshots/**'
6166
6267
docs-fmt:
6368
name: Docs format check

crates/vite_cli_snapshots/src/bin/vpt/cp.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,13 @@ pub fn run(args: &[String]) -> Result<(), Box<dyn std::error::Error>> {
1313
}
1414
copy_dir_recursive(src, dst)?;
1515
} else {
16-
std::fs::copy(src, dst)?;
16+
// `cp file existing-dir` copies INTO the directory, like real cp.
17+
let target = if dst.is_dir() {
18+
dst.join(src.file_name().ok_or("source has no file name")?)
19+
} else {
20+
dst.to_path_buf()
21+
};
22+
std::fs::copy(src, target)?;
1723
}
1824
Ok(())
1925
}

crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/vpt_selftest/snapshots.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ steps = [
1212
{ argv = ["vpt", "touch-file", "created-by-touch.txt"], comment = "touch-file creates missing files" },
1313
{ argv = ["vpt", "stat-file", "created-by-touch.txt", "notes"], comment = "stat-file reports the entry type: file, dir, or missing" },
1414
{ argv = ["vpt", "rm", "-f", "never-existed.txt"], comment = "rm -f ignores missing targets" },
15+
{ argv = ["vpt", "cp", "created-by-touch.txt", "notes"], comment = "cp into an existing directory, like real cp" },
16+
["vpt", "list-dir", "notes"],
1517
{ argv = ["vpt", "list-dir", "notes/hello.txt"], comment = "list-dir on a file prints the path, like ls" },
1618
{ argv = ["vpt", "chmod", "+x", "created-by-touch.txt"], comment = "symbolic +x is accepted (no-op on Windows)" },
1719
{ argv = ["vpt", "pipe-stdin", "--", "vpt", "read-stdin"], comment = "empty pipe-stdin data means empty stdin, not a bare newline" },

crates/vite_cli_snapshots/tests/cli_snapshots/fixtures/vpt_selftest/snapshots/file_roundtrip.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,20 @@ rm -f ignores missing targets
6666
```
6767
```
6868

69+
## `vpt cp created-by-touch.txt notes`
70+
71+
cp into an existing directory, like real cp
72+
73+
```
74+
```
75+
76+
## `vpt list-dir notes`
77+
78+
```
79+
created-by-touch.txt
80+
hello.txt
81+
```
82+
6983
## `vpt list-dir notes/hello.txt`
7084

7185
list-dir on a file prints the path, like ls

crates/vite_cli_snapshots/tests/cli_snapshots/main.rs

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -653,7 +653,6 @@ fn run_case(
653653
for step in &case.steps {
654654
let argv = step.argv();
655655
assert!(!argv.is_empty(), "step argv must not be empty");
656-
let program = runtime.resolve_program(&argv[0], &case_path)?;
657656

658657
// Most steps add no env of their own; borrow the case env then.
659658
let step_env_override;
@@ -667,6 +666,10 @@ fn run_case(
667666
step_env_override = env;
668667
&step_env_override
669668
};
669+
// Resolution honors a per-step PATH override, so steps testing shims
670+
// or custom prefixes run exactly the tool the child would see.
671+
let step_path = step_env.get("PATH").cloned().unwrap_or_else(|| case_path.clone());
672+
let program = runtime.resolve_program(&argv[0], &step_path)?;
670673
let step_cwd = stage.join(step.cwd().unwrap_or(case.cwd.as_str()));
671674
let timeout = step.timeout();
672675

@@ -827,15 +830,29 @@ fn run_case(
827830
}
828831
}
829832

830-
// Cleanup steps: best-effort, never snapshotted.
833+
// Cleanup steps: best-effort, never snapshotted. Per-step envs apply
834+
// here too: cleanup often depends on the same PATH/prefix overrides as
835+
// the step it tears down.
831836
for step in &case.after {
832837
let argv = step.argv();
833838
assert!(!argv.is_empty(), "after-step argv must not be empty");
834-
if let Ok(program) = runtime.resolve_program(&argv[0], &case_path) {
839+
let after_env_override;
840+
let after_env: &BTreeMap<String, OsString> = if step.envs().is_empty() {
841+
&case_env
842+
} else {
843+
let mut env = case_env.clone();
844+
for (k, v) in step.envs() {
845+
env.insert(k.clone(), v.into());
846+
}
847+
after_env_override = env;
848+
&after_env_override
849+
};
850+
let after_path = after_env.get("PATH").cloned().unwrap_or_else(|| case_path.clone());
851+
if let Ok(program) = runtime.resolve_program(&argv[0], &after_path) {
835852
let _ = std::process::Command::new(program)
836853
.args(&argv[1..])
837854
.env_clear()
838-
.envs(&case_env)
855+
.envs(after_env)
839856
.current_dir(stage.join(step.cwd().unwrap_or(case.cwd.as_str())))
840857
.stdin(std::process::Stdio::null())
841858
.output();
@@ -966,5 +983,9 @@ fn main() {
966983
}
967984
}
968985

969-
libtest_mimic::run(&args, tests).exit();
986+
let conclusion = libtest_mimic::run(&args, tests);
987+
// exit() never returns, so the staged run tree must be dropped first or
988+
// every run would leave its full tempdir behind.
989+
drop(tmp_dir);
990+
conclusion.exit();
970991
}

0 commit comments

Comments
 (0)