review round 2: assert the CRDT capability defaults, not their round-trip
Review finding (P2), and it is this lane's own defect class one level
down. Round 1 added a `-p pmacs-protocol --features crdt` step so that
crate compiles both ways. That step EXECUTED
`InstanceCapabilities::default` in both configurations and asserted
NOTHING about it: the crate's only use of that value is a transport
round-trip, and a round-trip is invariant to the values. An all-false
default — or one whose three fields disagreed with each other — encodes,
decodes, and passes identically in both builds.
Running code is not testing it. That is the same sentence this whole
lane is about, and round 1 committed the smaller version of it while
fixing the larger one.
Three tests now pin the defaults, and the split is the point:
* under `crdt`: multi_frontend, crdt_replica and semantic_render all
default true. Advertising false on a CRDT build would strand every
frontend in single-frontend mode.
* without `crdt`: all three default false. Advertising true would be
wire-protocol false advertising — those code paths are
conditionally compiled out.
* FrontendCapabilities::default is all-false in BOTH builds, and this
test is DELIBERATELY NOT feature-gated.
That third one pins an asymmetry nothing else did.
FrontendCapabilities derives Default and is feature-INVARIANT, while
InstanceCapabilities is feature-DEPENDENT. It is load-bearing rather
than an oversight: an instance advertises what it can do, a frontend
OPTS IN through the negotiation handshake, and a v1 frontend has no
local CRDT state regardless of how the crate it links was compiled.
Making the frontend side track the feature would have frontends
claiming support they do not have. A future edit that "makes them
consistent" now fails a test that says why not to.
All three fields are asserted separately rather than by comparing whole
structs, because they track one `cfg!` and a change flipping only some
of them is exactly the regression worth catching.
Bite-verified rather than assumed: mutating `multi_frontend` to a
literal false gives `FAILED. 18 passed; 1 failed` with the expected
assertion message; restoring returns 19/19. Both configurations now
report 19 tests, up from 17, with the correct cfg-gated test running in
each.
Left untested and recorded instead: `InstanceCapabilities::crdt_replica`
carries `#[serde(default = "default_true")]`, a THIRD default mechanism
that is unconditional and therefore disagrees with the `Default` impl in
a non-CRDT build. Exercising it needs a self-describing format and this
crate's only serde dependency is postcard, which is not one. Adding
serde_json as a dev-dependency to test a divergence this lane did not
introduce is scope creep.
Also corrects a note that round 1 made stale: the ledger's "do not
subtract the two jobs' totals" figures (3,485 and 3,747) were measured
BEFORE round 1 added the protocol step, and round 2 adds two tests to
that crate. Expected totals are now 3,487 and 3,766. The root-package
census is untouched at 3,467 / 3,746 — the new tests live in a sibling
crate, which is precisely the region scripts/feature-census cannot see.
Verified: fmt, diff-check, clippy on pmacs-protocol in both feature
configurations, and workspace clippy --features crdt --keep-going.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
6519bc3461
commit
71a1ebd4b7
|
|
@ -172,13 +172,20 @@ in the primary checkout on the laptop, not a worktree.
|
|||
`basedpyright` not skipped as it is locally) plus 1 doc test, minus
|
||||
the 30 ignored: 3,716 + 1 = 3,717. The job demonstrably compiled and
|
||||
ran the crdt corpus rather than reporting green over nothing.
|
||||
- **Do not compare the two jobs' raw totals.** `Test (ubuntu/luajit)`
|
||||
reports 3,485 and `Test (crdt)` 3,747 — a difference of 262, not 279.
|
||||
The jobs run different *sets*: the non-crdt job adds pmacs-protocol's
|
||||
17 tests, and 3,467 + 1 + 17 = 3,485 against 3,746 + 1 = 3,747. The
|
||||
dark count is the all-targets comparison, 3,746 − 3,467 = **279**.
|
||||
A reviewer who subtracts the job totals gets a wrong number that looks
|
||||
plausible.
|
||||
- **Do not compare the two jobs' raw totals.** In the first run
|
||||
`Test (ubuntu/luajit)` reported 3,485 and `Test (crdt)` 3,747 — a
|
||||
difference of 262, not 279, because the jobs ran different *sets*:
|
||||
3,467 + 1 doc + 17 protocol = 3,485, against 3,746 + 1 doc = 3,747.
|
||||
**The dark count is the all-targets comparison, 3,746 − 3,467 = 279.**
|
||||
A reviewer who subtracts job totals gets a wrong number that looks
|
||||
entirely plausible.
|
||||
- **Those two figures are already superseded** and are kept only to
|
||||
explain the trap. Review round 1 added a pmacs-protocol step to
|
||||
`crdt-test`, and round 2 added two capability tests to that crate,
|
||||
so the expected totals are now **3,487** (3,467 + 1 + 19) and
|
||||
**3,766** (3,746 + 1 + 19). The root-package census is untouched at
|
||||
3,467 / 3,746 — the new tests live in a sibling crate, which is
|
||||
exactly the region `scripts/feature-census` cannot see.
|
||||
|
||||
- **Root cause, unchanged:** `.github/workflows/ci.yml` never enabled
|
||||
the `crdt` feature anywhere. Every `#[cfg(feature = "crdt")]` test was
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
# Framing — the CRDT half of the test corpus is dark in CI
|
||||
|
||||
**Revision 4.** Status: **PR #209 open, all 14 checks green on the first
|
||||
**Revision 5.** Status: **PR #209 open, all 14 checks green on the first
|
||||
run.** Branch `ci-crdt-coverage`, based on `githubsucks/main` @
|
||||
`4223dd3` (#208). Approved at revision 2.
|
||||
|
||||
|
|
@ -22,6 +22,38 @@ run.** Branch `ci-crdt-coverage`, based on `githubsucks/main` @
|
|||
feature can matter to a crate a per-test census scores as
|
||||
unaffected**, and the script's header now says so.
|
||||
|
||||
**Revision 4 → 5** closes review round 2, and it is this lane's own
|
||||
defect class one level down.
|
||||
|
||||
- **Running the code was not testing it.** Round 1's
|
||||
`-p pmacs-protocol --features crdt` step *executed*
|
||||
`InstanceCapabilities::default` in both configurations, but the crate's
|
||||
only use of that value was a **transport round-trip** — and a
|
||||
round-trip is invariant to the values. An all-false default, or one
|
||||
whose three fields disagreed, encodes and decodes just as happily and
|
||||
passes in both builds. The step added execution and no assertion.
|
||||
- **Three tests now pin it**, and their split matters: one asserts all
|
||||
three fields `true` under `crdt`, one asserts all three `false`
|
||||
without it, and a third — **deliberately not feature-gated** — asserts
|
||||
`FrontendCapabilities::default` is all-false in *both* builds.
|
||||
- **That third test pins an asymmetry nothing else did.**
|
||||
`FrontendCapabilities` derives `Default` and is feature-**invariant**;
|
||||
`InstanceCapabilities` is feature-**dependent**. This is load-bearing,
|
||||
not an oversight: an instance advertises what it can do, a frontend
|
||||
**opts in** through negotiation, and a v1 frontend has no local CRDT
|
||||
state regardless of how the crate it links was compiled.
|
||||
- **Bite-verified rather than assumed**: mutating `multi_frontend` to a
|
||||
literal `false` produced `FAILED. 18 passed; 1 failed` with the
|
||||
expected assertion message; restoring returned 19/19.
|
||||
- **Left untested, deliberately:** `InstanceCapabilities::crdt_replica`
|
||||
carries `#[serde(default = "default_true")]`, a *third* default
|
||||
mechanism that is unconditional and therefore disagrees with the
|
||||
`Default` impl in a non-CRDT build. Exercising it needs a
|
||||
self-describing format, and this crate's only serde dependency is
|
||||
postcard, which is not one. Adding `serde_json` as a dev-dependency to
|
||||
test a divergence this lane did not introduce is scope creep; it is
|
||||
recorded here instead.
|
||||
|
||||
**Revision 2 → 3** records implementation findings, not a new design
|
||||
round. Three things changed:
|
||||
|
||||
|
|
|
|||
|
|
@ -2296,3 +2296,79 @@ pub enum InitialTargetResult {
|
|||
message: String,
|
||||
},
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod capability_default_tests {
|
||||
use super::{FrontendCapabilities, InstanceCapabilities};
|
||||
|
||||
// These exist because running code is not testing it.
|
||||
//
|
||||
// The `crdt` feature reaches this crate for exactly one purpose:
|
||||
// `InstanceCapabilities::default` reads `cfg!(feature = "crdt")`,
|
||||
// so the same source produces different advertised capabilities in
|
||||
// the two builds. Until these tests, the only thing exercising that
|
||||
// default was a transport round-trip, and a round-trip is INVARIANT
|
||||
// TO THE VALUES — an all-false default, or one where the three
|
||||
// fields disagreed with each other, encodes and decodes just as
|
||||
// happily and passes in both configurations.
|
||||
//
|
||||
// So the crate was being COMPILED both ways without either set of
|
||||
// values being asserted. That is the same defect class this lane
|
||||
// exists to close, one level down: the CI step that runs
|
||||
// pmacs-protocol under `crdt` executed this code but checked
|
||||
// nothing about it.
|
||||
|
||||
/// The instance advertises CRDT capability exactly when it was
|
||||
/// built with the feature. Advertising `true` on a non-CRDT build
|
||||
/// would be wire-protocol false advertising — the CRDT paths are
|
||||
/// conditionally compiled out — and advertising `false` on a CRDT
|
||||
/// build would strand every frontend in single-frontend mode.
|
||||
#[cfg(feature = "crdt")]
|
||||
#[test]
|
||||
fn instance_capability_defaults_are_enabled_under_crdt() {
|
||||
let caps = InstanceCapabilities::default();
|
||||
assert!(caps.multi_frontend, "multi_frontend must default true");
|
||||
assert!(caps.crdt_replica, "crdt_replica must default true");
|
||||
assert!(caps.semantic_render, "semantic_render must default true");
|
||||
}
|
||||
|
||||
/// The non-CRDT counterpart. All three track one `cfg!`, so a
|
||||
/// change that flipped only some of them would leave the daemon
|
||||
/// advertising a capability whose code paths are compiled out.
|
||||
#[cfg(not(feature = "crdt"))]
|
||||
#[test]
|
||||
fn instance_capability_defaults_are_disabled_without_crdt() {
|
||||
let caps = InstanceCapabilities::default();
|
||||
assert!(!caps.multi_frontend, "multi_frontend must default false");
|
||||
assert!(!caps.crdt_replica, "crdt_replica must default false");
|
||||
assert!(
|
||||
!caps.semantic_render,
|
||||
"semantic_render must default false; a semantic session is \
|
||||
necessarily a text replica and a non-CRDT build hosts neither"
|
||||
);
|
||||
}
|
||||
|
||||
/// **Deliberately NOT feature-gated** — this asserts the same thing
|
||||
/// in both builds, which is the point.
|
||||
///
|
||||
/// `FrontendCapabilities` derives `Default`, so it is
|
||||
/// feature-INVARIANT while `InstanceCapabilities` is
|
||||
/// feature-DEPENDENT. That asymmetry is load-bearing rather than an
|
||||
/// oversight: an instance advertises what it can do, while a
|
||||
/// frontend OPTS IN through the negotiation handshake, and a v1
|
||||
/// frontend has no local CRDT state regardless of how the crate it
|
||||
/// links was compiled. Making this one track the feature would have
|
||||
/// frontends claiming support they do not have.
|
||||
#[test]
|
||||
fn frontend_capability_defaults_do_not_track_the_crdt_feature() {
|
||||
let caps = FrontendCapabilities::default();
|
||||
assert!(
|
||||
!caps.multi_frontend,
|
||||
"frontend multi_frontend must default false in BOTH builds"
|
||||
);
|
||||
assert!(
|
||||
!caps.crdt_replica,
|
||||
"frontend crdt_replica must default false in BOTH builds"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue