Cover the source role with loopback wire-order tests - #115
Conversation
d9abea6 to
548df24
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Several tests can pass without fully verifying payload integrity or delayed permission leakage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds loopback integration coverage for source-role streaming and connection gates.
Changes:
- Tests wire ordering and PCM framing.
- Covers time-sync gating and reconnect permission reset.
- Adds an in-test WebSocket server harness.
File summaries
| File | Description |
|---|---|
tests/test_source_role.cpp |
Adds source-role loopback integration tests. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for (size_t i = 0; i < 16; ++i) { | ||
| EXPECT_EQ(static_cast<uint8_t>(first[WIRE_HEADER + i]), static_cast<uint8_t>(i & 0xFF)); | ||
| } | ||
|
|
||
| // The second chunk's anchor is exactly one chunk after the first (contiguous capture | ||
| // stamps); allow slack for the time filter refining its offset between the two sends. | ||
| const auto& second = events[static_cast<size_t>(binary_idx[1])].data; | ||
| ASSERT_EQ(second.size(), WIRE_HEADER + WIRE_CHUNK_PAYLOAD); | ||
| const int64_t ts2 = read_be64(reinterpret_cast<const uint8_t*>(second.data()) + 1); | ||
| EXPECT_NEAR(static_cast<double>(ts2 - ts1), 25000.0, 2000.0); | ||
|
|
||
| // The final short chunk carries the 10 ms remainder. | ||
| const auto& last = events[static_cast<size_t>(binary_idx[2])].data; | ||
| EXPECT_EQ(last.size(), WIRE_HEADER + WIRE_BYTES_PER_FRAME * 480); |
There was a problem hiding this comment.
Fixed in fcffae7: every payload byte of every chunk (both full chunks and the final short one) is audited against the write-side ramp, with a total-byte count pin.
| pump_for(harness.client, 400); | ||
| EXPECT_EQ(fake2.count_text_containing("client-stream/start"), 0U); | ||
| EXPECT_EQ(harness.listener.started, 1); |
There was a problem hiding this comment.
Fixed in fcffae7: the reconnect test now observes 700 ms of quiet — past the task's 500 ms failed-open retry window — before sending the fresh command, with the constant named at the site.
| // carrying [type 12][BE64 first-sample capture timestamp][PCM payload], then a final short | ||
| // chunk and client-stream/end on stop -- with write_audio() gated shut before the start | ||
| // command, validating frames while open, and gated shut again after the stop. | ||
| TEST(SourceWire, StreamsPcmEndToEndWithExactOrdering) { |
There was a problem hiding this comment.
Fixed in fcffae7: the mid-stream validation probes moved to their own focused test with an accepting control. The lifecycle gates (shut before start, open after, shut after stop) stay in the ordering test deliberately — they are the sequence under test — and are non-fatal EXPECTs, so they cannot mask the wire assertions.
Audit every payload byte of every chunk against the write-side ramp, pin the end-to-end test's 25 ms chunk arithmetic explicitly instead of leaning on the config default, split the mid-stream validation probes into their own focused test with an accepting control, and outlast the failed-open retry window before the reconnect test's fresh command.
Part 4/6 of the source@v1 stack (tracker: #95).
What it adds: loopback wire-order integration tests for the source role — a real client against an in-test WebSocket server, proving
client-stream/start→ typed+timestamped chunks →client-stream/endordering and every streaming gate (no start command, stale connection, no time sync, permission reset on reconnect).How it's used: tests only; no library changes.