Repository navigation
Behavioral gossip conformance testing, CI/coverage tooling, and ~94% test coverage - #2
Merged
Merged
Conversation
Message well-formedness alone (JSON parses, stays within size bounds) never actually tested whether a peer runs BINDA's protocol logic — only that it could produce valid-looking JSON. This wasn't the original design intent: "peers don't trust or distrust anything, they test each other for correct behaviours ... and if the behaviour isn't a binda's correct behaviour, they ignore the exchange." Adds gossip::ConformanceChallenge: every Request now bundles two synthetic tokens and a win condition, whose one correct answer is whatever crate::collision::resolve (the same deterministic logic a real collision uses) computes. A Rumors reply must answer that challenge correctly or its rumors are dropped entirely, regardless of how plausible they look — same treatment as a malformed message, and just as memoryless (a later exchange from the same address is judged on its own merits). node.rs tracks outstanding challenges per peer address in a bounded, periodically-pruned map (mirroring the rate limiters' own eviction pattern) so a flood of spoofed Digest messages can't grow it without bound. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds .github/workflows/ci.yml (fmt check, clippy, build, test, doc) and .github/workflows/coverage.yml (cargo-llvm-cov -> Codecov, needs a CODECOV_TOKEN repo secret to actually upload). Runs cargo fmt across the workspace so the new fmt-check CI job starts green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds substantial coverage across domain, client_api, collision, token, zone, wire, gossip, api, and store: error paths, boundary conditions, Display/TryFrom/Ord impls, and previously-untested branches (adopt_rumor's no-op and collision arms, set_records failure paths, oversized Request/Rumors rejection, malformed key/signature lengths, etc). Two real bugs surfaced and fixed along the way: - liveness::LivenessTracker::forget_client_by_key (called from store::reclaim_stale): a client that went stale and had its domains reclaimed kept its old registration count forever, permanently capping it below MAX_REGISTRATIONS_PER_CLIENT even after every one of its domains was freed and it came back live. Regression test: store::tests::client_can_reach_the_cap_again_after_going_stale_and_coming_back. - dns::build_response: a record whose RDATA failed to encode (e.g. an A record with a value that isn't a valid IP) still had its name/type/class/ttl bytes written before the failure, leaving a truncated answer in the message while ANCOUNT still counted it as present -- a corrupt packet, not just a miscount. Now encodes each answer into a scratch buffer first and only appends (and counts) ones that fully succeed. Regression tests: dns::tests::skips_record_with_unencodable_rdata_without_corrupting_the_message and ::skipped_record_does_not_corrupt_subsequent_valid_answers. binda-core: 115 tests passing (up from 53). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
reverse_lookup, forward_lookup, and is_forward_confirmed previously had no coverage of their actual network code path (query, send_and_receive, parse_response) since they talk to the real public DNS system by default. Adds an in-process fake resolver (a background thread on a local UDP socket that echoes the question and answers with a compression-pointer-based PTR or A record, exactly like a real resolver's reply) so these can be exercised deterministically without real internet access. Covers the full happy path plus both FCrDNS-mismatch failure modes (PTR doesn't name the claim, forward lookup disagrees) and an unreachable-resolver error path. fcrdns.rs coverage: 48% -> 93%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
query_offset_millis (and the sync loop that calls it) had no test coverage of its actual network round-trip, since NtpTimeSource talks to real public NTP servers by default. Adds an in-process fake SNTP server (answers the one 48-byte request it receives with a crafted Receive/Transmit timestamp claiming a chosen clock offset) to exercise the real send/receive/offset-calculation path deterministically, including a negative offset and the every-server-unreachable fallback, without needing real internet access. ntp.rs coverage: 40% -> ~90%+. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds unit tests for peers::PeerBook (learn/dedup/snapshot) and main::parse_args (extracted into parse_args_from, which takes an argument iterator instead of reading std::env::args() directly, so it's testable without spawning a process), and integration tests in node.rs covering the gossip conformance challenge end to end (two real Node instances converging via UDP; a rogue peer answering the challenge incorrectly and having its rumor rejected; one answering correctly and being adopted), the client API's probe/register-refusal paths over a real socket, and the resolver/native-DNS loops. Found and fixed a real gap along the way: gossip_push_loop skipped sending a Digest entirely when a node had no rumors of its own, which meant a freshly-joined, one-directionally-configured `--peer` could never actually receive a bootstrap peer's existing state -- the peer never learns the newcomer's address (since the newcomer never sends anything), so it never has anywhere new to push its own data. Now pushes even an empty digest, purely so the recipient learns the sender's address via the existing peers.learn call. binda binary crate: 0 tests -> 20 passing. Workspace total: 147 tests. Overall coverage (cargo llvm-cov): 60% -> 94%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds badges for the CI workflow, Codecov coverage, MIT license, and the nightly toolchain requirement. Documents how to run coverage locally with cargo-llvm-cov and notes that the Codecov badge needs a CODECOV_TOKEN repository secret to actually upload. Updates the Status section with the current ~94% coverage figure and narrows the "still missing" list to what's actually still uncovered (main()'s own async orchestration, and the sliver of ntp/fcrdns that only exercises against a real uncontrolled remote server rather than the local fake servers the test suite substitutes). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Sep 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #1, on the same branch (the earlier PR merged mid-session; these are the commits added afterward).
gossip.rs,node.rs): replaced "message parses and stays under a size cap" with an actual behavioral test. EveryGossipMessage::Requestnow bundles aConformanceChallenge(two synthetic tokens + a win condition) whose one correct answer requires actually running BINDA's own deterministic collision-resolution logic. ARumorsreply with a missing or wrong answer is dropped entirely, exactly like a malformed message — no persistent trust/distrust, judged fresh each exchange. This matches the original design intent ("peers test each other for correct behaviours ... if the behaviour isn't a binda's correct behaviour, they ignore the exchange") more literally than the previous well-formedness-only check..github/workflows/ci.yml(fmt, clippy, build, test, doc) and.github/workflows/coverage.yml(cargo-llvm-cov→ Codecov). README gets CI/coverage/license/toolchain badges and a coverage section (the Codecov badge needs aCODECOV_TOKENrepo secret added to actually upload).cargo-llvm-cov), workspace-wide test count 53 → 147. Notably includes:fcrdns.rsandntp.rs's real network code paths are exercised deterministically without needing internet access in CI.node.rsintegration tests: two live nodes converging via UDP gossip, a rogue peer answering the conformance challenge incorrectly and getting rejected, one answering correctly and getting adopted, plus the client API and resolver/native-DNS loops over real sockets.main.rs's CLI parsing extracted into a testableparse_args_fromandpeers.rs'sPeerBookfully covered.Bugs found and fixed while writing these tests
liveness.rs: a client that went stale and had its domains reclaimed kept its old registration count forever, permanently capping it at the limit even after every domain was freed and it came back live.dns.rs: a record whose RDATA failed to encode (e.g. an A record with a non-IP value) left a truncated answer in the response buffer whileANCOUNTstill counted it as present — a corrupt packet, not just a miscount.node.rs:gossip_push_loopskipped sending anything when a node had no data to announce, which meant a freshly-joined, one-directionally-configured--peercould never actually bootstrap from an existing node (the existing node never learns the newcomer's address, since the newcomer never sends anything).Notes for reviewers
register_via_client_api_is_refused_without_real_fcrdnsand the FCrDNS fake-server tests perform real or near-real network I/O (bounded, with generous timeouts); they should be deterministic but are the most environment-sensitive tests in the suite.main()'s own async orchestration (thin glue exercised by every manual run, not under a harness) and the sliver ofntp/fcrdnsthat only triggers against a real uncontrolled remote server's exact reply timing.🤖 Generated with Claude Code