fix(build): run every unittest target that was built, not two hardcoded ones #56
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "c4b/wire-envelope-tests"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found while verifying #54's t7 build: my new wire-envelope tests compiled and never executed. Neither did the ones already there — including the negative test whose own comment says it exists "so a maintainer has to argue the contract change explicitly". I cited that test as a guarantee earlier today. It has never run in this lane.
Two independent gaps, same shape
1. The target list was two items in a seven-item tree.
That list is mine, from earlier today — I restored encoder+framesink to close a 10-week silent-skip and didn't notice I was restoring a two-item list into a seven-item tree.
2. STEP 7 invoked those two binaries by name. So even adding a target to the lane would have changed nothing here — it'd build and sit there. The build goes green either way. Same failure shape as the silent-skip that STEP 7's fatal-on-missing check was added to close, one level up: there the binary was absent; here it exists and nobody invokes it.
The fix
STEP 7 now derives its binaries from
CHROMELESS_BUILD_TARGETS(any label whose target name ends in_unittests), so a target added to the lane is automatically executed and can't drift out of the runner again. Fatal-on-missing is preserved and now applies to every derived binary; a lane with no test targets logs a loud WARN rather than passing silently.Adds
cb_wire_envelope_unittests— that's the contract this repo publishes to third parties, so it's the one that most needs to run.Not adding
adm,input_dispatch,pointer_state. Nothing has compiled them in ~10 weeks; they may have bitrotted, and finding that out deserves its own change rather than being discovered while landing a codec fix.Verification
The derivation is shell, so it is verifiable without Chromium — exercised against a realistic target string:
bash -nclean. A t7 build is running now to answer the actual question: do the wire-envelope tests pass? That's the whole point of the change, and I'll report the result here.Stacked on #54 (c4/wire-envelope-dialects).
⚠ UNVERIFIED C++ — capture/ cannot be compiled here (no local Chromium; a build is 4-8h cold). A t7 build is running against this branch; do not merge until it is green. The research first, because the original plan was wrong about two things: 1. Physics is NOT a second dialect. webrtc_signaling.rs's SignalingEnvelope is #[serde(tag="type", rename_all="snake_case")] over variants Offer / Answer / Ice / …, so it emits exactly "offer", "answer", "ice" — byte identical to this codec's accept-list. The `sdp_offer` strings in that file are LOG LABELS (lines 1072, 1096), not wire tags. The plan's "delete physics's serde aliases" step has nothing to delete. 2. The PORTAL is the real second dialect, and it is genuinely incompatible. portal/src/canvas/browser_screencast_webrtc.rs:48-53: {"type":"sdp_offer", "sdp":"v=0..."} {"type":"sdp_answer", "sdp":"v=0..."} {"type":"ice_candidate", "candidate":"...", "sdpMid":"0", …} Flat: no `from`, payload inlined as siblings of `type`. Decode() rejected it on all three counts. It works today only because physics translates in the middle (pattern_c_envelope_to_portal_msg, screencast_ws.rs:6909). This change teaches Decode() the flat dialect and normalizes it to the same Envelope struct. Encode is untouched: we still always EMIT canonical, because an emitter that picked a dialect per-peer would need to know which peer it is addressing, which this codec deliberately does not. What this does NOT do, stated because the plan implied otherwise: it does not make pattern_c_envelope_to_portal_msg deletable. That translator is LOSSY in the other direction — it returns None for bye, request_renegotiate, probe_result and session_unhealthy, so those four tags never reach the portal at all. Removing it needs the portal to learn those tags first. "Liberalize the decoder" and "delete the translator" are two changes, not one. Design notes: * PortalTagFromString is a SEPARATE function from TagFromString, and the test asserts the two tag spaces stay disjoint. Decode() consults both; that is where the widening lives. Keeping it out of TagFromString is what stops "canonical" quietly becoming "anything we accept". * The accept-list stays CLOSED — three tags wider, not open. A new negative test uses `restart_ice`, well-formed in every respect except its tag. * `from` is absent in this dialect and is inferred as kClient. That is a statement (the portal is the browser-facing UI and the only producer of these frames), not a default — a future flat-but-not-client producer must send canonical instead. Callers do switch on `from`, so leaving it indeterminate was not an option. * A flat `ice_candidate` with no `candidate` field is the end-of-candidates marker: canonical says that with `data: null`, which has no flat equivalent because the payload IS the frame. The old RejectsHistoricSdpOfferTag test called sdp_offer "the historic v0 tag" and existed so that flipping the contract required an explicit argument rather than a quiet edit. The framing was wrong on the facts — it is neither historic nor v0, it is live — and this commit is that argument, made in the place the tripwire pointed at. conformance/suites/signaling-wire.mjs moves in lockstep: sdp_offer leaves REJECTED_TAGS for a new PORTAL_TAGS export. Without that the kit would assert the opposite of the code it ships beside. Verified locally: make verify passes, cxx-include-lint clean (121 files), conformance 23/23. NOT verified: that any of this compiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Found while verifying C4's t7 build: my new wire-envelope tests COMPILED and never executed. Neither did the ones that were already there — including the negative test whose own comment says it exists "so a maintainer has to argue the contract change explicitly". I cited that test as a guarantee earlier today. It has never run in this lane. Two independent gaps, same shape: 1. CHROMELESS_BUILD_TARGETS named two unittest targets. The tree declares SEVEN: cb_wire_envelope_unittests <- never built cloud_browser_adm_unittests <- never built cloud_browser_encoder_unittests built cloud_browser_framesink_capturer_unittests built cloud_browser_input_dispatch_unittests <- never built cloud_browser_pcf_unittests (t2-only, deliberate) cloud_browser_pointer_state_unittests <- never built Earlier today I restored encoder+framesink to close a 10-week silent-skip and did not notice I was restoring a two-item list into a seven-item tree. 2. STEP 7 invoked those two binaries BY NAME. So even adding a target to the lane would have changed nothing here — it would build and sit there. The mismatch is invisible: the build goes green either way. That is the same failure shape as the silent-skip STEP 7's fatal-on-missing check was added to close, one level up — there the binary was absent, here it exists and nobody invokes it. STEP 7 now DERIVES the list from CHROMELESS_BUILD_TARGETS (any label whose target name ends in _unittests), so a target added to the lane is automatically executed and cannot drift out of the runner again. The existing fatal-on-missing behaviour is preserved and now applies to every derived binary; a lane with no test targets at all logs a loud WARN rather than passing silently. Adds cb_wire_envelope_unittests to the x264-t7 lane. That is the contract this repo publishes to third parties, so it is the one that most needs to run. The other three unbuilt targets (adm, input_dispatch, pointer_state) are NOT added here: nothing has compiled them in ~10 weeks, they may have bitrotted, and finding that out is its own change rather than something to discover while landing a codec fix. Verified locally (this part is shell, so it is verifiable without Chromium): bash -n clean, and the derivation exercised against a realistic target string — picks exactly the two _unittests labels, correctly ignores cloud_browser_worker and headless:resource_pack_data. NOT verified: that cb_wire_envelope_unittests passes. A t7 build is running to find out; that is the entire point of the change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>✅ Verified on t7 — and it found one more defect first
The derived STEP 7 works. Final run:
Three binaries invoked from a three-target list. Before this change the list was derived from nothing — STEP 7 named two binaries literally, so the third built and sat there.
32/32 wire-envelope tests pass — the first time they have ever executed in any lane.
The bug this PR's own first run exposed (mine)
The initial version used
local -a test_binaries=()inside anifblock at script top level. bash rejectslocaloutside a function,set -eaborts instantly, and it prints nothing of its own:That reads exactly like an infrastructure kill — I chased reapers, node pressure and hostPath contention before reading my own diff. The compile had succeeded; four characters of mine failed.
bash -ndoes not catch it (runtime error, not syntax), so it passed every local check and the real feedback cost 27 minutes of Chromium.Fixed here, plus
tools/lint/shell_toplevel_local_lint.pyso the class cannot recur. Per CONTRIBUTING I checked the lint against the whole tree before landing: clean on all 35 shell scripts, and confirmed to fail on the real bug by reintroducing it on a copy. Wired into CI's Lint job.Merge order
This and #57 are complementary and both required: #57 makes the test compile (
//basewas a private dep in 19 targets), this makes STEP 7 invoke it. #54 depends on both.f539e9e46d18fa878fa9