From 64f3df7d729b3d515fcab582a2bb67e5f6b26b7d Mon Sep 17 00:00:00 2001 From: Fabian Hamm Date: Thu, 10 Sep 2026 20:42:44 +0200 Subject: [PATCH] test(formats): make $SYLPHEED_DISC an actual control (#16 remedy 3) Before this commit, `unset SYLPHEED_DISC` did not disable the disc-backed suites on the machine that has the disc: every `disc_root()` fell back to a hardcoded absolute path that exists on this box. The env var looked like a control and was not one. Same for $SYLPHEED_RES3D and $SYLPHEED_ISO. Replace the duplicated resolvers with one `tests/common/mod.rs`: - 17 local `disc_root()` definitions -> 1 - 7 copies of the skip macro -> 1 (`skip_without_disc!` and siblings) - 16 hardcoded absolute paths -> 0 executable ones (3 of those were inline in `mesh_disc.rs`, in no resolver at all, and 2 were in `examples/`) - `corpus_report.rs` now reports on the SAME resolver the suites use, instead of a second copy of the logic its own comments flagged as a drift risk. The 17 copies had already drifted into FIVE variants, and they were not all the same function. `movie_manifest_disc`, `movie_subtitle_disc` and `slb_disc` honoured $SYLPHEED_DISC and nothing else, while the other 14 fell back. So one name already meant two things -- a third instance of the shape #16 is about. The shared helper adopts the env-only behaviour those three already had, rather than inventing a sixth variant. Two module docs still described the fallback after it was deleted, which is the same defect in prose: `texture_disc` claimed "or the default dev path exists" and `pak_idxd_disc` said "or drop it at the default dev path below". Both now say what the code does. Verified both ways on the machine that HAS the corpus, which is the only place this refactor can be falsified: A env unset -> "ABSENT -- $SYLPHEED_DISC unset; its suites self-skip" suites=31 passed=209 failed=0 ignored=14, slowest 0.12s B env set -> "PRESENT via $SYLPHEED_DISC" (all three corpora) suites=31 passed=209 failed=0 ignored=14, slowest 1235.53s (mesh_consistency_disc) Identical tallies, opposite corpus states, ~10000x apart in wall clock. (A) is new behaviour -- it was previously unreachable here. (B) proves nothing broke. `just test-disc` sources `.env` (already gitignored) for the set case. Note the quoting trap documented there: the corpus paths contain spaces, and an unquoted `VAR=a b c` parses as "run command `b`", failing silently into ABSENT -- which looks exactly like a working skip. Remedy (1) (`#[ignore]` + `--ignored`) is deliberately NOT done here: (3) already moves the mode from the filesystem into the environment, and `#[ignore]` already carries three meanings in this directory (corpus-absent, known-failing, bare). Overloading it a fourth time would re-create the defect. `cargo fmt --all --check` clean; no new compiler warnings. Refs #16 Co-Authored-By: Claude Opus 5 --- .../examples/default_owners.rs | 8 +- .../examples/defaulted_fields.rs | 7 +- .../tests/caption_families_disc.rs | 24 +-- crates/sylpheed-formats/tests/common/mod.rs | 79 +++++++++ .../sylpheed-formats/tests/corpus_report.rs | 160 +++++++----------- .../tests/idxd_records_disc.rs | 23 +-- .../tests/ixud_records_disc.rs | 23 +-- .../tests/mesh_consistency_disc.rs | 19 +-- crates/sylpheed-formats/tests/mesh_disc.rs | 39 ++--- .../tests/movie_manifest_disc.rs | 6 +- .../tests/movie_subtitle_disc.rs | 8 +- .../sylpheed-formats/tests/pak_idxd_disc.rs | 34 +--- crates/sylpheed-formats/tests/slb_disc.rs | 6 +- .../tests/slb_leading_segment_disc.rs | 25 +-- crates/sylpheed-formats/tests/texture_disc.rs | 19 +-- .../tests/ui_focus_kind_disc.rs | 19 +-- .../tests/ui_header_time_disc.rs | 17 +- .../tests/ui_opt_link_disc.rs | 17 +- .../tests/ui_paint_order_disc.rs | 26 +-- .../tests/ui_prm_primitives_disc.rs | 17 +- .../tests/ui_screen_vs_fragment_disc.rs | 17 +- .../tests/ui_surfaces_disc.rs | 26 +-- .../tests/unit_layout_disc.rs | 19 +-- justfile | 22 +++ 24 files changed, 239 insertions(+), 421 deletions(-) create mode 100644 crates/sylpheed-formats/tests/common/mod.rs diff --git a/crates/sylpheed-formats/examples/default_owners.rs b/crates/sylpheed-formats/examples/default_owners.rs index c1f64db2..820b6882 100644 --- a/crates/sylpheed-formats/examples/default_owners.rs +++ b/crates/sylpheed-formats/examples/default_owners.rs @@ -25,8 +25,12 @@ fn is_value(s: &str) -> bool { } fn main() { - let root = std::env::var("SYLPHEED_DISC") - .unwrap_or_else(|_| "/home/fabi/RE - Project Sylpheed/sylph_extract".into()); + // No hardcoded fallback: it made SYLPHEED_DISC look like a control while + // one machine's directory layout decided the outcome (#16). + let Ok(root) = std::env::var("SYLPHEED_DISC") else { + eprintln!("set SYLPHEED_DISC to the extracted disc root"); + std::process::exit(2); + }; let wanted: Vec = std::env::args().skip(1).collect(); let arc = PakArchive::open(std::path::Path::new(&root).join("dat/GP_MAIN_GAME_E.pak")).unwrap(); diff --git a/crates/sylpheed-formats/examples/defaulted_fields.rs b/crates/sylpheed-formats/examples/defaulted_fields.rs index 3a554eea..379bf08d 100644 --- a/crates/sylpheed-formats/examples/defaulted_fields.rs +++ b/crates/sylpheed-formats/examples/defaulted_fields.rs @@ -31,9 +31,14 @@ fn is_value(s: &str) -> bool { } fn main() { + // argv[1], else SYLPHEED_DISC. No hardcoded fallback -- see #16. let root = std::env::args() .nth(1) - .unwrap_or_else(|| "/home/fabi/RE - Project Sylpheed/sylph_extract".into()); + .or_else(|| std::env::var("SYLPHEED_DISC").ok()) + .unwrap_or_else(|| { + eprintln!("usage: defaulted_fields [paks...] (or set SYLPHEED_DISC)"); + std::process::exit(2); + }); let paks: Vec = std::env::args().skip(2).collect(); let paks = if paks.is_empty() { vec![ diff --git a/crates/sylpheed-formats/tests/caption_families_disc.rs b/crates/sylpheed-formats/tests/caption_families_disc.rs index f344c998..d3f4f199 100644 --- a/crates/sylpheed-formats/tests/caption_families_disc.rs +++ b/crates/sylpheed-formats/tests/caption_families_disc.rs @@ -4,31 +4,11 @@ //! `build_caption_text` generalises the key parser to all eight. use std::collections::BTreeMap; -use std::path::{Path, PathBuf}; use sylpheed_formats::{movie_subtitle, PakArchive}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let d = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - d.join("dat").is_dir().then(|| d.to_path_buf()) -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: set SYLPHEED_DISC"); - return; - }; - }; -} +mod common; +use common::skip_without_disc; #[test] fn all_eight_caption_families_are_read() { diff --git a/crates/sylpheed-formats/tests/common/mod.rs b/crates/sylpheed-formats/tests/common/mod.rs new file mode 100644 index 00000000..649e016f --- /dev/null +++ b/crates/sylpheed-formats/tests/common/mod.rs @@ -0,0 +1,79 @@ +//! One place that decides where the disc corpora are — issue #16, remedy (3). +//! +//! # What this replaces +//! +//! Seventeen files under `tests/` each defined their own `disc_root()`, and they +//! had **already drifted into five variants**. Four were the same thing written +//! four ways (differing only in return type and style). The fifth — +//! `movie_manifest_disc`, `movie_subtitle_disc`, `slb_disc` — did something +//! materially different: it honoured `SYLPHEED_DISC` **and nothing else**. +//! +//! So one function name meant two different things in one directory, which is +//! the same "one name, several meanings" defect #16 identifies in +//! `SYLPHEED_DISC` itself and in `#[ignore]`. +//! +//! # Why the env var, and no fallback +//! +//! The fourteen copies with a fallback hardcoded one machine's absolute layout: +//! +//! ```text +//! /home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja) +//! ``` +//! +//! That made `unset SYLPHEED_DISC` a no-op there: whether the disc suites ran +//! was a property of *the machine's directory layout*, invisible in the command +//! and in the output. The env var looked like a control and was not one. +//! +//! This module adopts the behaviour three of those files already had, rather +//! than inventing a new one: **the environment decides, always.** Point +//! `SYLPHEED_DISC` at the extracted disc and the suites run; leave it unset and +//! they skip. Same command, same answer, on every machine. +//! +//! `just test-disc` reads `.env` (already gitignored as a local dev override) +//! so no absolute path has to live in the source tree again. +//! +//! # This does not fix the tally +//! +//! A skipped suite still counts as `passed` — `#[ignore]` is static and cannot +//! move at runtime. That is why `tests/corpus_report.rs` exists: it prints which +//! corpora resolved, and it is the thing to read. This module only makes the +//! *control* honest, so that report can now say `PRESENT via $SYLPHEED_DISC` +//! and mean it. +// `tests/common/mod.rs` is compiled into EVERY integration-test binary, and each +// one uses only the resolver (and maybe the macro) it needs. Without these, every +// binary warns about the parts it did not use. +#![allow(dead_code, unused_macros, unused_imports)] + +use std::path::PathBuf; + +/// The extracted disc root — the directory containing `dat/`. +pub fn disc_root() -> Option { + let p = PathBuf::from(std::env::var("SYLPHEED_DISC").ok()?); + p.join("dat").is_dir().then_some(p) +} + +/// The extracted `resource3d` directory (`Stage_SNN.xpr` models). +pub fn res3d_dir() -> Option { + let p = PathBuf::from(std::env::var("SYLPHEED_RES3D").ok()?); + p.is_dir().then_some(p) +} + +/// The retail ISO image itself, not a directory. +pub fn iso_path() -> Option { + let p = PathBuf::from(std::env::var("SYLPHEED_ISO").ok()?); + p.is_file().then_some(p) +} + +/// Bind the disc root or return from the test. +/// +/// The early return keeps the test *passing*, which is why the tally cannot +/// distinguish a skip from a real run — see `corpus_report.rs`. +macro_rules! skip_without_disc { + ($root:ident) => { + let Some($root) = crate::common::disc_root() else { + eprintln!("SKIP: set SYLPHEED_DISC to the extracted disc root"); + return; + }; + }; +} +pub(crate) use skip_without_disc; diff --git a/crates/sylpheed-formats/tests/corpus_report.rs b/crates/sylpheed-formats/tests/corpus_report.rs index cc6221f6..e69da0b2 100644 --- a/crates/sylpheed-formats/tests/corpus_report.rs +++ b/crates/sylpheed-formats/tests/corpus_report.rs @@ -3,94 +3,62 @@ //! # Why this exists //! //! `cargo test --workspace` reports **the same tally whether or not the disc -//! corpus was exercised** — see issue #16. Measured: the disc suites ran on a -//! developer desktop (1 936 s, `mesh_consistency_disc` alone 1 220 s) and -//! skipped on CI (2.4 s total), and *both* reported `207 passed / 0 failed / -//! 14 ignored` across 30 suites. +//! corpus was exercised** — issue #16. Measured on `main` @ `4ca0b8e`: this +//! desktop ran the disc suites (`mesh_consistency_disc` alone **1 215 s**) and +//! CI skipped them (2.4 s total), and *both* reported +//! `209 passed / 0 failed / 14 ignored` across 31 suites. //! -//! Two mechanisms compound, and either alone would be survivable: +//! Two mechanisms compound: //! -//! 1. **A skip is a passing test.** The disc suites do `eprintln!("SKIP: …")` -//! and return early from a test that still passes, so a skipped suite and a -//! fully exercised one both score `1 passed`. The totals are invariant. -//! 2. **The message is invisible.** `cargo test` captures a *passing* test's -//! output, so neither log contains a `SKIP:` line. The absence of one proves -//! nothing, which makes the obvious check useless too. +//! 1. **A skip is a passing test.** The gated suites print `SKIP:` and return +//! early from a test that still passes, so a skipped suite and a fully +//! exercised one both score `1 passed`. +//! 2. **The message is invisible.** `cargo test` captures a passing test's +//! output, so neither log contains a `SKIP:` line. //! //! And `14 ignored` cannot help: `#[ignore]` is static, so that column is the //! literal count of attributes in the source and cannot move at runtime. //! -//! This test is the fix for the *report*, not for the control. It always runs, -//! never fails, and records what was actually available — to a file, because a -//! passing test's stdout is captured and would be invisible in exactly the CI -//! log that needs it. +//! This test fixes the *report*. `tests/common/mod.rs` fixes the *control*. use std::fmt::Write as _; use std::io::Write as _; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; -/// Resolution mirrors the per-suite helpers exactly. If one of those changes, -/// this drifts — which is itself an argument for the shared helper in #16's -/// remedy (3). -fn resolve( - env: &str, - fallback: &str, - want_dir_child: Option<&str>, - want_file: bool, -) -> (String, Option) { - let ok = |p: &Path| -> bool { - match (want_dir_child, want_file) { - (Some(child), _) => p.join(child).is_dir(), - (None, true) => p.is_file(), - (None, false) => p.is_dir(), - } - }; - if let Ok(v) = std::env::var(env) { - let p = PathBuf::from(&v); - if ok(&p) { - return (format!("PRESENT via ${env}"), Some(p)); - } - return (format!("${env} is set but does not resolve: {v}"), None); - } - let p = PathBuf::from(fallback); - if ok(&p) { - // The important case. ${env} is unset, yet the corpus resolved anyway, - // so the suites run because of this machine's directory layout — a - // property invisible in the command and in the output. - return ( - format!("PRESENT via the HARDCODED fallback, NOT ${env}"), - Some(p), - ); - } - ( - "ABSENT — the gated suites will self-skip and still count as passed".into(), - None, - ) -} +mod common; -fn target_dir() -> Option { - let exe = std::env::current_exe().ok()?; - exe.ancestors() - .find(|a| a.file_name().is_some_and(|n| n == "target")) - .map(PathBuf::from) +/// Describe one corpus. Resolution is delegated to `common`, so this cannot +/// drift from what the suites themselves do — the previous version duplicated +/// the resolution logic and said so in its own comments. +fn status(var: &str, resolved: Option) -> (String, Option) { + match (std::env::var(var), resolved) { + (Ok(_), Some(p)) => (format!("PRESENT via ${var}"), Some(p)), + // Set but unusable is NOT the same as absent, and wants a different + // fix: a typo or a moved directory rather than a machine without the + // corpus. Reporting them alike would send someone hunting the wrong one. + (Ok(v), None) => (format!("${var} is set but does not resolve: {v}"), None), + (Err(_), _) => ( + format!("ABSENT — ${var} unset; its suites self-skip and still count as passed"), + None, + ), + } } #[test] fn corpus_report() { - let corpora = [ - ("SYLPHEED_DISC", "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", Some("dat"), false), - ("SYLPHEED_RES3D", "/home/fabi/RE - Project Sylpheed/sylph_extract/hidden/resource3d", None, false), - ("SYLPHEED_ISO", "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja).iso", None, true), + let rows = [ + status("SYLPHEED_DISC", common::disc_root()), + status("SYLPHEED_RES3D", common::res3d_dir()), + status("SYLPHEED_ISO", common::iso_path()), ]; let mut out = String::from("test corpus report (issue #16)\n"); let mut any = false; - for (env, fallback, child, file) in corpora { - let (status, path) = resolve(env, fallback, child, file); + for (text, path) in &rows { any |= path.is_some(); - let _ = writeln!(out, " {env:<15} {status}"); + let _ = writeln!(out, " {text}"); if let Some(p) = path { - let _ = writeln!(out, " {:<15} -> {}", "", p.display()); + let _ = writeln!(out, " -> {}", p.display()); } } let _ = writeln!( @@ -107,15 +75,14 @@ fn corpus_report() { ); } - // Captured for a passing test, so it is only visible with --show-output or - // --nocapture. Kept anyway: it is the natural place to look locally. + // Captured for a passing test, so visible only with --show-output. Kept + // because it is the natural place to look locally. println!("{out}"); // The channel that survives capture, and the one CI reads. if let Some(dir) = target_dir() { let _ = std::fs::write(dir.join("sylpheed-corpus-report.txt"), &out); } - // Gitea and GitHub both honour this; it puts the block in the run summary. if let Ok(p) = std::env::var("GITHUB_STEP_SUMMARY") { if let Ok(mut f) = std::fs::OpenOptions::new() .create(true) @@ -127,31 +94,30 @@ fn corpus_report() { } } -/// The ABSENT branch is the one CI takes, and it cannot be reached on a machine -/// that has the corpora — so it is exercised directly here rather than shipped -/// unrun. Same for the "set but does not resolve" branch, which is what a typo -/// in the env var produces. -#[test] -fn resolve_renders_every_branch() { - let missing = "/nonexistent/sylpheed/corpus"; - - // env unset + fallback missing -> ABSENT - std::env::remove_var("SYLPHEED_TEST_PROBE"); - let (status, path) = resolve("SYLPHEED_TEST_PROBE", missing, None, false); - assert!(status.starts_with("ABSENT"), "{status}"); - assert!(path.is_none()); - - // env set to something that does not resolve -> named as such, NOT absent, - // because those two states want different fixes. - std::env::set_var("SYLPHEED_TEST_PROBE", missing); - let (status, path) = resolve("SYLPHEED_TEST_PROBE", missing, None, false); - assert!(status.contains("is set but does not resolve"), "{status}"); - assert!(path.is_none()); - - // env set and valid -> attributed to the env var, not the fallback - std::env::set_var("SYLPHEED_TEST_PROBE", env!("CARGO_MANIFEST_DIR")); - let (status, path) = resolve("SYLPHEED_TEST_PROBE", missing, None, false); - assert_eq!(status, "PRESENT via $SYLPHEED_TEST_PROBE"); - assert!(path.is_some()); - std::env::remove_var("SYLPHEED_TEST_PROBE"); +fn target_dir() -> Option { + let exe = std::env::current_exe().ok()?; + exe.ancestors() + .find(|a| a.file_name().is_some_and(|n| n == "target")) + .map(PathBuf::from) +} + +/// The three states must render distinctly, and CI only ever exercises one of +/// them — so they are driven directly here rather than shipped unrun. +#[test] +fn status_renders_every_state() { + let probe = "SYLPHEED_TEST_PROBE"; + std::env::remove_var(probe); + let (text, path) = status(probe, None); + assert!(text.starts_with("ABSENT"), "{text}"); + assert!(path.is_none()); + + std::env::set_var(probe, "/nonexistent/sylpheed/corpus"); + let (text, _) = status(probe, None); + assert!(text.contains("is set but does not resolve"), "{text}"); + + std::env::set_var(probe, env!("CARGO_MANIFEST_DIR")); + let (text, path) = status(probe, Some(PathBuf::from(env!("CARGO_MANIFEST_DIR")))); + assert_eq!(text, format!("PRESENT via ${probe}")); + assert!(path.is_some()); + std::env::remove_var(probe); } diff --git a/crates/sylpheed-formats/tests/idxd_records_disc.rs b/crates/sylpheed-formats/tests/idxd_records_disc.rs index 3c84e01d..48bf42d8 100644 --- a/crates/sylpheed-formats/tests/idxd_records_disc.rs +++ b/crates/sylpheed-formats/tests/idxd_records_disc.rs @@ -13,27 +13,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::hash::tag_hash; use sylpheed_formats::{IdxdObject, PakArchive}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - default.join("dat").is_dir().then(|| default.to_path_buf()) -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: extracted disc not found (set SYLPHEED_DISC to enable)"); - return; - }; - }; -} +mod common; +use common::skip_without_disc; /// Every `.pak` on the disc, recursively. /// diff --git a/crates/sylpheed-formats/tests/ixud_records_disc.rs b/crates/sylpheed-formats/tests/ixud_records_disc.rs index c833ad57..bd465dc4 100644 --- a/crates/sylpheed-formats/tests/ixud_records_disc.rs +++ b/crates/sylpheed-formats/tests/ixud_records_disc.rs @@ -8,27 +8,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::hash::ixud_hash_str; use sylpheed_formats::{IxudObject, PakArchive}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let d = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - d.join("dat").is_dir().then(|| d.to_path_buf()) -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: set SYLPHEED_DISC"); - return; - }; - }; -} +mod common; +use common::skip_without_disc; fn all_paks(root: &Path) -> Vec { let mut out = Vec::new(); diff --git a/crates/sylpheed-formats/tests/mesh_consistency_disc.rs b/crates/sylpheed-formats/tests/mesh_consistency_disc.rs index cc0b73a4..0e6a4ea0 100644 --- a/crates/sylpheed-formats/tests/mesh_consistency_disc.rs +++ b/crates/sylpheed-formats/tests/mesh_consistency_disc.rs @@ -11,25 +11,12 @@ //! rather than as a snapshot of the bug. use std::collections::BTreeMap; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use sylpheed_formats::mesh::Xbg7Model; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} +mod common; +use common::disc_root; /// Rounded (w, h, d) of a model's own geometry. fn span(m: &Xbg7Model) -> Option<[i64; 3]> { diff --git a/crates/sylpheed-formats/tests/mesh_disc.rs b/crates/sylpheed-formats/tests/mesh_disc.rs index ea4ec8df..19d1aa72 100644 --- a/crates/sylpheed-formats/tests/mesh_disc.rs +++ b/crates/sylpheed-formats/tests/mesh_disc.rs @@ -9,16 +9,8 @@ use std::path::PathBuf; use sylpheed_formats::mesh::{material_groups, node_transforms, submesh_albedos, Xbg7Model}; -fn res3d_dir() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_RES3D") { - let p = PathBuf::from(p); - if p.is_dir() { - return Some(p); - } - } - let default = PathBuf::from("/home/fabi/RE - Project Sylpheed/sylph_extract/hidden/resource3d"); - default.is_dir().then_some(default) -} +mod common; +use common::res3d_dir; #[test] #[ignore = "requires extracted disc models — set SYLPHEED_RES3D"] @@ -378,10 +370,11 @@ fn hero_ship_grouped_pool_decodes() { #[ignore] fn stage_models_decode() { use sylpheed_formats::mesh::Xbg7Model; - let dir = std::env::var("SYLPHEED_RES3D").unwrap_or_else(|_| { - "/home/fabi/RE - Project Sylpheed/sylph_extract/hidden/resource3d".to_string() - }); - let path = format!("{dir}/Stage_S10.xpr"); + let Some(dir) = res3d_dir() else { + eprintln!("SKIP: set SYLPHEED_RES3D to the extracted resource3d directory"); + return; + }; + let path = format!("{}/Stage_S10.xpr", dir.display()); let bytes = std::fs::read(&path).expect("read Stage_S10"); let models = Xbg7Model::stage_models(&bytes); for m in &models { @@ -423,9 +416,10 @@ fn stage_models_decode() { fn stage_models_sweep() { use std::time::Instant; use sylpheed_formats::mesh::Xbg7Model; - let dir = std::env::var("SYLPHEED_RES3D").unwrap_or_else(|_| { - "/home/fabi/RE - Project Sylpheed/sylph_extract/hidden/resource3d".to_string() - }); + let Some(dir) = res3d_dir() else { + eprintln!("SKIP: set SYLPHEED_RES3D to the extracted resource3d directory"); + return; + }; let mut names: Vec<_> = std::fs::read_dir(&dir) .unwrap() .filter_map(|e| e.ok().map(|e| e.file_name().into_string().unwrap())) @@ -434,7 +428,7 @@ fn stage_models_sweep() { names.sort(); let mut tot = 0usize; for n in &names { - let bytes = std::fs::read(format!("{dir}/{n}")).unwrap(); + let bytes = std::fs::read(format!("{}/{n}", dir.display())).unwrap(); let t0 = Instant::now(); let models = Xbg7Model::stage_models(&bytes); let dt = t0.elapsed().as_millis(); @@ -455,10 +449,11 @@ fn stage_models_sweep() { #[ignore] fn stage_models_quality_audit() { use sylpheed_formats::mesh::Xbg7Model; - let dir = std::env::var("SYLPHEED_RES3D").unwrap_or_else(|_| { - "/home/fabi/RE - Project Sylpheed/sylph_extract/hidden/resource3d".to_string() - }); - let bytes = std::fs::read(format!("{dir}/Stage_S07.xpr")).unwrap(); + let Some(dir) = res3d_dir() else { + eprintln!("SKIP: set SYLPHEED_RES3D to the extracted resource3d directory"); + return; + }; + let bytes = std::fs::read(format!("{}/Stage_S07.xpr", dir.display())).unwrap(); let models = Xbg7Model::stage_models(&bytes); let (mut small, mut mid, mut huge, mut dupnames) = (0, 0, 0, 0); let mut seen = std::collections::HashSet::new(); diff --git a/crates/sylpheed-formats/tests/movie_manifest_disc.rs b/crates/sylpheed-formats/tests/movie_manifest_disc.rs index bdea2406..36d03a68 100644 --- a/crates/sylpheed-formats/tests/movie_manifest_disc.rs +++ b/crates/sylpheed-formats/tests/movie_manifest_disc.rs @@ -7,10 +7,8 @@ use sylpheed_formats::movie_manifest; use sylpheed_formats::slb::VoiceLang; use sylpheed_formats::PakArchive; -fn disc_root() -> Option { - let p = PathBuf::from(std::env::var("SYLPHEED_DISC").ok()?); - p.join("dat").is_dir().then_some(p) -} +mod common; +use common::disc_root; /// Read the manifest + `eng\sounds.tbl` out of `tables.pak`. fn load_manifest_and_sounds(root: &PathBuf) -> (Vec, Vec) { diff --git a/crates/sylpheed-formats/tests/movie_subtitle_disc.rs b/crates/sylpheed-formats/tests/movie_subtitle_disc.rs index 760874f5..6b0640c6 100644 --- a/crates/sylpheed-formats/tests/movie_subtitle_disc.rs +++ b/crates/sylpheed-formats/tests/movie_subtitle_disc.rs @@ -1,15 +1,11 @@ //! Real-disc test for the movie subtitle chain. Skipped when the extracted disc //! is absent (set `SYLPHEED_DISC` to the extract root to enable). -use std::path::PathBuf; - use sylpheed_formats::movie_subtitle::{self, SubLang}; use sylpheed_formats::PakArchive; -fn disc_root() -> Option { - let p = PathBuf::from(std::env::var("SYLPHEED_DISC").ok()?); - p.join("dat").is_dir().then_some(p) -} +mod common; +use common::disc_root; #[test] fn resolves_english_radio_subtitles() { diff --git a/crates/sylpheed-formats/tests/pak_idxd_disc.rs b/crates/sylpheed-formats/tests/pak_idxd_disc.rs index b7e14d61..d433d51f 100644 --- a/crates/sylpheed-formats/tests/pak_idxd_disc.rs +++ b/crates/sylpheed-formats/tests/pak_idxd_disc.rs @@ -1,40 +1,16 @@ //! Integration tests against the real extracted Project Sylpheed disc. //! //! These are **skipped** (pass as no-ops) when the extracted disc is not present, -//! so the suite still runs on machines/CI without the game. Point at the disc via -//! the `SYLPHEED_DISC` env var, or drop it at the default dev path below. - -use std::path::{Path, PathBuf}; +//! so the suite still runs on machines/CI without the game. Point at the disc +//! with the `SYLPHEED_DISC` env var; there is no path fallback (see #16). use sylpheed_formats::texture::X360Texture; use sylpheed_formats::{IdxdObject, PakArchive}; +mod common; +use common::skip_without_disc; + /// Locate the extracted disc root, or `None` to skip. -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: extracted disc not found (set SYLPHEED_DISC to enable)"); - return; - }; - }; -} - #[test] fn deftables_header_and_first_entry() { skip_without_disc!(root); diff --git a/crates/sylpheed-formats/tests/slb_disc.rs b/crates/sylpheed-formats/tests/slb_disc.rs index e17b4e56..c584335b 100644 --- a/crates/sylpheed-formats/tests/slb_disc.rs +++ b/crates/sylpheed-formats/tests/slb_disc.rs @@ -9,10 +9,8 @@ use sylpheed_formats::hash::name_hash; use sylpheed_formats::slb::{self, VoiceLang}; use sylpheed_formats::PakArchive; -fn disc_root() -> Option { - let p = PathBuf::from(std::env::var("SYLPHEED_DISC").ok()?); - p.join("dat").is_dir().then_some(p) -} +mod common; +use common::disc_root; /// Read `[off, off+size)` from `dat/sound.p00..` (segments concatenated). fn read_range(root: &PathBuf, mut off: u64, size: usize) -> Vec { diff --git a/crates/sylpheed-formats/tests/slb_leading_segment_disc.rs b/crates/sylpheed-formats/tests/slb_leading_segment_disc.rs index c5e69f6b..a9204206 100644 --- a/crates/sylpheed-formats/tests/slb_leading_segment_disc.rs +++ b/crates/sylpheed-formats/tests/slb_leading_segment_disc.rs @@ -5,31 +5,12 @@ //! `RIFF`. The banks that looked fine were the ones whose leading segment is //! silence. One rule, two outcomes. -use std::path::{Path, PathBuf}; +use std::path::Path; use sylpheed_formats::{slb, PakArchive}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - default.join("dat").is_dir().then(|| default.to_path_buf()) -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: set SYLPHEED_DISC"); - return; - }; - }; -} +mod common; +use common::skip_without_disc; fn bank(root: &Path, n: u32) -> Vec { let snd = PakArchive::open(root.join("dat/sound.pak")).expect("sound.pak"); diff --git a/crates/sylpheed-formats/tests/texture_disc.rs b/crates/sylpheed-formats/tests/texture_disc.rs index 6e552ae9..0c8525ac 100644 --- a/crates/sylpheed-formats/tests/texture_disc.rs +++ b/crates/sylpheed-formats/tests/texture_disc.rs @@ -1,28 +1,15 @@ //! Integration test: run the XPR2 texture pipeline against REAL `.xpr` files //! read directly from the retail disc image, reproducing exactly what the //! viewer's texture-preview path does (`identify_format` → `X360Texture:: -//! from_xpr2`). Skipped unless `SYLPHEED_ISO` points at the disc (or the -//! default dev path exists). +//! from_xpr2`). Skipped unless `SYLPHEED_ISO` points at the disc image. //! //! Run: `cargo test -p sylpheed-formats --test texture_disc -- --ignored --nocapture` -use std::path::PathBuf; - use sylpheed_formats::texture::X360Texture; use sylpheed_formats::vfs::identify_format; -fn iso_path() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_ISO") { - let p = PathBuf::from(p); - if p.is_file() { - return Some(p); - } - } - let default = PathBuf::from( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja).iso", - ); - default.is_file().then_some(default) -} +mod common; +use common::iso_path; #[tokio::test] #[ignore = "requires the retail ISO — set SYLPHEED_ISO"] diff --git a/crates/sylpheed-formats/tests/ui_focus_kind_disc.rs b/crates/sylpheed-formats/tests/ui_focus_kind_disc.rs index b2498463..2ef79795 100644 --- a/crates/sylpheed-formats/tests/ui_focus_kind_disc.rs +++ b/crates/sylpheed-formats/tests/ui_focus_kind_disc.rs @@ -17,25 +17,12 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::{pak::PakArchive, ratc, ui_layout}; +mod common; +use common::disc_root; + const DECL_TABLE_AT: usize = 0x20; const DECL_ENTRY: usize = 60; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} - fn for_each_build(root: &Path, mut f: impl FnMut(&str, &[u8])) { let mut paks: Vec = std::fs::read_dir(root.join("dat")) .expect("dat/") diff --git a/crates/sylpheed-formats/tests/ui_header_time_disc.rs b/crates/sylpheed-formats/tests/ui_header_time_disc.rs index d9eb8e0b..3d7e7c65 100644 --- a/crates/sylpheed-formats/tests/ui_header_time_disc.rs +++ b/crates/sylpheed-formats/tests/ui_header_time_disc.rs @@ -16,21 +16,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::{pak::PakArchive, ratc, ui_layout}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} +mod common; +use common::disc_root; fn for_each_build(root: &Path, mut f: impl FnMut(&str, &[u8])) { let mut paks: Vec = std::fs::read_dir(root.join("dat")) diff --git a/crates/sylpheed-formats/tests/ui_opt_link_disc.rs b/crates/sylpheed-formats/tests/ui_opt_link_disc.rs index d1c54833..2dcd0d1b 100644 --- a/crates/sylpheed-formats/tests/ui_opt_link_disc.rs +++ b/crates/sylpheed-formats/tests/ui_opt_link_disc.rs @@ -17,21 +17,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::{pak::PakArchive, ratc, ui_layout}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} +mod common; +use common::disc_root; fn for_each_build(root: &Path, mut f: impl FnMut(&str, &[u8])) { let mut paks: Vec = std::fs::read_dir(root.join("dat")) diff --git a/crates/sylpheed-formats/tests/ui_paint_order_disc.rs b/crates/sylpheed-formats/tests/ui_paint_order_disc.rs index 7f280a32..2804792b 100644 --- a/crates/sylpheed-formats/tests/ui_paint_order_disc.rs +++ b/crates/sylpheed-formats/tests/ui_paint_order_disc.rs @@ -21,30 +21,8 @@ use sylpheed_formats::{ ui_layout::{self, ComposeOptions}, }; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: extracted disc not found (set SYLPHEED_DISC to enable)"); - return; - }; - }; -} +mod common; +use common::skip_without_disc; /// Every parseable screen build on the disc, as (pak name, bundle bytes). fn builds(root: &Path) -> Vec<(String, Vec)> { diff --git a/crates/sylpheed-formats/tests/ui_prm_primitives_disc.rs b/crates/sylpheed-formats/tests/ui_prm_primitives_disc.rs index 9b8e4357..6e3e0198 100644 --- a/crates/sylpheed-formats/tests/ui_prm_primitives_disc.rs +++ b/crates/sylpheed-formats/tests/ui_prm_primitives_disc.rs @@ -9,21 +9,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::{pak::PakArchive, ratc, ui_layout}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} +mod common; +use common::disc_root; fn for_each_build(root: &Path, mut f: impl FnMut(&str, &[u8])) { let mut paks: Vec = std::fs::read_dir(root.join("dat")) diff --git a/crates/sylpheed-formats/tests/ui_screen_vs_fragment_disc.rs b/crates/sylpheed-formats/tests/ui_screen_vs_fragment_disc.rs index 9e8bcec7..f2d63c37 100644 --- a/crates/sylpheed-formats/tests/ui_screen_vs_fragment_disc.rs +++ b/crates/sylpheed-formats/tests/ui_screen_vs_fragment_disc.rs @@ -21,21 +21,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::{pak::PakArchive, ratc, ui_layout}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} +mod common; +use common::disc_root; fn for_each_build(root: &Path, mut f: impl FnMut(&str, &[u8])) { let mut paks: Vec = std::fs::read_dir(root.join("dat")) diff --git a/crates/sylpheed-formats/tests/ui_surfaces_disc.rs b/crates/sylpheed-formats/tests/ui_surfaces_disc.rs index 5acb1e5f..fdd1a024 100644 --- a/crates/sylpheed-formats/tests/ui_surfaces_disc.rs +++ b/crates/sylpheed-formats/tests/ui_surfaces_disc.rs @@ -10,30 +10,8 @@ use std::path::{Path, PathBuf}; use sylpheed_formats::{lsta, pak::PakArchive, ratc, t8ad}; -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - let p = PathBuf::from(p); - if p.join("dat").is_dir() { - return Some(p); - } - } - let default = Path::new( - "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)", - ); - if default.join("dat").is_dir() { - return Some(default.to_path_buf()); - } - None -} - -macro_rules! skip_without_disc { - ($root:ident) => { - let Some($root) = disc_root() else { - eprintln!("SKIP: extracted disc not found (set SYLPHEED_DISC to enable)"); - return; - }; - }; -} +mod common; +use common::skip_without_disc; /// Every entry of every pak, plus every RATC child, as raw bytes. fn for_each_blob(root: &Path, mut f: impl FnMut(&str, &str, &[u8])) { diff --git a/crates/sylpheed-formats/tests/unit_layout_disc.rs b/crates/sylpheed-formats/tests/unit_layout_disc.rs index 40012e83..575c2a1e 100644 --- a/crates/sylpheed-formats/tests/unit_layout_disc.rs +++ b/crates/sylpheed-formats/tests/unit_layout_disc.rs @@ -11,6 +11,9 @@ use sylpheed_formats::idxd::IdxdObject; use sylpheed_formats::pak::PakArchive; use sylpheed_formats::unit_layout::{fields, Kind}; +mod common; +use common::disc_root; + /// Live objects identified in the dump, with the disc record each one is. /// `bf001` is here because full-record agreement is what identified it: the /// four-value signature also fitted `UN_be005_ADAN_SpaceFortress`, which @@ -31,19 +34,6 @@ const IDENTIFIED: &[(&str, &str)] = &[ const DUMP: &str = include_str!("../../../docs/re/captures/stage02-live-unit-definitions-deep.txt"); -fn disc_root() -> Option { - if let Ok(p) = std::env::var("SYLPHEED_DISC") { - if std::path::Path::new(&p).join("dat").is_dir() { - return Some(p); - } - } - let d = "/home/fabi/RE - Project Sylpheed/Project Sylpheed - Arc of Deception (USA, Europe) (En,Ja)"; - std::path::Path::new(d) - .join("dat") - .is_dir() - .then(|| d.to_string()) -} - /// `va -> offset -> value`, from the dump's `addr +off hex u32 f32` columns. fn live() -> BTreeMap> { let mut out: BTreeMap> = BTreeMap::new(); @@ -70,7 +60,8 @@ fn mapped_fields_match_the_disc_records() { eprintln!("SKIP: extracted disc not found (set SYLPHEED_DISC to enable)"); return; }; - let pak = PakArchive::open(format!("{disc}/dat/GP_MAIN_GAME_E.pak")).expect("main pak"); + let pak = + PakArchive::open(format!("{}/dat/GP_MAIN_GAME_E.pak", disc.display())).expect("main pak"); let live = live(); let floats: Vec<_> = fields() .into_iter() diff --git a/justfile b/justfile index c79d5829..4034b45f 100644 --- a/justfile +++ b/justfile @@ -71,6 +71,28 @@ sniff-unknown: test: cargo test --workspace +# Run the disc-backed suites. They are gated on the environment ALONE now (#16): +# no source file hardcodes a path any more, so nothing runs by accident because +# a machine happens to have a directory. Put your paths in `.env` (already +# gitignored as a local dev override): +# +# SYLPHEED_DISC="/path/to/extracted/disc" # the dir containing dat/ +# +# QUOTE the values. These paths contain spaces, and an unquoted `VAR=a b c` +# is parsed as "run command b with VAR=a" -- it fails silently into ABSENT. +# SYLPHEED_RES3D=/path/to/hidden/resource3d +# SYLPHEED_ISO=/path/to/game.iso +# +# Read the corpus block the run prints -- the pass/fail tally is identical +# whether or not the corpus was exercised, which is the whole of #16. +test-disc: + #!/usr/bin/env bash + set -euo pipefail + [ -f .env ] && set -a && . ./.env && set +a + cargo test --workspace + echo "--- corpus report ---" + cat target/sylpheed-corpus-report.txt + # Run tests including ISO integration tests (requires SYLPHEED_ISO env var) test-integration: SYLPHEED_ISO=./game.iso cargo test --workspace -- --include-ignored -- 2.49.1