Skip to content

Commit c0db341

Browse files
authored
fix(export): make fixture_or_skip! absence demandable in CI (#2802) (#2835)
* fix(export): let CI demand fixture completeness via IFC_LITE_REQUIRE_FIXTURES fixture_or_skip! (and its fixture_opt backend) already eprintln! a skip message, but cargo test captures stdout/stderr for a passing test and only releases the buffer on failure -- so on a plain `cargo test` the message is written and discarded, and the test looks identical to one that actually asserted something (#2802). Measured on rust/export's lib target: 56 of 228 tests are this kind of no-op pass without `pnpm fixtures`; with fixtures fetched, 227 of 228 run for real (one references a fixture, issues/860_solid_stratum.ifc, that isn't catalogued in tests/models/manifest.json at all). Setting IFC_LITE_REQUIRE_FIXTURES=1 turns a missing fixture into a panic instead of a skip, so CI (which already runs `pnpm fixtures` before `cargo test`) can fail loudly on fixture drift instead of silently running fewer tests. Unset, behavior is unchanged: a fixture-less local `cargo test` remains a legitimate, skipping workflow. Not wired into any workflow here -- that's the maintainer's call, and the stale issues/860_solid_stratum.ifc reference would need fixing (or dropping) before the flag could be turned on without a spurious failure. * fix(export): fail closed on unrecognised IFC_LITE_REQUIRE_FIXTURES values, wire gate into CI louistrue's review on #2835 found two gaps: (1) the env var was never set anywhere, so the gate protected nothing as merged; (2) the value check (`== Ok("1")`) fell through to "off" for any unrecognised truthy spelling (`true`, `yes`, `TRUE`), which is the exact silent-pass failure this PR exists to close. Extract the parse into `require_fixtures()`: unset/empty/"0" stays off (unchanged default), "1" turns the gate on, and anything else panics instead of guessing. Wire `IFC_LITE_REQUIRE_FIXTURES: "1"` into the `rust-tests` job's test step in test.yml, the only job that both fetches fixtures unconditionally and runs `cargo test --workspace` against ifc-lite-export. Note: issues/860_solid_stratum.ifc, referenced by gltf::tests::glb_nodes_have_export_rows_for_legacy_products, has no entry in tests/models/manifest.json and can never be fetched by scripts/fixtures/fetch-fixtures.mjs. Enabling this gate will turn that test — and therefore the rust-tests job — red until that stale reference is resolved. * fix(export): reject non-Unicode IFC_LITE_REQUIRE_FIXTURES instead of silently disabling the gate require_fixtures() matched Err(_) => false, which treated a present but non-Unicode value the same as an unset variable. That let a misconfigured value silently disable the fixture-enforcement gate instead of failing loudly like every other unrecognised value does. Split the pure decision into require_fixtures_from(Result<String, VarError>) so NotPresent and NotUnicode are matched separately, and so a test can cover NotUnicode without mutating the real process environment (which would race other tests calling require_fixtures concurrently).
1 parent 071eb0a commit c0db341

2 files changed

Lines changed: 101 additions & 0 deletions

File tree

.github/workflows/test.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -720,6 +720,11 @@ jobs:
720720
- name: Clippy (lint gate)
721721
run: cargo clippy --workspace --all-targets -- -D warnings
722722
- name: Rust tests
723+
# Fixtures are fetched above (Fetch fixtures / Cache test fixtures),
724+
# so a missing one here is fixture drift, not a legitimate local skip
725+
# (rust/export/src/test_support.rs) — make that a hard failure.
726+
env:
727+
IFC_LITE_REQUIRE_FIXTURES: "1"
723728
run: cargo test --workspace
724729

725730
# Geometry watertightness / triangulation-invariance census.

rust/export/src/test_support.rs

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,63 @@
1414
//! behaviours shipped side by side in the same file.
1515
//!
1616
//! This is the one place that decides. Use [`fixture_or_skip!`] from a test.
17+
//!
18+
//! A skip here is a real `eprintln!`, but `cargo test` captures stdout/stderr
19+
//! for a test that passes and only releases the buffer on failure -- so on a
20+
//! plain `cargo test` (no `--nocapture`) the skip message is written and then
21+
//! thrown away, and the only trace of it is the test showing `ok` like every
22+
//! other one. That is issue #2802: a passing `fixture_or_skip!` test looks
23+
//! identical, in the only output anyone reads, to one that actually asserted
24+
//! something. `IFC_LITE_REQUIRE_FIXTURES=1` closes that for CI: with it set,
25+
//! a missing fixture is a hard `panic!` (a real failure, never captured away)
26+
//! instead of a skip. CI already runs `pnpm fixtures` before `cargo test`, so
27+
//! setting this var there costs nothing on a correct run and turns fixture
28+
//! drift into a loud failure instead of a quietly-smaller test suite. Unset
29+
//! (the default), behavior is unchanged: a fixture-less local `cargo test` is
30+
//! still a legitimate, silently-skipping workflow.
31+
//!
32+
//! The value is parsed strictly (see [`require_fixtures`]): only `"1"` turns
33+
//! the gate on and only unset/empty/`"0"` leave it off. An unrecognised value
34+
//! -- `true`, `yes`, `TRUE` -- panics instead of being treated as either,
35+
//! because falling through to "off" for a typo would recreate exactly the
36+
//! silent-pass problem this module exists to close.
37+
38+
/// Parse `IFC_LITE_REQUIRE_FIXTURES`, failing closed on the config itself.
39+
///
40+
/// Unset, empty, or `"0"` means "off" (the historical default: skip). `"1"`
41+
/// means "on". Anything else -- `true`, `yes`, `TRUE`, a typo -- panics rather
42+
/// than being treated as either value. A `== Ok("1")` check that let
43+
/// unrecognised strings fall through to "off" would land the misconfiguration
44+
/// on the permissive side: `IFC_LITE_REQUIRE_FIXTURES=true` in a workflow file
45+
/// would read as "gate enabled" while actually leaving every fixture test free
46+
/// to skip, silently and indistinguishably from the gate working -- exactly
47+
/// the failure mode this module exists to remove. Guessing "off" for an
48+
/// unrecognised value would reproduce that; refusing to guess does not.
49+
fn require_fixtures() -> bool {
50+
require_fixtures_from(std::env::var("IFC_LITE_REQUIRE_FIXTURES"))
51+
}
52+
53+
/// The pure decision behind [`require_fixtures`], taken as a parameter so
54+
/// tests can exercise every `Result<String, VarError>` shape (including
55+
/// `NotUnicode`) without mutating the real process environment -- `cargo
56+
/// test` runs this crate's tests on multiple threads that share one process,
57+
/// so setting `IFC_LITE_REQUIRE_FIXTURES` for one test would race every other
58+
/// test that calls [`require_fixtures`] concurrently.
59+
fn require_fixtures_from(var: Result<String, std::env::VarError>) -> bool {
60+
match var {
61+
Err(std::env::VarError::NotPresent) => false,
62+
Err(std::env::VarError::NotUnicode(v)) => panic!(
63+
"IFC_LITE_REQUIRE_FIXTURES={v:?} is not recognised (use \"1\" or \"0\") — \
64+
refusing to guess, because guessing \"off\" would silently disable the gate"
65+
),
66+
Ok(v) if v.is_empty() || v == "0" => false,
67+
Ok(v) if v == "1" => true,
68+
Ok(v) => panic!(
69+
"IFC_LITE_REQUIRE_FIXTURES={v:?} is not recognised (use \"1\" or \"0\") — \
70+
refusing to guess, because guessing \"off\" would silently disable the gate"
71+
),
72+
}
73+
}
1774

1875
/// Bytes of the catalogued fixture at `rel` (relative to `tests/models/`), or
1976
/// `None` when it has not been fetched.
@@ -23,11 +80,21 @@
2380
/// fixture setup, not an unfetched fixture, and it panics: treating those as
2481
/// absence would let a whole crate's tests skip while CI reported green, which
2582
/// is the exact failure mode this module exists to remove.
83+
///
84+
/// When `IFC_LITE_REQUIRE_FIXTURES=1` is set, a missing fixture panics too --
85+
/// see the module doc. Unset, empty, or `"0"` keeps the skip. Any other value
86+
/// (e.g. `true`, `yes`, a typo) is itself a hard error -- see
87+
/// [`require_fixtures`].
2688
pub(crate) fn fixture_opt(rel: &str) -> Option<Vec<u8>> {
2789
let path = format!("{}/../../tests/models/{}", env!("CARGO_MANIFEST_DIR"), rel);
2890
match std::fs::read(&path) {
2991
Ok(bytes) => Some(bytes),
3092
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
93+
if require_fixtures() {
94+
panic!(
95+
"fixture {rel} not present and IFC_LITE_REQUIRE_FIXTURES=1 — run `pnpm fixtures` to download (sha256 in tests/models/manifest.json)"
96+
);
97+
}
3198
eprintln!(
3299
"skipping: fixture {rel} not present — run `pnpm fixtures` to download (sha256 in tests/models/manifest.json)"
33100
);
@@ -50,3 +117,32 @@ macro_rules! fixture_or_skip {
50117
}
51118
};
52119
}
120+
121+
#[cfg(test)]
122+
mod tests {
123+
use super::require_fixtures_from;
124+
use std::env::VarError;
125+
use std::ffi::OsString;
126+
127+
/// A present-but-not-Unicode value must fail closed the same way an
128+
/// unrecognised string does (panic), not fall through to "off" like an
129+
/// unset variable would. Before this fix, `require_fixtures` matched
130+
/// `Err(_) => false`, which treated `VarError::NotUnicode` the same as
131+
/// `NotPresent` and silently disabled the gate for a misconfigured value
132+
/// instead of panicking.
133+
#[test]
134+
fn not_unicode_is_rejected_not_treated_as_absent() {
135+
let result = std::panic::catch_unwind(|| {
136+
require_fixtures_from(Err(VarError::NotUnicode(OsString::from("\u{FFFD}"))))
137+
});
138+
assert!(
139+
result.is_err(),
140+
"non-Unicode value must panic, not silently disable the gate"
141+
);
142+
}
143+
144+
#[test]
145+
fn not_present_is_off() {
146+
assert!(!require_fixtures_from(Err(VarError::NotPresent)));
147+
}
148+
}

0 commit comments

Comments
 (0)