ci: run chat-delegate and web-container tests, and guard the gap class - #615
Conversation
The chat-delegate's 40 unit tests never ran in CI. Makefile.toml defines test-chat-delegate and rolls it into `cargo make test`, but no workflow invoked it, so the tests gated only a developer's local run. Same for web-container-contract + web-container-tool (9 tests). Adds a cargo test step for each, plus scripts/check-ci-test-coverage.sh, which fails CI when any [workspace] member has no `cargo test -p <name>` step in build.yml. That converts a recurring per-crate oversight into a CI failure. Closes #614 Refs #612, freenet/freenet-core#2776 [AI-assisted - Claude]
… the real migration gate Three review fixes: 1. The guard's header cited five prior incidents as the class it fixes, but two of them (river-core's bulk lib tests, the nine common/tests allowlist files) happened while river-core DID have -p test steps — a configuration the guard passes. It checks presence, not adequacy. Says so now, since a guard overclaiming its coverage is the failure mode this PR is about. 2. `\s` is GNU-only, so the script failed spuriously on macOS/BSD. Uses [[:space:]] throughout. 3. The build.yml caveat said what the delegate tests don't cover but not where that coverage lives. Now points at the #613 pin in `cargo test -p river-ui --bins`, and cites handlers.rs:191-198 (191 is the cfg arm; 190 was the prose comment). Also parses `members = [...]` by accumulating to the closing bracket, so a single-line reformat is handled instead of running to EOF and reporting "member '2' has no Cargo.toml" (it had picked up `resolver = "2"`). [AI-assisted - Claude]
|
Review fixes pushed in 1 (SHOULD-FIX) — the guard's header overclaimed. Fair catch, and the
2 — GNU-only 3 — where the coverage actually lives. The build.yml caveat now names the
It also records the measurement rather than just the conclusion (deleting Both optionals, taken. The cfg citation is now Re-verified after the changes — the four guard mutations you ran, against
The fourth now passes rather than failing closed, which is the intended One process note worth recording: two of my [AI-assisted - Claude] |
Problem
The
chat-delegatecrate has 40 unit tests that have never executed in CI(the issue estimated ~39; the exact count is 40 — 13 in
handlers.rs, 15 insubscription/tests.rs, 12 inversioning.rs).Makefile.tomldefinestest-chat-delegateand rolls it intocargo make test, but no workflow ever invoked it.build.ymlhas explicit steps forroom-contract,river-core,riverctlandriver-ui; the delegate wassimply never added. The only workflows mentioning
chat-delegatearecheck-cli-wasm.ymlandcheck-delegate-migration.yml, and neither runscargo test— the latter only does a WASM byte-hash comparison.A test that no CI job runs is indistinguishable from a test that passes. That
is bad anywhere, and worse here: this is the crate where River's secret storage
and its delegate-re-key migration path live, and the delegate is re-keyed
roughly weekly (#612). A silent regression there destroys user identity.
Found while fixing it, same bug, different crate:
web-container-contract(6 lib + 2 integration tests) and
web-container-tool(1 test) were alsonever run by any workflow. That includes
test_tool_and_contract_compatibility, which pins that the signing tool andthe verifying contract agree on the parameter encoding.
Verified before changing anything, per the issue's request: the premise held in
full. No subset of the delegate's tests was running.
Approach
Three changes to
.github/workflows/build.yml:cargo test -p chat-delegate— a step alongside the existing per-crateones.
cargo test -p web-container-contract -p web-container-tool— the samegap, found while fixing the first.
scripts/check-ci-test-coverage.sh— a guard that fails CI when any[workspace]member has nocargo test -p <name>step inbuild.yml.On the toolchain question the issue raised: no
--target x86_64-unknown-linux-gnupin is needed, unlike theMakefile.tomltask. Thepin there pairs with
--target-dir target/nativeso a developer's local hostbuild cannot clobber the wasm artifacts the UI pulls in via
include_bytes!.CI has no such conflict —
cargo make buildhas already produced the wasm, andevery existing test step likewise builds for the host into the same
CARGO_TARGET_DIR. The crate's default target is wasm only by convention ofhow it is built, not by a
[build] targetsetting, so a bare hostcargo testis correct.
room-contract, also a wasm cdylib, is already wired exactly thisway.
On the guard (point 3), which is the part not strictly asked for. The
per-crate steps in
build.ymlcarry comments recording four previous instancesof this same gap: river-ui's unit tests, riverctl's unit tests, the bulk of
river-core's lib tests, and the nine
common/testsfiles that an allowlist of--test <name>steps silently skipped. This issue is the fifth and sixth. Eachprevious fix added the missing step, which fixes the instance and not the
class. The script reads
[workspace] membersdirectly and resolves eachmember's package name from its own
Cargo.toml, so adding a crate withoutwiring its tests now fails CI. It is plain bash with no
jq/pythondependency. Happy to drop it if reviewers consider it out of scope.
Testing
No test rot: all 40 delegate tests passed unmodified on first run, as did all
9 web-container tests. Nothing was deleted, skipped, or
#[ignore]d — thebefore/after count of passing tests is 40 → 40 and 9 → 9. What changed is that
they now gate merges.
Mutation evidence — the new delegate step can actually fail
Expectation stated before running: inverting one assertion in each of the three
test modules should produce exactly three failures and a non-zero exit, proving
all three modules genuinely execute rather than just one.
Mutations applied (
handlers.rs:832assert_eq!(result.len(), 1)→99;versioning.rs:164assert_eq!(gen, 13)→14;subscription/tests.rs:192assert_eq!(m1, m2)→assert_ne!), then the exact command CI runs:All mutations reverted; the tree is clean against the baseline.
Mutation evidence — the guard can actually fail
Removing the
cargo test -p chat-delegateline frombuild.yml:Unmutated, it reports all 7 members ok and exits 0.
Scope caveat — what these 40 tests do NOT cover
Worth recording so the new green checkmark is not over-read. On native targets
DelegateCtx'sset_secretis a no-op andget_secretalways returnsNone(freenet-stdlib's stub; see the
cfgathandlers.rs:190). So these testscover dispatch and pure-value logic, not storage round-trips.
Measured, rather than assumed: I removed the
set_key_index(...)call fromhandle_store_request— a genuine #612-class regression that would stop everyroom from migrating — and all 40 delegate tests still passed. The
source-scrape pin added by #613 caught it:
Follow-up, deliberately not done here (issue #614 point 6): that pin's own
comment says to move it into the delegate crate "once they are [wired into
CI]". This PR makes that possible. But the evidence above shows it must stay a
source-scrape when it moves — the behavioral version is still not
expressible against a stub ctx that never stores anything. Moving it also needs
care about self-matching: as an in-file pin its needles would appear in its own
assertion strings, so the existing body-slicing helper has to stay. Clean
separate PR.
Closes #614
Refs #612, #613, freenet/freenet-core#2776
[AI-assisted - Claude]