Repository navigation
Stop corpus-gated tests passing without assertions - #2
Conversation
|
Went through this locally: the corpus tests pass on the real sample data and correctly report as ignored without it, and the no-silent-skips guard fires on the old One small thing before I approve, for consistency:
Can you route the legacy var through the same resolver + a |
Tests keyed on a sample corpus that CI never has returned early and still reported success, so the suite looked green while asserting nothing. Move the corpus resolver into a shared support crate that fails loudly when a run declares the data required, mark the corpus-only tests ignored so they read as skipped rather than passed, and add decoder property tests that need no corpus. Enforce it: run under nextest, publish coverage without gating on a number, fail the build if a silent skip reappears, and require a green suite before release. Set the minimum Rust to 1.89, the true floor for the current sources and dependencies.
`optional_corpus_decodes_to_exact_eof` read XAERO_LEGACY_CORPUS with a bare env::var().expect(), so it was the one corpus test that did not go through resolve_corpus. Give resolve_corpus the variable name, add legacy_corpus_root() beside corpus_root(), and point the legacy sweep at it, so required mode names the variable the caller actually reads instead of always naming XAERO_CORPUS. The sweep still stops rather than skipping. It reads a private archive that no CI runner can reach, so nothing can ever declare it required, and a skip there would report a pass while asserting nothing, which is the shape this branch exists to remove. The panic now names the flag that excludes this one test, so running the ignored set against the public corpus does not trip over the private one. Also correct four module docs and the developer setup guide, which all still described the pre-#[ignore] silent-skip behaviour, and ignore lcov.info so the documented coverage command leaves no untracked file behind.
The test read two hardcoded region paths and continued past either one it could not open. With both absent the loop body never ran, so the test reported a pass having asserted nothing. That is the defect this branch exists to remove: the harness closed the missing-variable case and left the missing-file case open. Found by pointing XAERO_CORPUS at an empty directory and running the ignored set. Of the 17 corpus tests, this was the only one that passed. It now names the missing file and stops. The no-silent-skips guard cannot catch this shape, because it looks for a missing environment variable rather than a missing file. Also record that the legacy sweep's test name is quoted in the developer setup guide and in its own panic message, so a rename updates both, and document the nextest form of the ignored-test command alongside the cargo test form.
d8944f4 to
f428e0e
Compare
|
Routed it through the resolver. I left the sweep failing rather than skipping, though. The gate only changes one branch, variable unset and not required, and there the resolver yields nothing, so the test has to return early and report a pass having asserted nothing. That's the shape this branch removes. The gate meant to prevent it can't be turned on either: the archive isn't reachable from a GitHub-hosted runner, so nothing can set Your That and the nextest equivalent are in START-HERE, and the panic message names the flag. I don't have the corpus here, so I've verified the filtering but not a full green run against real data. Also fixed: four module docs and the START-HERE dev section still described the old silent-skip behaviour, and One question. Rebased onto main, so the diff is the harness alone. All 7 workflow jobs green on a scratch PR in my fork. |
|
Two things I hit while in here, both separate from your review.
The coverage summary reads 43.30% because the runner has no corpus and skips 18 tests. |
dekrom
left a comment
dekrom
left a comment
There was a problem hiding this comment.
Verified locally: builds clean, fmt/clippy green, full suite passes against the real sample corpus, and the previously silent corpus tests now fail loudly when the data is absent (confirmed by pointing XAERO_CORPUS at an empty dir). Both the legacy-variable routing and the truncation_never_panics fix are correct. Production changes are test-module only — no runtime behaviour changes. Good to go.
PR #2 landed while this branch was being written, and the two efforts met in the same places: - The NBT recursion cap is the same fix found twice. Upstream's version is kept, along with its nesting tests; the only thing carried over from this side is a case for deeply nested lists, which recurse through a different arm and cost an attacker five bytes a level. - The shared test-support crate replaces the per-file corpus helpers this branch had grown, including its own XAERO_REQUIRE_CORPUS gate. Upstream's <VAR>_REQUIRED convention is more general and is already the contract the other tests use, so every corpus test here now resolves through it. - Three merge-fixture tests and one ingest test still skipped silently when the corpus was absent, which is exactly what PR #2 set out to remove. They fail loudly now like the rest. Kept from this branch: the sparse-cut assertion in the corpus round-trip, which checks that every truncated prefix decodes to a prefix of the whole rather than assuming a cut always reports itself. Verified with the corpus present: round-trip still byte-identical, all seventeen corpus-driven tests pass, fmt, clippy and the wasm check clean.
Depends on #1 (the NBT recursion-cap fix): this branch carries that fix as its first commit, so merge #1 first and this diff reduces to the harness alone.
Corpus-gated tests returned early and still reported success when the sample data was absent, so the suite looked green while asserting nothing. The corpus resolver moves into a shared support crate that fails loudly when a run declares the data required, the corpus-only tests are #[ignore]d so they read as skipped rather than passed, and property tests cover the decoder without a corpus. Enforced in CI: tests run under nextest, coverage is published without a gate, a guard fails the build if a silent skip reappears, and releases require a passing test job. Minimum Rust is set to 1.89.