Test-suite audit fixes: USB seam, macOS ports, dead-code deletion, clock seam + 1 real bug fix - #36
Merged
Merged
Conversation
…ivergence Addresses the test-suite audit (assessment.html) findings 1 and 5, plus the one genuine production bug the audit surfaced. lasercube_usb (finding 1): - Introduce a shared `UsbEndpoints` seam (src/protocols/usb_transfer.rs) mirroring the rusb DeviceHandle transfers, with a blanket impl for the real handle -- the DatagramSocket-equivalent the audit called for. Make `Stream<H: UsbEndpoints>` generic over it; `open()` stays concrete. - Add fake-device tests: chunking by bulk_packet_sample_count incl. the chunk_bytes==0 fallback, flip-x/flip-y defaults, rate clamp, status guard, short-write. Extract `classify_stream_error` as a pure fn with a table test covering every rusb::Error variant. Real bug fix (flagged by the audit): `try_write_points` fed the estimator the caller's unclamped pps while `write_frame` clamps the wire rate to the device max, so whenever pps > max_dac_rate the software buffer model drained faster than the hardware and could blank a frame's tail early. `write_frame` now returns the effective (clamped) rate and the backend records that. macOS test ports (finding 5): - The MockDac bound the fixed LaserCube ports on 127.0.0.101+ aliases, which macOS does not bring up -> 12 tests failed locally. Add cmd_port/data_port to AddressedDevice; the mock now binds ephemeral ports on 127.0.0.1. Add a macOS CI job so this regresses loudly.
Audit recommendation #5: a plain `cargo test` runs only the default feature set and silently compiles out the feature-gated integration tests (e.g. receiver_server.rs), so ~40% of the e2e surface never runs locally. Add `test-ci`/`check-ci`/`clippy-ci` cargo aliases, make the pre-commit hook use --all-features, and document it in the README.
… adapter-branch tests
… MockOpen Introduce a HeliosComm<H: UsbEndpoints> transfer engine that owns all frame and control USB logic (bulk writes with STALL/clear_halt recovery and short-write detection, interrupt control round-trips, firmware probe). The HeliosDac::Open variant now carries a HeliosComm over the concrete rusb handle and delegates to it; open() runs the init handshake then wraps the handle. Deletes the in-production #[cfg(test)] MockOpen variant, MockUsbState, and mock_check, along with every MockOpen short-circuit — so the encode + STALL + short-write paths (previously hardware-only, mock-bypassed) are now exercised through a scripted FakeUsb implementing UsbEndpoints, mirroring the LaserCube stream test pattern. Backend: leak/is_connected updated to the new enum shape; status-poll error handling extracted to on_status_error and frame prep to prepare_frame_buffer so the state machine and clamp/encode logic are unit-testable without a live device. Pure error-classification coverage preserved.
…k/park/reconnect coverage to run()
…g seam Audit finding #4 / recommendation #6. Introduce a `Clock` trait injected at the driver::run seam via `LoopCtx`, so pacing waits can be driven with virtual time in tests instead of real wall-clock sleeps. - `SystemClock` is the production clock (Instant::now + thread::sleep) and is wired in by default through `DriverInputs`, so runtime behavior is unchanged. - `LoopCtx`'s three sleep helpers and the estimator drain-poll now read time and sleep through `ctx.clock`. - A test `FakeClock` advances a virtual `now` on `sleep` instead of blocking; two demo tests show a 30s pacing wait completing with zero wall-clock time while virtual time advances the full duration, and a stop-request returning promptly. Per the audit's scoping (and confirmed with the maintainer), this lands the seam and a determinism demonstration; virtualizing the adapters' internal deadline state and converting the ~69 existing wall-clock test sleeps is a deliberately separate follow-up (the audit's "large" option, whose payoff it rated low since wall-clock estimator decay is itself correct real-world behavior).
The run_legacy deletion removed the IdlePolicy import the ChunkRequest doc link resolved against; use a fully-qualified path so `cargo doc -D warnings` (CI) passes.
The setup-rust composite action unconditionally ran `sudo apt-get`, which does not exist on macOS, so the `Test (macOS)` job failed at setup before any tests ran. libudev/libasound are Linux-only (macOS uses CoreAudio and its own USB stack), so gate the step behind `runner.os == 'Linux'`.
…nd local-CI parity tooling - helios: route HeliosComm through a boxed `UsbEndpoints + Send` seam and add a shared `test_support` fake transport so comm/backend tests exercise the frame/STALL-recovery paths that were previously hardware-only. - usb_transfer / presentation output_model / chunk_producer: expand seam and regression coverage (blocking FIFO, clock pacing). - tooling: add `doc-ci` cargo alias, run `cargo doc` (RUSTDOCFLAGS=-D warnings) in the pre-commit hook, and document the CI-mirroring aliases in the README so local runs match CI's blocking jobs.
test_fill_result_end_drains_with_queue_depth asserted the whole `run()` completes in <50ms against a 100ms drain timeout, leaving no headroom for `run()`'s setup/teardown — it failed on the loaded macOS CI runner where that overhead alone exceeded 50ms. Use a 2s drain-timeout sentinel and a 500ms ceiling so the empty-queue fast path (which breaks before its first poll sleep) is unmistakably distinguished from waiting the timeout, without a jitter-sensitive sub-100ms wall-clock budget.
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
1 participant
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.
Addresses every finding from the test-suite audit (
assessment.html). The audit's core thesis — strong suite where a seam exists; gaps map to where no seam was built — held up: across four independent analysis passes, only one genuine production bug surfaced (fixed here). The rest were test-quality gaps: dead-path tests, missing seams, and coverage at the wrong boundary.🐛 The one real bug (flagged by the audit, fixed here)
lasercube_usbestimator/rate divergence —try_write_pointsfed the software buffer estimator the caller's unclamped pps, whilewrite_frameclamps the wire rate to the device max. Wheneverpps > max_dac_rate, the estimate drained faster than the hardware and could blank a frame's tail early.write_framenow returns the effective (clamped) rate and the backend records that. Guarded by a regression test.Every other finding was a test-seam issue, not incorrect production code — the porting/coverage work below confirms the production paths behave correctly.
Findings addressed
rusb(newest code, zero behavioral tests)UsbEndpointstrait (theDatagramSocket-equivalent);Stream<H>genericized; fake-device tests for chunking /chunk_bytes==0fallback / flip defaults / rate clamp / short-write / status guard;classify_stream_errorextracted + table-tested#[cfg(test)] MockOpenbranches inside production methodsHeliosComm<H: UsbEndpoints>; deletedMockOpen/MockUsbState/mock_check; added hardware-free tests for the STALL/clear_halt, short-write, firmware-probe, and encode paths that were previously hardware-onlyrun_legacyscheduler; color-delay/startup-blank only tested on the dead pathrun_legacy+ its helpers + ~34 dead tests; ported color-delay / startup-blank / disarm-parking / reconnect-rejection coverage to the realrun()/ChunkProducerpath (asserting bytes reaching the backend, incl. x/y-preserved-while-color-shifted)FrameSession-level BlockingFifo tests (arm/disarm, ring-clear-per-rearm, filter, metrics, WouldBlock, reconnecton_reconnectring-clear) + NetworkFifo adapter-branch testslasercube_networktests failed on macOS (loopback-alias binds)MockDacbinds ephemeral ports on127.0.0.1via newAddressedDevicecmd/data ports; added a macOS CI jobcargo testsilently ran ~40% of the e2e suitetest-ci/check-ci/clippy-cialiases,--all-featurespre-commit hook, README sectionClockseam at thedriver::run/LoopCtxseam (SystemClockdefault = zero behavior change) +FakeClock+ demo tests showing a 30s pacing wait completing on virtual time with no wall-clock delayFinding 7 (scattered implementation-coupling — the audit's lowest-priority observation, not a numbered recommendation) is largely subsumed by finding 2's deletion of the legacy private-state tests; the residual private-field assertions in
presentation/tests.rsare left as a documented follow-up (refactoring passing tests there risks coverage loss for marginal benefit).Finding 4 scope was confirmed with the maintainer: land the seam + a determinism demo now; virtualizing the adapters' internal deadline state and converting the ~69 existing wall-clock test sleeps is a deliberate follow-up (the audit's "large" option, whose payoff it rated low since wall-clock estimator decay is itself correct real-world behavior).
Verification
cargo test --all-features→ 699 passed, 8 ignored;cargo clippy --all-features -- -D warningsclean;cargo fmt --checkclean;cargo doc --all-features(-D warnings) clean; feature-combination checks pass. The 12 macOS-only failures the audit reported now pass locally.🤖 Generated with Claude Code