test: replace fixed sleeps with bounded polling and add negative coverage for server-controlled parsers - #1094
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (25)
📝 WalkthroughWalkthroughThe PR replaces fixed async sleeps with bounded state-based synchronization, centralizes E2E helpers, expands app-state and privacy parser tests, adds JID fuzzing, and broadens runtime, shutdown, and allocation-test coverage. ChangesDeterministic test synchronization
E2E disconnect and helper consolidation
App-state decoding coverage
JID parser fuzzing
Runtime lifecycle coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| src/test_utils.rs | New poll_until, wait_for_outbound_tasks, and wait_for_notifier_listeners helpers; correctly uses tokio::time::Instant for the deadline so both the deadline and the sleep step live on the same clock. |
| tests/e2e/src/lib.rs | Adds wait_for_disconnected, hoists has_child, peer_session_addr, and scan_sessions from per-file copies; the new shared scan_sessions includes session records without a current state (returning has_pending_pre_key=false), whereas the previous session_reuse.rs copy silently excluded them — a subtle semantic change for callers that assert all returned sessions are fully established. |
| src/flush_scope.rs | Adds #[cfg(test)] flush_waiters() accessor exposing idle.total_listeners() so tests can poll a real observable instead of sleeping. |
| src/lib.rs | Extracts min_allocs from its two call sites into a shared helper; uses bounded retry (100k iterations) so a genuine regression fails with the real observed count rather than spinning forever. |
| wacore/appstate/src/decode.rs | Adds comprehensive negative-case tests for every rejection branch of decode_record plus a happy-path and a non-JSON-index tolerance test. |
| wacore/appstate/src/patch_decode.rs | New test module covering parse_patch_list, parse_patch_lists, error collection variants, and CollectionSyncError display. |
| wacore/binary/fuzz/fuzz_targets/parse_jid.rs | New fuzz target checking panic-freedom and display/parse round-trip; build_jid carefully excludes separator chars to avoid non-round-trippable edge cases. |
| wacore/src/runtime.rs | New direct tests for AbortHandle and Tokio runtime (timeout, abort, detach, blocking); all bounded by a 5s test timeout. |
Reviews (5): Last reviewed commit: "test: bound the runtime helper awaits an..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📦 Binary size report
.text per crate
Baseline: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/tests/chatstate_ttl.rs (1)
57-61: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep the recipient offline until the chatstate is sent.
wait_for_disconnected()only observes a transient teardown. Withreconnect_immediately(), B can reconnect before line 63, letting this pass through live delivery rather than validating offline queuing within the TTL. Gate reconnect in the mock or use a test hook that holds the reconnect until after the send.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/tests/chatstate_ttl.rs` around lines 57 - 61, Update the reconnect flow in the chatstate TTL test around client_b.client.reconnect_immediately() and wait_for_disconnected() so recipient B remains offline until the chatstate send completes. Gate or defer B’s reconnect using the mock or an existing test hook, then release it after the send, ensuring delivery exercises offline queuing rather than live delivery.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent_docs/e2e_testing.md`:
- Around line 84-85: Update TestClient::disconnect() to fail or synchronously
abort and join the run task when its five-second timeout expires, then retain
the documented completed-shutdown contract. In agent_docs/e2e_testing.md lines
84-85, qualify the statement until that guarantee exists. In
tests/e2e/tests/receipts.rs lines 274-276, remove reliance on immediate socket
closure or enforce it through the corrected helper.
In `@src/client/tests.rs`:
- Around line 839-840: The comments at src/client/tests.rs lines 839-840 and
963-964 should explain why notifier registration matters, not merely what it
does. Update the comment near wait_for_notifier_listeners at lines 839-840 to
state that registration proves the offline-sync waiter reached its await point,
and update the comment at lines 963-964 to state that registration makes the
pending-session assertion scheduler-independent.
In `@wacore/binary/fuzz/fuzz_targets/parse_jid.rs`:
- Around line 72-82: The fuzz target currently discards results from
parse_jid_fast, parse_jid_ref, and Jid formatter methods, so divergences and
invalid formatting are not detected. Retain both parser results and assert their
normalized outputs agree, then retain to_string, to_ad_string, and
to_non_ad_string results and validate each against its documented invariant
while preserving the existing device_key and display_eq checks.
---
Outside diff comments:
In `@tests/e2e/tests/chatstate_ttl.rs`:
- Around line 57-61: Update the reconnect flow in the chatstate TTL test around
client_b.client.reconnect_immediately() and wait_for_disconnected() so recipient
B remains offline until the chatstate send completes. Gate or defer B’s
reconnect using the mock or an existing test hook, then release it after the
send, ensuring delivery exercises offline queuing rather than live delivery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee89c47d-a87d-44d3-924a-f5bbd9f2c478
📒 Files selected for processing (23)
Cargo.tomlagent_docs/e2e_testing.mdsrc/client/extension_lifecycle.rssrc/client/tests.rssrc/flush_scope.rssrc/message/tests.rssrc/test_utils.rstests/e2e/src/lib.rstests/e2e/tests/chatstate_ttl.rstests/e2e/tests/lid_sessions.rstests/e2e/tests/offline_groups.rstests/e2e/tests/offline_messages.rstests/e2e/tests/offline_receipts.rstests/e2e/tests/privacy_tokens.rstests/e2e/tests/receipts.rstests/e2e/tests/session_reuse.rstests/e2e/tests/status.rswacore/appstate/src/decode.rswacore/appstate/src/patch_decode.rswacore/binary/fuzz/Cargo.tomlwacore/binary/fuzz/fuzz_targets/parse_jid.rswacore/src/iq/privacy.rswacore/src/runtime.rs
|
On the outside-diff note about The reconnect race is pre-existing: the test already called Gating reconnect in the mock server is a real improvement, but it's a change to the test harness rather than a sleep-to-poll swap, so it belongs in its own change. Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/src/lib.rs`:
- Around line 501-504: The disconnect barrier must establish a stable offline
period rather than relying on the first disconnected observation. In
tests/e2e/src/lib.rs lines 501-504, update the disconnect-waiting logic to
expose or await confirmation that the client remains offline despite reconnect
scheduling. In tests/e2e/tests/receipts.rs lines 110, 190, 228, and 239, await
that stable offline condition for the specified client before sending, marking
messages read, or receiving messages.
In `@wacore/binary/fuzz/fuzz_targets/parse_jid.rs`:
- Around line 96-104: Update the non-empty-user branch in the JID fuzz target so
parsing the result of jid.to_non_ad_string() is asserted to succeed rather than
conditionally ignored. Preserve the existing identity assertion using the
successfully parsed bare JID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3927f387-cafc-4aa2-834d-92bdefbcf4c8
📒 Files selected for processing (6)
agent_docs/e2e_testing.mdsrc/client/tests.rssrc/test_utils.rstests/e2e/src/lib.rstests/e2e/tests/receipts.rswacore/binary/fuzz/fuzz_targets/parse_jid.rs
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Heads-up on
Both changes are mutation-verified rather than just "passes now":
No fixed sleeps were reintroduced anywhere. Generated by Claude Code |
`reconnect()` tears the socket down in a background task, so the fixed 100ms sleep that followed it was the only thing standing between an offline test and a client that had not gone offline yet. Add `TestClient::wait_for_disconnected()` and wait on the connection flag, which is the observable here since `Event::Disconnected` is suppressed for expected disconnects. Sleeps that followed `disconnect()` are dropped outright: it already awaits the run task. Also hoist the helpers that had been copied per test file (`has_child`, `scan_sessions` and its address builder) into the shared crate, keeping the single form that reports both the JID and whether a session record exists.
Every `sleep(10ms); assert!(task.is_finished())` in the unit tests was a bet on the scheduler: too short and it flakes on a loaded runner, long enough to be safe and the suite pays for it on every run. Add a shared `poll_until` helper with a 5s deadline and a clear failure message, and build the waits the tests actually need on top of it. Each converted site now polls a real observable rather than elapsed time: - registered `event_listener` listeners for "a task is parked here" — both the offline-sync notifier and `FlushScope::idle` register their listener before re-checking their condition, so a listener proves the waiter reached its await point; - the lifecycle `terminal` flag, which is set immediately before shutdown reaches for the scope-registry lock a blocked callback is holding; - `FlushScope::pending()` for the retry-receipt counters, since `spawn` takes its guard synchronously and a drained scope therefore proves the spawned task ran — including when it must deliberately do nothing. The PDO sleeps guarded nothing at all: `run_pdo_request` is fully awaited, and four of those tests asserted nothing after sleeping. They now assert on its return value, which distinguishes an armed request from a skipped one.
`decode_record` rejects seven distinct shapes of malformed server data and none of them were exercised; `patch_decode` had no tests at all. Both now have a happy path plus a case per rejection, so a regression that starts accepting a truncated index or a mismatched MAC fails here instead of downstream. Same for the privacy IQ parser, mirroring the malformed-response tests in `iq::groups`, and for `wacore::runtime`, where `ShutdownNotifier`, `AbortHandle` (including abort-on-drop) and `timeout` had only indirect coverage through callers.
JIDs arrive from the wire as arbitrary strings, so the parser is reachable with anything. The target parses, and where parsing succeeds also round-trips through `to_string` and re-parses, so a shape that survives one direction but not the other is caught too.
`poll_until` timed its deadline with the wall clock while stepping with `tokio::time::sleep`, so a test that ever paused Tokio's clock would hang until the real deadline fired and then blame the condition. Both sides now use `tokio::time::Instant`. `wait_for_disconnected` re-samples the connection flag once the deadline passes: teardown runs on another task, so a disconnect that lands between the flag check and the deadline check is a success, not a timeout.
The fuzz target discarded the results of every parse and format call, so it only ever caught panics. Add two oracles it can actually fail on: - parse_jid_ref only layers server validation over parse_jid_fast, so it must never accept text the scanner rejected, and the parts they keep must match. - to_non_ad_string must re-parse to the same identity with agent and device cleared. to_ad_string deliberately gets no round-trip check: it always renders user.agent:device, which the parser does not read back as an agent. Also clarify that TestClient::disconnect() caps its wait and warns rather than failing, and say why notifier registration is the thing the still-waiting assertions depend on.
The helper caps its wait on the run task and warns rather than failing, so the no-receipt assertion rests on the assert_no_event window and B never reconnecting, not on the socket being provably down at that line.
The fuzz target asserted that `to_non_ad_string` always re-parses to the same identity. It does not: the parse fallback can leave `.` or `:` inside the user part, and re-rendering that as `user@server` lets the parser read those separators back as an agent or device. A brute-force sweep over 32768 short inputs found 1739 such cases, e.g. `0.1:@s.whatsapp.net` parses to user `0.1`, renders to `0.1@s.whatsapp.net`, and comes back as user `0` with device 1. Guard the assertion on a separator-free user, which the same sweep clears with zero failures.
Inside the separator-free guard the non-AD rendering is just `user@server`, so a formatter regression that emits something unparseable should fail the target loudly instead of being skipped by an `if let Ok`. A sweep over 32768 short inputs found zero parse failures here, so the stricter form is the accurate oracle.
`test_alloc::ALLOCS` is a global-allocator hook, so any concurrently running test inflates a measurement window. The three call sites took the minimum over a fixed 100 windows and hoped one of them landed quiet; under a loaded CI runner all 100 can be dirty, which is how `lookup_borrows_composite_key_without_allocating` failed with 2 instead of 1. Replace the fixed count with `test_alloc::min_allocs`, which retries until the delta reaches the expected value under a large bounded budget. Ambient traffic now costs iterations rather than a false failure, while a real regression never reaches the target and still fails with the observed count. Verified by mutation: adding one allocation to the measured op fails the assertion, and the full budget runs in ~0.3s. Also strengthen `notify_wakes_every_registered_listener`. Its waiters were never polled before `notify()`, so nothing was registered, and resolution is the wrong observable anyway: the sticky flag lets a re-polled waiter finish whether or not it was woken, so the test passed with `notify(1)`. Poll every waiter to Pending first and count wakeups through per-waiter wakers. Verified by mutation: `notify(1)` now fails the test.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wacore/src/runtime.rs (2)
507-510: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBound the blocking-helper test.
This await bypasses
TEST_TIMEOUT, so ifspawn_blockingever regresses and never completes, CI can hang instead of failing clearly.Proposed fix
- let value = super::blocking(&TokioTestRuntime, || 6 * 7).await; + let value = tokio::time::timeout( + TEST_TIMEOUT, + super::blocking(&TokioTestRuntime, || 6 * 7), + ) + .await + .expect("timed out waiting for the blocking closure"); assert_eq!(value, 42);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/src/runtime.rs` around lines 507 - 510, Update the blocking_ferries_the_closure_result_back test to await the blocking helper through the existing TEST_TIMEOUT mechanism. Preserve the current closure result and 42 assertion while ensuring a stalled spawn_blocking operation fails within the configured test timeout.
430-432: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace
yield_now()with an explicit task-readiness signal.
tokio::task::yield_now()only puts the current task back in the queue; it does not guarantee the spawned task will be polled beforehandle.abort(). Send a one-shot signal when the task starts polling, await it withTEST_TIMEOUT, and keeppending()as the park point to cover the cancel-a-running-task path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/src/runtime.rs` around lines 430 - 432, Replace the yield_now synchronization in the spawned-task cancellation test with an explicit one-shot readiness signal sent when the task begins polling, and await that signal using TEST_TIMEOUT before calling handle.abort(). Keep pending() as the task’s park point so the test still exercises cancellation of a running task.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/binary/fuzz/fuzz_targets/parse_jid.rs`:
- Around line 93-101: Condense the comment above the re-parse logic to briefly
explain that separator characters in the user part create parsing ambiguity,
while omitting the detailed walkthrough, examples, and to_ad_string discussion.
Preserve only the why-focused rationale for restricting the round-trip case.
---
Outside diff comments:
In `@wacore/src/runtime.rs`:
- Around line 507-510: Update the blocking_ferries_the_closure_result_back test
to await the blocking helper through the existing TEST_TIMEOUT mechanism.
Preserve the current closure result and 42 assertion while ensuring a stalled
spawn_blocking operation fails within the configured test timeout.
- Around line 430-432: Replace the yield_now synchronization in the spawned-task
cancellation test with an explicit one-shot readiness signal sent when the task
begins polling, and await that signal using TEST_TIMEOUT before calling
handle.abort(). Keep pending() as the task’s park point so the test still
exercises cancellation of a running task.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 770f27d3-c599-486f-ab8c-44f856437760
📒 Files selected for processing (5)
src/client/tests.rssrc/lib.rssrc/msg_secret_buffer.rswacore/binary/fuzz/fuzz_targets/parse_jid.rswacore/src/runtime.rs
Two runtime tests could hang instead of failing: the blocking helper was awaited without a deadline, and the parked-task helper relied on `yield_now` to get the spawned future polled, which only requeues the calling task. Wrap the blocking await in TEST_TIMEOUT and have the spawned task report its first poll over a oneshot, so `abort()` provably cancels a running task. Also trims the JID fuzz-target and allocation-counter comments down to the reasoning, dropping the implementation walkthroughs.
11f5a20 to
835a055
Compare
2 participants
Summary
Two kinds of gap in the suite: tests that synchronize on a fixed sleep, and parsers that reject malformed server data along branches nothing ever exercised. The sleeps are the more urgent half — every one of them is a bet on the scheduler, so it either flakes on a loaded runner or makes the suite slower on every single run. All of them are gone; the rule going forward is that a test waits on the condition itself, or polls it with a bounded deadline that fails with a clear message.
No production behavior changes here. The one non-test change is a
#[cfg(test)]accessor onFlushScopeso tests can see how many flushers are parked on it.Changes
Anti-flakiness
TestClient::wait_for_disconnected()replaces the 100ms sleeps that followedreconnect()across the offline e2e tests.reconnect()tears the socket down in a background task, so the sleep was the only thing standing between the test and a client that had not gone offline yet — andEvent::Disconnectedis suppressed for expected disconnects, which leaves the connection flag as the observable. Sleeps that followeddisconnect()are dropped outright rather than converted: it already awaits the run task, so there was nothing to wait for.In the unit tests, a shared
poll_untilhelper (5s deadline, names what it was waiting for on failure) replaces thesleep(10ms); assert!(task.is_finished())pattern. Each site polls a real observable:event_listenerlisteners as the proof that "a task is parked here" — the offline-sync notifier andFlushScope::idleboth register their listener before re-checking their condition, so a listener means the waiter genuinely reached its await point;terminalflag, which is set immediately before shutdown reaches for the scope-registry lock the blocked close callback holds;FlushScope::pending()for the retry-receipt counter tests —spawntakes its guard synchronously, so a drained scope proves the spawned task ran, including the cases where it must deliberately do nothing.The PDO sleeps turned out to be guarding nothing:
run_pdo_requestis fully awaited, and four of those tests asserted nothing at all after sleeping. They now assert on its return value, which is what actually distinguishes an armed request from a skipped one.agent_docs/e2e_testing.mdis updated because it still told people to sleep 100ms afterreconnect(), which would have let the pattern grow back.Negative coverage
decode_recordrejects seven distinct shapes of malformed server data and had none of them under test;patch_decodehad no tests at all. Both get a happy path plus a case per rejection, so a regression that starts accepting a truncated index or a mismatched MAC fails at the parser instead of somewhere downstream. Same treatment for the privacy IQ parser, following the malformed-response tests already iniq::groups.wacore::runtimegets direct tests forShutdownNotifierwith multiple listeners,AbortHandle::abort()and abort-on-drop, andtimeoutboth expiring and completing — previously only covered indirectly through callers.Fuzz
New
parse_jidtarget next tounmarshal_ref. JIDs are attacker-controlled attribute values on every stanza and the parse path mixes a hand-rolled scanner with string slicing, so it checks two properties: arbitrary text never panics, and a JID built from the fuzz bytes survives a render/re-parse round-trip unchanged.Helpers
has_childwas copied into two e2e test files andscan_sessionsexisted in two divergent forms. Both are hoisted into the shared e2e crate, keeping the single form that reports the JID along with whether a session record exists.Validation
The touched e2e tests are left to CI — no mock server available locally.
One deviation worth flagging: the branch is
claude/test-suite-quality-t2n6bfrather thantest/polling-and-negative-coverage, because the branch name was fixed by the environment this ran in.Generated by Claude Code