Guard against integration tests that silently never run - #1756
Conversation
fluree-db-api sets `autotests = false` and declares its 60 test targets explicitly, so a tests/*.rs becomes a test binary only if it is named in a [[test]] block or pulled into a harness via #[path]. A file that is neither is never compiled and never run, and `cargo test` still reports success — from Cargo's point of view there is nothing to build. With ~260 case files that is easy to do by accident and gives no signal when it happens. tests/harness_coverage.rs closes the gap. It parses the [[test]] blocks out of Cargo.toml, collects the #[path] lines from the files those targets name, and fails listing anything under tests/ that neither set covers. Reachability is derived from the manifest outwards rather than from file names, so an undeclared tests/grp_foo.rs is reported as an orphan itself instead of being assumed to be a target, and the files it references are not credited as covered on its say-so. It finds no orphans today. A [[test]] target may omit `path`, in which case Cargo infers tests/<name>.rs. This crate declares it_absent_subject_scan_narrowing and it_values_object_bounds that way, so a plain `path = "tests/…"` substring check would report both as false orphans. A unit test over a synthetic manifest pins that form alongside the explicit one, and pins that other target kinds are ignored. The guard is wired into grp_misc so it is subject to the invariant it enforces. fluree-db-server carries an identical copy; the module docs on each note that a fix to one belongs in the other. Sharing a single implementation would mean a dev-dependency crate existing only to hold one test, since integration tests cannot reach a crate's regular dependencies.
whole_graph_scan_limit() reads FLUREE_MAX_GRAPH_SCAN_FLAKES from the process
environment on every scan and has no programmatic override, so the backstop test
must set that variable to exercise the cap. Under bare `cargo test` a binary
runs its tests as threads in one process, so the cap applied between the set_var
and the remove_var leaked into whichever graph-sync tests happened to be running
alongside. delta_sync_commits_only_the_delta, identical_resync_is_a_noop,
sync_does_not_touch_other_graphs and policy_gated_sync_targets_the_named_graph
all failed with WholeGraphScanTooLarge { limit: 2 } depending on scheduling.
Move that one test into tests/it_sync_graph_scan_backstop.rs with its own
[[test]] target; the remaining graph-sync tests stay in grp_ledger. Keeping all
the env mutation inside a single test fn is not enough on its own. That stops
the mutators racing each other, not the mutation reaching siblings in the same
binary.
nextest gives every test its own process, which is why CI has always been green
and why this only reproduces with `cargo test -p fluree-db-api --test grp_ledger`.
docs/contributing/tests.md requires that a test mutating process-global env gets its own [[test]] binary: a grouped binary runs its tests as threads in one process under bare `cargo test`, so the mutation reaches every sibling running at the time. Nothing enforced that, and nextest, which CI uses, gives every test its own process, so a violation is invisible until someone runs cargo test. harness_coverage.rs in both crates now fails, naming the files, when anything compiled into a shared harness calls set_var or remove_var. A target counts as a harness only when it pulls in a sibling top-level case file; standalone targets reach shared helpers by #[path] as well, and they are exempt because being alone in a process is what makes the mutation safe. Matching is on the call shape at a word boundary, so `unset_var(` does not count, and the needles are assembled with concat! so the guard does not flag itself.
fluree-db-api and fluree-db-server carried byte-identical copies of the
reachability and env-isolation guards, roughly 250 lines each, kept in step by
a note in the module docs and nothing else. Any fix to one silently belonged in
the other.
Move the logic into fluree-test-support, a dev-dependency alongside the existing
fluree-bench-* support crates and likewise never a release artifact. Each
crate's tests/harness_coverage.rs becomes two calls passing
env!("CARGO_MANIFEST_DIR"), which expands at the call site so the crate under
test is the one checked. Adopting the guards in a third crate is now a
dev-dependency and sixteen lines rather than a copy.
The manifest parser's test moves with it and becomes a plain unit test. It
exercises a parser against a synthetic string and has nothing to do with either
crate's layout. A second unit test pins the env matcher: `std::env::set_var(`
and a bare `set_var(` match, `unset_var(` and `my_remove_var(` do not, and prose
naming the function without calling it does not either.
Remove the env-mutation check. It scanned Rust source for set_var and remove_var call shapes, which produced both false positives and false negatives. A doc comment describing why a test avoids env mutation failed the build, with no suppression mechanism. Helper modules reached transitively were never scanned, including support/span_capture.rs, which is compiled into all twelve api harnesses. The harness/standalone distinction was inferred from whether a `#[path]` member contained a slash, which misclassified targets in both directions. The rule remains documented in docs/contributing/tests.md. Parse the manifest with the toml crate instead of scanning lines. The previous version dropped a [[test]] block when the header carried a trailing comment or a value used single quotes, which caused declared files to be reported as orphans. toml 0.8 is already in the lockfile and used by three other crates in this workspace. Declare harness_coverage as its own [[test]] target instead of a harness member. As a member it was reachable only through a single #[path] line; removing that line would stop it compiling without failing any test.
bplatz
left a comment
bplatz
left a comment
There was a problem hiding this comment.
Approving. The guard passes in both crates, its unit tests pass, and I confirmed the two properties the design leans on by running it against synthetic crate trees: an undeclared harness plus its member are both reported (laundering resistance works), and a #[path] inside a //! or // comment does not falsely credit coverage. Only these two crates use autotests = false and both now carry it. Cargo.lock adds no new external dependency, clippy and fmt are clean, the new standalone target passes, and grp_ledger is 159/159. Dropping 145 lines of duplication out of the server file is a nice side effect.
A few things to look at before merging — the first is the only one I'd actually ask for. The rest are nits or follow-up material.
(Scope of what I ran: harness_coverage in both crates, fluree-test-support, it_sync_graph_scan_backstop, grp_ledger, plus the stress runs described inline. Not the full api battery.)
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
@zonotope -- since I started this review (but did not submit) yesterday, @bplatz has also reviewed now. I'm going to just submit this as a comment review so any of the Claude findings below (which I'll leave verbatim) can be available as additional/corroborating feedback to what Brian himself left.
This closes the sharpest instance of a trap class this repo has genuinely been bitten by: under autotests = false, a tests/*.rs that nobody declares and nobody #[path]-includes is never compiled, never run, and cargo test still exits 0 — with 260 case files behind ~60 targets in fluree-db-api alone, and until now a guard only in fluree-db-server, as a hand-rolled copy.
I verified this the constructive way rather than by reading. Every failure mode in your verification table reproduced red against the real trees: an orphan file, an undeclared harness plus the member it pulls in (both named — the laundering defense works), a deleted [[test]] block, and a deleted #[path] member line; clean tree green on both crates, which also proves no false positives on the inferred-path and required-features-gated declarations. One mutation was additionally driven through the real compiled harness_coverage binary under nextest to confirm the wiring end-to-end. Both guard targets have no required-features, so CI's cargo nextest run --workspace --all-features builds and runs them — the guard is not itself an instance of the disease it treats.
The bug the guard-building surfaced is real and the fix holds: with the BASE it_sync_graph.rs restored, cargo test -p fluree-db-api --test grp_ledger -- --test-threads=32 fails with WholeGraphScanTooLarge exactly as described (scheduling-dependent — default thread counts stayed green here, 32 threads hit all four tests you named); at HEAD the same command passes repeatedly. I also traced whole_graph_scan_limit() to confirm the per-scan env read with no programmatic override, so the standalone binary is the right fix, and your observation that moving the whole file out would not have sufficed is correct and worth having in the record.
Design calls are the ones I'd have made: guard as its own [[test]] target (the harness-member version could be disabled by deleting one line — the exact orphan it hunts); a real TOML parser over line-scanning; the shared crate on the fluree-bench-support pattern, turning server's 160-line copy into 15; and — especially — the final commit's decision to remove the env-mutation scanner after honestly cataloguing its false positives, keeping that rule as documented convention. The scope line in the PR body and docs/contributing/tests.md is drawn accurately: reachability is mechanically enforced, the other two standalone rules are review's job, and workspace-excluded suites remain a separate, still-open member of the trap class this PR never claimed to close.
Two optional doc-level notes inline; neither needs to hold this up.
Adherence checklist:
- Patterns / abstractions ✔ — extends the existing support-crate pattern (
fluree-bench-supportprecedent) and removes a duplicated hand-rolled implementation; no orthogonal construct, no new external dependency (toml 0.8already in the lockfile). - Performance ✔ — neutral; no engine code touched, dev-only crate excluded from dist, two extra tiny link steps and a milliseconds-scale guard.
- Testing ✔ — the guard itself runs under both bare
cargo testand CI's--all-featuresnextest in both crates; its inference is unit-tested; all claimed failure modes verified by construction; the relocated backstop test runs by name; W3C suite green on this head. - Conventions ✔ — fmt/clippy green on this exact head against the workspace's denied-lint set; commit bodies exemplary;
docs/contributing/tests.mdupdated in step with the mechanism, including the adoption recipe for a third crate.
| criterion = { version = "0.5", features = ["async_tokio"] } | ||
| fluree-bench-support = { path = "../fluree-bench-support" } | ||
| fluree-test-support = { path = "../fluree-test-support" } | ||
| fluree-bench-alloc = { path = "../fluree-bench-alloc" } |
There was a problem hiding this comment.
Optional, on record only — no change requested. The guard's own [[test]] block is now the one thing nothing watches: delete the block while the file stays and the guard becomes an unreported orphan. That's the irreducible self-reference limit, and the own-target design still strictly beats the harness-member design it replaces (a manifest-block deletion is far more review-visible than one #[path] line among fifty). A workspace-level meta-check for two crates would be over-engineering, and the inline comments you added at both declaration sites are exactly the right mitigation — flagging it only so the residual is on record.
The walk credited only `#[path = "..."]` lines, spaced exactly that way, in files named by a [[test]] target. Three shapes that rustc compiles and runs were reported as orphans: a plain `mod it_x;` in a declared harness, a member reached through a second `#[path]` hop, and `#[path="x.rs"]` without spaces. All were false positives, so nothing went unrun, but each sent the reader looking for a problem that did not exist. Walk from the declared targets through every file reached, resolving both forms the way rustc does: `#[path]` relative to the containing file's directory, plain `mod x;` beside a crate root or mod.rs and below any other file. Nothing is read unless it was reached from the manifest, so an undeclared harness still cannot launder its members into the covered set. Exercise the guard against throwaway crate trees so the accepted spellings, transitive membership, the laundering defence and `#[path]` inside comments are pinned rather than verified by hand.
A crate adopting the guard before it has any integration tests panicked with `read tests/: NotFound`. Treat an absent directory as nothing to reach; any other read error still fails, naming the directory. Declared `[[test]]` paths were compared as written, so `./tests/x.rs` was a false orphan. Run them through the same normaliser the module walk uses. Next: item 6, the table-driven manifest test covering header comments, single-quoted values, inline tables, non-canonical spacing, and required-features after path not spilling into the next block.
The move from line scanning to the toml crate was justified by header comments, single-quoted values, inline tables and non-canonical spacing, none of which had a test, and the port dropped the one case the old server guard did pin: `required-features` after `path` not spilling the target into the next block. One table-driven test covers the set.
3 participants
fluree-db-apiandfluree-db-serverboth setautotests = falseand declare their[[test]]targets explicitly, grouping their case files into a handful of binaries so each crate links a few test binaries instead of one per file. That saves a great deal of build time and costs one invariant that nothing checks.This adds a guard for that invariant, shares it between the two crates through a small
fluree-test-supportcrate, and fixes a live test-isolation bug found while building it.The invariant
With auto-discovery off, a
tests/*.rsbecomes a test binary only if it is named in a[[test]]block or pulled into a harness via#[path]. A file that is neither is never compiled and never run — andcargo teststill reports success, because from Cargo's point of view there is nothing to build. With roughly 260 case files behind 60 declared targets influree-db-api, that is an easy mistake to make and gives no signal when it happens.assert_every_test_file_is_reachablederives reachability fromCargo.tomloutwards, never from file names: it reads the[[test]]blocks, collects the#[path]members of the files those targets name, and fails listing anything undertests/that neither set covers. An undeclaredtests/grp_foo.rsis therefore reported as an orphan itself rather than assumed to be a target, and the files it references are not credited as covered on its say-so.The guard is declared as its own
[[test]]target in each crate rather than pulled into a harness. As a harness member, deleting the single#[path]line that wires it in would stop it compiling — making it an orphan of exactly the kind it exists to catch.The bug this found
it_sync_graph'swhole_graph_scan_backstop_fails_loud_before_materializingsetsFLUREE_MAX_GRAPH_SCAN_FLAKES=2to exercise the whole-graph memory cap.whole_graph_scan_limit()(fluree-db-transact/src/stage.rs) reads that variable from the process environment on every scan and has no programmatic override, so setting it is the only way to test the cap.Grouped into
grp_ledger, the cap reached whatever else was running between theset_varand theremove_var.delta_sync_commits_only_the_delta,identical_resync_is_a_noop,sync_does_not_touch_other_graphsandpolicy_gated_sync_targets_the_named_grapheach failed withWholeGraphScanTooLarge { limit: 2 }depending on scheduling. It reproduces onmainwithcargo test -p fluree-db-api --test grp_ledger, and has never been visible to CI, which runs nextest — every test gets its own process there, so the leak cannot occur.Worth knowing for anyone hitting this pattern again: moving the whole file to its own binary is not sufficient. The leak runs from one test to its fourteen siblings, so the file still failed against itself. Only the mutating test moves out, into
tests/it_sync_graph_scan_backstop.rs; the rest stay ingrp_ledger.Deliberately not enforced
docs/contributing/tests.mdlists three categories of test that must keep their own binary: feature-gated suites, tests that mutate process-global env, and tests whose assertions are about instrumentation. Only reachability is mechanically enforced here. The other two are conventions checked by review.That is a deliberate limit rather than an omission. Detecting them means scanning Rust source for call shapes, and a prototype of that check produced both false positives (a doc comment explaining why a test avoids
set_varfailed the build, with no way to suppress it) and false negatives (helper modules reached transitively were never scanned; aliased or reformatted calls slipped through). It also could not distinguish a standalone target that shares a helper from a genuine harness. The cost in blind spots and unsuppressable failures exceeded what review already provides, so the rule stays documented rather than automated.Verification
tests/*.rscargo test -p fluree-db-server --all-featurescargo test -p fluree-db-api --all-features --no-fail-fastcargo test -p fluree-test-supportcargo fmt --check,cargo clippy --all-features --all-targets -- -D warningscargo check --workspace --all-targetsThe api failures are
it_iceberg_direct,it_storage_s3_testcontainersandit_vended_credentials_testcontainers, allSocketNotFoundError("/var/run/docker.sock"). They are environmental, unchanged frommain, and unrelated to these changes. Everything outside those three suites passes.Failure modes are verified by construction rather than by inspection: the guard is run against a real orphan wired into a real crate, and confirmed to pass again once removed.
Implementation notes
declared_test_pathsuses thetomlcrate rather than scanning lines, so manifest shapes such as comments on a[[test]]header, single-quoted values, inline tables and non-canonical key spacing are handled by a real parser.toml 0.8is already in the lockfile and already used byfluree-db-cli,fluree-db-serverandfluree-bench-support, so this adds no new external dependency.fluree-test-supportsits alongside the existingfluree-bench-*support crates and is likewisedist = false— internal tooling, never a release artifact. It carries unit tests for the one piece of inference it adds on top of TOML: that a[[test]]withoutpathmeanstests/<name>.rs, and that other target kinds are not test targets.fluree-db-serveralready carried a copy of this guard. Moving the logic into the shared crate reduces that file from 160 lines to 15, so this PR removes an existing duplication rather than creating one.Adopting elsewhere
A dev-dependency on
fluree-test-support, plus atests/harness_coverage.rscallingassert_every_test_file_is_reachable(env!("CARGO_MANIFEST_DIR")), declared as its own[[test]]target.env!expands at the call site, so each crate is checked against its own manifest andtests/directory. Only these two crates useautotests = falsetoday, so no third crate is silently exposed.