From 4886832ca6e76bae5c3bebb328aa6846c2d6fe0c Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Wed, 19 Aug 2026 16:01:06 -0400 Subject: [PATCH] test(unit-rules): fuzz the round trip, and stop testing against mutable files Two thousand generated documents, read, written and read again. Every loss so far was found by looking somewhere new and six times the answer was that nobody had looked there yet; this looks where nobody chose. It found one in its first document: an entity inside another was hoisted out and re-emitted as a sibling, because every entity was taken for a unit whatever held it. Only the document's own are, which is what MULParser reads. The corpus no longer walks MegaMek's logs/ directory. That is its scratch output, rewritten by anything that starts MegaMek - a run of the bridge here rewrote it mid-session and took the only ejected crew in the corpus with it. Change-Id: I9be724a83dc4bc9ebde21332e51f59153dffaa4e --- crates/helm-bv/tests/conformance.rs | 8 ++ crates/helm-unitfile/src/mul.rs | 167 +++++++++++++++++++++++++++- 2 files changed, 174 insertions(+), 1 deletion(-) diff --git a/crates/helm-bv/tests/conformance.rs b/crates/helm-bv/tests/conformance.rs index 748cacf..67bbcfc 100644 --- a/crates/helm-bv/tests/conformance.rs +++ b/crates/helm-bv/tests/conformance.rs @@ -489,6 +489,14 @@ fn collect_muls(dir: &std::path::Path, out: &mut Vec) { }; for entry in entries.flatten() { let path = entry.path(); + // `logs/` is MegaMek's own scratch, not data it ships: anything that + // starts MegaMek rewrites it. A corpus that includes it changes under + // the test, and a case it once covered can disappear without anyone + // touching the repository - which is how the only ejected crew in the + // corpus went missing. + if path.file_name().is_some_and(|n| n == "logs") { + continue; + } if path.is_dir() { collect_muls(&path, out); } else if path diff --git a/crates/helm-unitfile/src/mul.rs b/crates/helm-unitfile/src/mul.rs index a2ee5b7..d40f668 100644 --- a/crates/helm-unitfile/src/mul.rs +++ b/crates/helm-unitfile/src/mul.rs @@ -335,12 +335,17 @@ fn between(open: &mut [Kept], name: &str, text: String, prologue: &mut Vec /// Attach a finished element to whatever contains it - or, if it is an entity, /// read it into the model. fn close(open: &mut [Kept], done: Kept, mul: &mut Mul) { + // Only an entity the document holds directly is a unit, which is what + // MULParser reads. One nested inside another is not a `.mul` shape at all, + // and treating it as a unit hoists it out of whatever held it - so it is + // kept where it was found and left alone. + let is_unit = done.name == "entity" && open.len() <= 1; // An entity is read into the model, and the document keeps an empty one in // its place. The position is what the document needs - a `.mul` can carry // `` and a crew that got out beside its units, and writing them // back in the wrong order is still a change to somebody's file - while the // entity itself is held once, by the unit that speaks for it. - let done = if done.name == "entity" { + let done = if is_unit { let name = done.name.clone(); mul.units.push(read_entity(done)); Kept { @@ -1322,6 +1327,166 @@ mod tests { assert!(changed[0].1.contains("camoCategory=Clans/Ghost Bear")); } + /// A tiny deterministic generator, so a failure can be rerun. + /// + /// Hand-rolled rather than a property-testing crate: what is wanted here + /// is a few thousand awkward documents, not a shrinking framework, and + /// this crate's parsers carry no dependencies for the same reason. + struct Rng(u64); + + impl Rng { + fn next(&mut self) -> u64 { + // xorshift64*, which is enough randomness to build XML with. + self.0 ^= self.0 >> 12; + self.0 ^= self.0 << 25; + self.0 ^= self.0 >> 27; + self.0.wrapping_mul(0x2545_f491_4f6c_dd1d) + } + + fn pick<'a, T>(&mut self, from: &'a [T]) -> &'a T { + &from[(self.next() % from.len() as u64) as usize] + } + + fn upto(&mut self, n: u64) -> u64 { + self.next() % n + } + } + + /// Build a document this crate has never seen, out of the pieces a `.mul` + /// is made of and the ones it is not. + /// + /// The names and values are deliberately a mixture: the ones this crate + /// acts on, so the edit paths are exercised, and ones it has never heard + /// of, so the keeping paths are. The values include the ones that have + /// caught it out before - a word where a number was expected, an + /// ampersand, an empty string. + fn awkward_document(seed: u64) -> String { + const ELEMENTS: [&str; 10] = [ + "entity", + "pilot", + "location", + "armor", + "slot", + "force", + "quirks", + "bombs", + "escapedCrew", + "somethingNew", + ]; + const NAMES: [&str; 10] = [ + "chassis", + "model", + "type", + "index", + "points", + "shots", + "isDestroyed", + "gunnery", + "camoCategory", + "aFieldFromTheFuture", + ]; + const VALUES: [&str; 10] = [ + "Destroyed", + "N/A", + "0", + "-1", + "99999", + "", + "Loki (Hellbringer)", + "Bob & Sons", + "a \"quoted\" thing", + "", + ]; + let mut rng = Rng(seed | 1); + let mut out = String::from("\n"); + if rng.upto(2) == 0 { + out.push_str("\n"); + } + out.push_str("\n"); + + for _ in 0..rng.upto(4) + 1 { + let mut depth = 0; + for _ in 0..rng.upto(8) + 1 { + match rng.upto(6) { + 0 if depth > 0 => { + depth -= 1; + out.push_str("\n"); + // Only entities are opened as containers below, so the + // close is always theirs. + } + 1 => out.push_str("\n"), + 2 => out.push_str("some text\n"), + 3 => { + out.push_str("\n"); + depth += 1; + } + _ => { + out.push('<'); + out.push_str(rng.pick(&ELEMENTS)); + attributes(&mut rng, &NAMES, &VALUES, &mut out); + out.push_str("/>\n"); + } + } + } + for _ in 0..depth { + out.push_str("\n"); + } + } + out.push_str("\n"); + out + } + + /// A few attributes, each named once: a repeated name is not XML, and a + /// parser is right to refuse it rather than pick one. + fn attributes(rng: &mut Rng, names: &[&str], values: &[&str], out: &mut String) { + let mut used: Vec<&str> = Vec::new(); + for _ in 0..rng.upto(4) { + let name = *rng.pick(names); + if used.contains(&name) { + continue; + } + used.push(name); + out.push_str(&format!(" {name}=\"{}\"", escaped(rng.pick(values)))); + } + } + + /// Attribute values have to be legal XML before they can be parsed. + fn escaped(value: &str) -> String { + let mut out = String::new(); + for c in value.chars() { + match c { + '&' => out.push_str("&"), + '<' => out.push_str("<"), + '>' => out.push_str(">"), + '"' => out.push_str("""), + _ => out.push(c), + } + } + out + } + + /// Two thousand documents this crate has never seen, each read, written + /// and read again. + /// + /// Every loss found by hand so far was found by looking somewhere new - + /// attributes, then elements beside the units, then the version, then the + /// order of locations, then comments, then the notice above the document. + /// Six times the answer was that nobody had looked there yet. This looks + /// in places nobody chose. + #[test] + fn documents_nobody_wrote_survive_a_round_trip() { + for seed in 1..=2000 { + let xml = awkward_document(seed); + if let Err(why) = survives_structurally(&xml) { + panic!("seed {seed} lost something:{why}\n\n{xml}"); + } + } + } + // `type` must not match `crewType`, and `armor` must not match // `armorDivisor`: an attribute name only counts at a word boundary. #[test] -- 2.51.2