diff --git a/crates/av1/tests/it/common/mod.rs b/crates/av1/tests/it/common/mod.rs index 13b4567d6..c500221c0 100644 --- a/crates/av1/tests/it/common/mod.rs +++ b/crates/av1/tests/it/common/mod.rs @@ -76,7 +76,7 @@ pub fn make(ff: &Path, spec: &Spec) -> PathBuf { args.extend(spec.enc.iter().map(String::as_str)); args.extend(["-f", "ivf", t.as_str()]); run(ff, &args); - std::fs::rename(&tmp, &path).unwrap(); + filmcraft_testkit::fixtures::publish(&tmp, &path).unwrap(); } path } diff --git a/crates/dnx/tests/it/common/mod.rs b/crates/dnx/tests/it/common/mod.rs index 67858f079..996c1ba91 100644 --- a/crates/dnx/tests/it/common/mod.rs +++ b/crates/dnx/tests/it/common/mod.rs @@ -49,7 +49,7 @@ pub fn make(ff: &Path, spec: &Spec) -> PathBuf { let t = tmp.to_str().unwrap().to_string(); args.push(&t); run(ff, &args); - std::fs::rename(&tmp, &path).unwrap(); + filmcraft_testkit::fixtures::publish(&tmp, &path).unwrap(); } path } @@ -278,7 +278,7 @@ pub fn source_frames(ff: &Path, spec: SourceFrameSpec<'_>) -> Vec { let tmp = filmcraft_testkit::temp_path(&path); let f = frames.to_string(); run(ff, &["-f", "lavfi", "-i", lavfi, "-frames:v", &f, "-f", "rawvideo", "-pix_fmt", pix, tmp.to_str().unwrap()]); - std::fs::rename(&tmp, &path).unwrap(); + filmcraft_testkit::fixtures::publish(&tmp, &path).unwrap(); } let bytes = std::fs::read(&path).unwrap(); let s: Vec = if depth == 8 { diff --git a/crates/h264/tests/it/common/mod.rs b/crates/h264/tests/it/common/mod.rs index 2de4d62d2..869613dc5 100644 --- a/crates/h264/tests/it/common/mod.rs +++ b/crates/h264/tests/it/common/mod.rs @@ -364,7 +364,7 @@ pub fn ensure(f: &Fixture) -> Option<(PathBuf, PathBuf)> { eprintln!("SKIPPED ({}): encoder unavailable", f.name); return None; } - std::fs::rename(&tmp, &h264).unwrap(); + filmcraft_testkit::fixtures::publish(&tmp, &h264).unwrap(); } if !yuv.exists() { let tmp = filmcraft_testkit::temp_path(&yuv); @@ -372,7 +372,7 @@ pub fn ensure(f: &Fixture) -> Option<(PathBuf, PathBuf)> { c.args(["-hide_banner", "-loglevel", "error", "-y", "-i"]).arg(&h264); c.args(["-fps_mode", "passthrough", "-f", "rawvideo", "-pix_fmt", "yuv420p"]).arg(&tmp); assert!(run(&mut c), "reference decode failed for {}", f.name); - std::fs::rename(&tmp, &yuv).unwrap(); + filmcraft_testkit::fixtures::publish(&tmp, &yuv).unwrap(); } Some((h264, yuv)) } diff --git a/crates/hevc/tests/it/common/mod.rs b/crates/hevc/tests/it/common/mod.rs index 61c1033f9..cebd9019f 100644 --- a/crates/hevc/tests/it/common/mod.rs +++ b/crates/hevc/tests/it/common/mod.rs @@ -142,7 +142,7 @@ pub fn ensure(f: &Fixture) -> Option<(PathBuf, PathBuf)> { eprintln!("SKIPPED ({}): encoder unavailable", f.name); return None; } - std::fs::rename(&tmp, &hevc).unwrap(); + filmcraft_testkit::fixtures::publish(&tmp, &hevc).unwrap(); } if !yuv.exists() { let tmp = filmcraft_testkit::temp_path(&yuv); @@ -150,7 +150,7 @@ pub fn ensure(f: &Fixture) -> Option<(PathBuf, PathBuf)> { c.args(["-hide_banner", "-loglevel", "error", "-y", "-i"]).arg(&hevc); c.args(["-fps_mode", "passthrough", "-f", "rawvideo", "-pix_fmt", pix_fmt(f.bit_depth)]).arg(&tmp); assert!(run(&mut c), "reference decode failed for {}", f.name); - std::fs::rename(&tmp, &yuv).unwrap(); + filmcraft_testkit::fixtures::publish(&tmp, &yuv).unwrap(); } Some((hevc, yuv)) } diff --git a/crates/isobmff/tests/it/common/mod.rs b/crates/isobmff/tests/it/common/mod.rs index 895c217d4..0b0bd8615 100644 --- a/crates/isobmff/tests/it/common/mod.rs +++ b/crates/isobmff/tests/it/common/mod.rs @@ -113,7 +113,7 @@ fn run_ffmpeg(ff: &Path, args: &[&str], out: &Path) -> bool { let tmp = filmcraft_testkit::temp_path(out); let st = Command::new(ff).args(["-v", "error", "-y", "-nostdin"]).args(args).arg(&tmp).output(); match st { - Ok(o) if o.status.success() => std::fs::rename(&tmp, out).is_ok(), + Ok(o) if o.status.success() => filmcraft_testkit::fixtures::publish(&tmp, out).is_ok(), Ok(o) => { eprintln!("ffmpeg failed for {}: {}", out.display(), String::from_utf8_lossy(&o.stderr)); let _ = std::fs::remove_file(&tmp); diff --git a/crates/matroska/tests/common/mod.rs b/crates/matroska/tests/common/mod.rs index ad82697dd..54af75230 100644 --- a/crates/matroska/tests/common/mod.rs +++ b/crates/matroska/tests/common/mod.rs @@ -114,7 +114,7 @@ pub fn fixture(ffmpeg: &Path, name: &str) -> Option { eprintln!("could not generate {name} (encoder unavailable?); skipping"); return None; } - std::fs::rename(&tmp, &out).unwrap(); // atomic: concurrent generators race harmlessly + filmcraft_testkit::fixtures::publish(&tmp, &out).unwrap(); // atomic: concurrent generators race harmlessly Some(out) } diff --git a/crates/prores/tests/it/common/mod.rs b/crates/prores/tests/it/common/mod.rs index c31245537..f2b37c53d 100644 --- a/crates/prores/tests/it/common/mod.rs +++ b/crates/prores/tests/it/common/mod.rs @@ -47,7 +47,7 @@ pub fn make(ff: &Path, spec: &Spec) -> PathBuf { let t = tmp.to_str().unwrap().to_string(); args.push(&t); run(ff, &args); - std::fs::rename(&tmp, &path).unwrap(); + filmcraft_testkit::fixtures::publish(&tmp, &path).unwrap(); } path } @@ -265,10 +265,10 @@ pub fn source_frames(ff: &Path, spec: SourceFrameSpec<'_>) -> Vec { (ChromaFormat::Yuv444, false) => "yuv444p10le", (ChromaFormat::Yuv444, true) => "yuva444p10le", }; - let tmp = fixture_dir().join(format!("{name}.tmp.raw")); + let tmp = filmcraft_testkit::temp_path(&path); let f = frames.to_string(); run(ff, &["-f", "lavfi", "-i", lavfi, "-frames:v", &f, "-f", "rawvideo", "-pix_fmt", pix, tmp.to_str().unwrap()]); - std::fs::rename(&tmp, &path).unwrap(); + filmcraft_testkit::fixtures::publish(&tmp, &path).unwrap(); } let bytes = std::fs::read(&path).unwrap(); let s: Vec = bytes.as_chunks::<2>().0.iter().map(|c| u16::from_le_bytes([c[0], c[1]])).collect(); diff --git a/crates/testkit/src/fixtures.rs b/crates/testkit/src/fixtures.rs index 22e3c7a88..059cc22d8 100644 --- a/crates/testkit/src/fixtures.rs +++ b/crates/testkit/src/fixtures.rs @@ -46,11 +46,34 @@ pub fn temp_path(out: &Path) -> PathBuf { out.with_file_name(name) } -/// Return `out` if it already exists (non-empty); otherwise run `make(tmp)` on a [`temp_path`] +fn complete_file(path: &Path) -> bool { + std::fs::metadata(path).is_ok_and(|m| m.is_file() && m.len() > 0) +} + +/// Publish a complete temporary file, or reuse a complete concurrently published +/// winner. On failure, attempt to remove only the caller's temporary file. +/// This is for files, not directories; callers must use a unique [`temp_path`]. +pub fn publish(tmp: &Path, out: &Path) -> std::io::Result<()> { + if !complete_file(tmp) { + let winner = complete_file(out); + let _ = std::fs::remove_file(tmp); + return if winner { Ok(()) } else { Err(std::io::Error::new(std::io::ErrorKind::InvalidData, "temporary fixture is not a non-empty file")) }; + } + match std::fs::rename(tmp, out) { + Ok(()) => Ok(()), + Err(error) => { + let winner = complete_file(out); + let _ = std::fs::remove_file(tmp); + if winner { Ok(()) } else { Err(error) } + } + } +} + +/// Return `out` if it is a non-empty file; otherwise run `make(tmp)` on a [`temp_path`] /// and atomically rename the result into place. Concurrent generators race harmlessly. /// `None` when `make` reports failure (the temporary file is removed). pub fn generate(out: &Path, make: impl FnOnce(&Path) -> bool) -> Option { - if std::fs::metadata(out).is_ok_and(|m| m.len() > 0) { + if complete_file(out) { return Some(out.to_path_buf()); } if let Some(dir) = out.parent() { @@ -61,7 +84,7 @@ pub fn generate(out: &Path, make: impl FnOnce(&Path) -> bool) -> Option let _ = std::fs::remove_file(&tmp); return None; } - std::fs::rename(&tmp, out).ok()?; + publish(&tmp, out).ok()?; Some(out.to_path_buf()) } @@ -103,6 +126,154 @@ pub fn generate_and_report(name: &str, outputs: &[PathBuf], generate: impl Fn mod tests { use super::*; + #[cfg(windows)] + #[test] + fn direct_publication_reuses_a_locked_complete_winner() { + use std::os::windows::fs::OpenOptionsExt; + let dir = temp_path(&std::env::temp_dir().join("filmcraft-testkit-direct-publish")); + std::fs::create_dir_all(&dir).unwrap(); + let out = dir.join("fixture.bin"); + let tmp = temp_path(&out); + std::fs::write(&out, b"winner").unwrap(); + std::fs::write(&tmp, b"candidate").unwrap(); + let reader = std::fs::OpenOptions::new().read(true).share_mode(1).open(&out).unwrap(); + assert!(publish(&tmp, &out).is_ok()); + assert_eq!(std::fs::read(&out).unwrap(), b"winner"); + assert!(!tmp.exists()); + drop(reader); + std::fs::remove_dir_all(dir).unwrap(); + } + + #[test] + fn direct_publication_rejects_invalid_temporary_without_output() { + for kind in ["empty", "missing", "directory"] { + let dir = temp_path(&std::env::temp_dir().join("filmcraft-testkit-invalid-publish")); + std::fs::create_dir_all(&dir).unwrap(); + let out = dir.join("fixture.bin"); + let tmp = temp_path(&out); + match kind { + "empty" => std::fs::write(&tmp, b"").unwrap(), + "directory" => { + std::fs::create_dir(&tmp).unwrap(); + std::fs::write(tmp.join("child"), b"caller owned").unwrap(); + } + _ => {} + } + assert!(publish(&tmp, &out).is_err(), "accepted {kind} temporary"); + assert!(!out.exists(), "published {kind} temporary"); + if kind == "directory" { + assert_eq!(std::fs::read(tmp.join("child")).unwrap(), b"caller owned"); + std::fs::remove_file(tmp.join("child")).unwrap(); + std::fs::remove_dir(&tmp).unwrap(); + } else { + assert!(!tmp.exists(), "left {kind} temporary file"); + } + std::fs::remove_dir(&dir).unwrap(); + } + } + + #[test] + fn direct_publication_rejects_invalid_temporary_with_empty_output() { + for kind in ["empty", "missing", "directory"] { + let dir = temp_path(&std::env::temp_dir().join("filmcraft-testkit-invalid-empty-out")); + std::fs::create_dir_all(&dir).unwrap(); + let out = dir.join("fixture.bin"); + let tmp = temp_path(&out); + std::fs::write(&out, b"").unwrap(); + match kind { + "empty" => std::fs::write(&tmp, b"").unwrap(), + "directory" => { + std::fs::create_dir(&tmp).unwrap(); + std::fs::write(tmp.join("child"), b"caller owned").unwrap(); + } + _ => {} + } + assert!(publish(&tmp, &out).is_err(), "accepted {kind} temporary"); + assert!(out.is_file()); + assert_eq!(std::fs::read(&out).unwrap(), b""); + if kind == "directory" { + assert_eq!(std::fs::read(tmp.join("child")).unwrap(), b"caller owned"); + std::fs::remove_file(tmp.join("child")).unwrap(); + std::fs::remove_dir(&tmp).unwrap(); + } else { + assert!(!tmp.exists(), "left {kind} temporary file"); + } + std::fs::remove_file(&out).unwrap(); + std::fs::remove_dir(&dir).unwrap(); + } + } + + #[test] + fn direct_publication_preserves_complete_winner_with_invalid_temporary() { + for kind in ["empty", "missing", "directory"] { + let dir = temp_path(&std::env::temp_dir().join("filmcraft-testkit-invalid-winner")); + std::fs::create_dir_all(&dir).unwrap(); + let out = dir.join("fixture.bin"); + let tmp = temp_path(&out); + std::fs::write(&out, b"complete winner").unwrap(); + match kind { + "empty" => std::fs::write(&tmp, b"").unwrap(), + "directory" => { + std::fs::create_dir(&tmp).unwrap(); + std::fs::write(tmp.join("child"), b"caller owned").unwrap(); + } + _ => {} + } + assert!(publish(&tmp, &out).is_ok(), "did not reuse winner for {kind} temporary"); + assert_eq!(std::fs::read(&out).unwrap(), b"complete winner", "changed winner for {kind} temporary"); + if kind == "directory" { + assert_eq!(std::fs::read(tmp.join("child")).unwrap(), b"caller owned"); + std::fs::remove_file(tmp.join("child")).unwrap(); + std::fs::remove_dir(&tmp).unwrap(); + } else { + assert!(!tmp.exists(), "left {kind} temporary file"); + } + std::fs::remove_file(&out).unwrap(); + std::fs::remove_dir(&dir).unwrap(); + } + } + + // These callbacks expose an inherited generate validation gap: success alone + // does not establish that the generated path is a complete fixture file. + #[test] + fn generate_rejects_successful_callback_producing_empty_file() { + let dir = temp_path(&std::env::temp_dir().join("filmcraft-testkit-generate-empty")); + std::fs::create_dir_all(&dir).unwrap(); + let out = dir.join("fixture.bin"); + let mut temporary = None; + let result = generate(&out, |tmp| { + temporary = Some(tmp.to_path_buf()); + std::fs::write(tmp, b"").unwrap(); + true + }); + assert!(result.is_none()); + assert!(!out.exists()); + assert!(!temporary.unwrap().exists()); + std::fs::remove_dir(&dir).unwrap(); + } + + #[test] + fn generate_rejects_successful_callback_producing_directory() { + let dir = temp_path(&std::env::temp_dir().join("filmcraft-testkit-generate-directory")); + std::fs::create_dir_all(&dir).unwrap(); + let out = dir.join("fixture.bin"); + let mut temporary = None; + let result = generate(&out, |tmp| { + temporary = Some(tmp.to_path_buf()); + std::fs::create_dir(tmp).unwrap(); + std::fs::write(tmp.join("child"), b"caller owned").unwrap(); + true + }); + assert!(result.is_none()); + assert!(!out.exists()); + let tmp = temporary.unwrap(); + assert!(tmp.is_dir()); + assert_eq!(std::fs::read(tmp.join("child")).unwrap(), b"caller owned"); + std::fs::remove_file(tmp.join("child")).unwrap(); + std::fs::remove_dir(&tmp).unwrap(); + std::fs::remove_dir(&dir).unwrap(); + } + #[test] fn root_is_the_workspace() { assert!(workspace_root().join("Cargo.toml").exists()); @@ -133,4 +304,54 @@ mod tests { let p2 = generate(&out, |_| panic!("regenerated")).unwrap(); assert_eq!(p, p2); } + + #[cfg(windows)] + #[test] + fn concurrent_complete_fixture_survives_blocked_publication() { + use std::os::windows::fs::OpenOptionsExt; + + let dir = temp_path(&std::env::temp_dir().join("filmcraft-testkit-publish")); + std::fs::create_dir_all(&dir).unwrap(); + let out = dir.join("fixture.bin"); + let mut reader = None; + let mut temporary = None; + let result = generate(&out, |tmp| { + temporary = Some(tmp.to_path_buf()); + std::fs::write(&out, b"complete winner").unwrap(); + // Simulate another test reading the winner without delete sharing. + reader = Some(std::fs::OpenOptions::new().read(true).share_mode(1).open(&out).unwrap()); + std::fs::write(tmp, b"complete candidate").is_ok() + }); + assert_eq!(result.as_deref(), Some(out.as_path())); + assert_eq!(std::fs::read(&out).unwrap(), b"complete winner"); + assert!(!temporary.unwrap().exists()); + drop(reader); + std::fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn failed_publication_does_not_accept_a_directory_or_leave_temporary() { + let dir = temp_path(&std::env::temp_dir().join("filmcraft-testkit-publish-directory")); + std::fs::create_dir_all(&dir).unwrap(); + let out = dir.join("fixture.bin"); + let mut temporary = None; + let result = generate(&out, |tmp| { + temporary = Some(tmp.to_path_buf()); + std::fs::create_dir_all(&out).unwrap(); + std::fs::write(out.join("child"), b"not a fixture").unwrap(); + std::fs::write(tmp, b"candidate").is_ok() + }); + assert!(result.is_none()); + assert!(!temporary.unwrap().exists()); + let mut called = false; + assert!( + generate(&out, |_| { + called = true; + false + }) + .is_none() + ); + assert!(called, "a directory must not be reused as a cached file"); + std::fs::remove_dir_all(&dir).unwrap(); + } }