Tests split to improve paralelism - #317
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR reorganizes documentation from architecture-focused content to a structured guide with external references, introduces new debugging and testing best practices docs, refactors the offline test suite into focused modules by scenario type, and adds connection-related utility methods to Client and TestClient. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Bencher Report![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
tests/e2e/tests/offline_groups.rs (1)
383-431: Consider moving the 35s TTL test to a separate file for parallelism.Per the documentation in
agent_docs/e2e_testing.md, tests within a file run sequentially while files run in parallel. This 35-second sleep makes the entireoffline_groups.rsfile take at least 35s, becoming a bottleneck.Consider moving
test_expired_chatstate_not_deliveredto a separate file (e.g.,chatstate_ttl.rs) so it can run in parallel with other offline group tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/offline_groups.rs` around lines 383 - 431, The long 35s sleep in test_expired_chatstate_not_delivered is making offline_groups.rs run slowly because tests in the same file are sequential; move the entire async fn test_expired_chatstate_not_delivered (including its imports/setup usage of TestClient, Event, tokio::test, and env_logger init) into a new test file (e.g., chatstate_ttl.rs) under tests/e2e/tests, remove it from offline_groups.rs, and ensure the new file contains the same top-level use/imports and module attributes so the test compiles and runs independently (keeping the function name test_expired_chatstate_not_delivered and its logic unchanged) which allows parallel execution of the TTL test with the other offline group tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent_docs/protocol_architecture.md`:
- Around line 118-125: Update the fenced code block showing the directory tree
for "wacore/src/iq/" by adding the language identifier "text" after the opening
backticks so the block becomes a ```text fenced block; locate the block that
contains the lines starting with "wacore/src/iq/" and the entries "mod.rs",
"spec.rs", "node.rs", "groups.rs", and "blocklist.rs" and change the opening
fence from ``` to ```text to satisfy markdownlint MD040.
In `@tests/e2e/src/lib.rs`:
- Around line 192-198: The reconnect_and_wait helper can match a stale
Event::Connected from the TestClient's long-lived broadcast channel; before
calling self.client.reconnect().await, drain any buffered Connected events from
the TestClient's broadcast receiver (the same channel wait_for_event uses) so
that only a new Connected produced by the upcoming reconnect is matched; then
call self.client.reconnect().await and keep the existing wait_for_event(10, |e|
matches!(e, Event::Connected(_))). Ensure you reference the same receiver used
by wait_for_event (the TestClient broadcast channel) when implementing the drain
loop so old Connected events are removed.
In `@tests/e2e/tests/offline_groups.rs`:
- Around line 413-425: The test currently treats an Ok(event) as a non-failing
warning when a chatstate is received after TTL expiry; change this to an
explicit assertion so the test fails on TTL enforcement regressions: in
tests/e2e/tests/offline_groups.rs replace the match on result (and the info!
warning path referencing event) with an assertion such as
assert!(result.is_err(), "B received chatstate after TTL expired: {:?}", event)
or, if TTL is intentionally unimplemented, annotate the test with #[ignore] and
add a TODO comment; target the match/result handling and the info!/event usage
when making the change.
- Around line 465-475: The test currently only logs whether `result` matched Ok
or Err so it never fails; change the match on `result` to assert success
instead: replace the match with either `let event = result.expect("Expected
fresh chatstate to be delivered to B")` (or `assert!(result.is_ok(), "B did not
receive chatstate within timeout: {:?}", result.err())`), then keep the
`info!("B received fresh chatstate: {:?}", event);` line using the unwrapped
`event`; remove the silent-accepting `Err(e)` branch so the test fails when
`result` is an error.
In `@tests/e2e/tests/offline_messages.rs`:
- Around line 34-37: The fixed 100ms sleep after dropping client_b.connection
(client_b.client.reconnect().await) is racing; replace sleeps with explicit
waits for connection state transitions: after calling
client_b.client.reconnect().await, await the client-side "disconnected"
confirmation (e.g., a helper like client_b.wait_for_disconnected() or polling
client_b.connection_state) and then await the server-observed disconnect if
available, and similarly await client_b.wait_for_reconnected() before
proceeding. Update the three places mentioned (the reconnect block around
client_b.client.reconnect(), and the other occurrences at the blocks currently
using tokio::time::sleep) to use these explicit disconnected/reconnected wait
helpers or explicit state checks so offline-queue assertions are deterministic.
- Around line 193-211: The test currently only collects msg_ids from
client_a.client.send_message and never verifies delivery to client_b; update the
test (in offline_messages.rs) to reconnect or start client_b after sending and
drain its events (watching for Event::Message) to assert that all five messages
with matching conversation text ("Offline message 1".."Offline message 5") are
received, using jid_b and msg_ids to correlate deliveries, or alternatively
rename the test to indicate it only verifies sender-side acceptance; modify the
code around msg_ids, client_a.client.send_message, and client_b
reconnection/event loop accordingly.
In `@tests/e2e/tests/offline_receipts.rs`:
- Around line 30-33: Replace the brittle tokio::time::sleep calls (e.g., the
sleep after client_b.disconnect().await) with explicit state-based waits that
observe the harness/client state (for example, poll or await a
harness.wait_for_disconnected(client_b) or loop until client_b.is_disconnected()
returns true) before proceeding; do the same for reconnect waits (use
harness.wait_for_reconnected(client_x) or client_x.is_connected()) wherever
tokio::time::sleep is used (notably the blocks around lines 90-93, 104-107,
175-178, 283-286) so the test only proceeds when the actual
disconnected/reconnected state is reached rather than relying on fixed delays.
- Around line 122-149: The test currently swallows a timeout from
client_a.wait_for_event in the Err(e) arm and only logs, so it should instead
fail the test (or mark the test ignored if the flow isn't implemented). Replace
the Err(e) block in the match on receipt_event so it panics or asserts (e.g.,
panic!("A did not receive deferred delivery receipt: {}", e) or assert!(false,
...)) to fail the test on timeout; if this behavior isn't implemented yet,
annotate the test with #[ignore] and add a TODO comment referencing the deferred
receipt flow. Ensure changes target the match handling of receipt_event and the
client_a.wait_for_event call.
- Around line 297-320: The test currently accepts any Event::Presence and
doesn't verify sender or final state; update the assertions so the first
presence_event is from jid_a and its presence.state (or equivalent field on the
Presence struct) equals "available" and then assert that the second
wait_for_event call fails (i.e. assert!(second.is_err(), "Expected no second
presence due to coalescing")), referencing client_b, wait_for_event,
Event::Presence, jid_a and the presence.state field to locate the checks;
alternatively mark the test #[ignore] if coalescing isn't implemented yet.
In `@tests/e2e/tests/receipts.rs`:
- Line 150: The 100ms hardcoded pause after reconnect() in
tests/e2e/tests/receipts.rs is too short and can cause flakiness; change the
sleep to a configurable delay (e.g., read from an env var like
E2E_RECONNECT_DELAY_MS with a sensible default such as 1000ms) and use that
value for tokio::time::sleep(Duration::from_millis(...)). This ensures the test
waits long enough for server-side session cleanup (disconnect() in
src/client.rs) before invoking send_message, while allowing CI tuning without
modifying source.
---
Nitpick comments:
In `@tests/e2e/tests/offline_groups.rs`:
- Around line 383-431: The long 35s sleep in
test_expired_chatstate_not_delivered is making offline_groups.rs run slowly
because tests in the same file are sequential; move the entire async fn
test_expired_chatstate_not_delivered (including its imports/setup usage of
TestClient, Event, tokio::test, and env_logger init) into a new test file (e.g.,
chatstate_ttl.rs) under tests/e2e/tests, remove it from offline_groups.rs, and
ensure the new file contains the same top-level use/imports and module
attributes so the test compiles and runs independently (keeping the function
name test_expired_chatstate_not_delivered and its logic unchanged) which allows
parallel execution of the TTL test with the other offline group tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ab25a8b-3184-4654-8992-fb3925c396af
📒 Files selected for processing (12)
AGENTS.mdCLAUDE.mdagent_docs/debugging.mdagent_docs/e2e_testing.mdagent_docs/feature_implementation.mdagent_docs/protocol_architecture.mdtests/e2e/src/lib.rstests/e2e/tests/offline_events.rstests/e2e/tests/offline_groups.rstests/e2e/tests/offline_messages.rstests/e2e/tests/offline_receipts.rstests/e2e/tests/receipts.rs
💤 Files with no reviewable changes (1)
- tests/e2e/tests/offline_events.rs
Comment on lines
+413
to
+425
| match result { | ||
| Err(_) => { | ||
| info!("Confirmed: expired chatstate was NOT delivered to B"); | ||
| } | ||
| Ok(event) => { | ||
| // This might happen if the server doesn't implement TTL expiry yet | ||
| info!( | ||
| "WARNING: B received chatstate after TTL should have expired: {:?}", | ||
| event | ||
| ); | ||
| // Don't fail — this reveals what needs fixing | ||
| } | ||
| } |
There was a problem hiding this comment.
Weak assertion: test passes even when TTL enforcement fails.
The test logs a warning but doesn't fail when an expired chatstate IS delivered. This makes the test ineffective as a regression guard—a broken TTL implementation would silently pass.
If TTL enforcement isn't implemented yet in the mock server, consider marking this test with #[ignore] and a TODO comment, or using assert!(result.is_err(), ...) to make the expectation explicit.
Suggested fix to enforce the expected behavior
match result {
Err(_) => {
info!("Confirmed: expired chatstate was NOT delivered to B");
}
Ok(event) => {
- // This might happen if the server doesn't implement TTL expiry yet
- info!(
- "WARNING: B received chatstate after TTL should have expired: {:?}",
- event
- );
- // Don't fail — this reveals what needs fixing
+ panic!(
+ "Expired chatstate should NOT be delivered after TTL. Got: {:?}",
+ event
+ );
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| match result { | |
| Err(_) => { | |
| info!("Confirmed: expired chatstate was NOT delivered to B"); | |
| } | |
| Ok(event) => { | |
| // This might happen if the server doesn't implement TTL expiry yet | |
| info!( | |
| "WARNING: B received chatstate after TTL should have expired: {:?}", | |
| event | |
| ); | |
| // Don't fail — this reveals what needs fixing | |
| } | |
| } | |
| match result { | |
| Err(_) => { | |
| info!("Confirmed: expired chatstate was NOT delivered to B"); | |
| } | |
| Ok(event) => { | |
| panic!( | |
| "Expired chatstate should NOT be delivered after TTL. Got: {:?}", | |
| event | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/offline_groups.rs` around lines 413 - 425, The test currently
treats an Ok(event) as a non-failing warning when a chatstate is received after
TTL expiry; change this to an explicit assertion so the test fails on TTL
enforcement regressions: in tests/e2e/tests/offline_groups.rs replace the match
on result (and the info! warning path referencing event) with an assertion such
as assert!(result.is_err(), "B received chatstate after TTL expired: {:?}",
event) or, if TTL is intentionally unimplemented, annotate the test with
#[ignore] and add a TODO comment; target the match/result handling and the
info!/event usage when making the change.
Comment on lines
+465
to
+475
| match result { | ||
| Ok(event) => { | ||
| info!("B received fresh chatstate: {:?}", event); | ||
| } | ||
| Err(e) => { | ||
| info!( | ||
| "B did not receive chatstate within timeout: {} (may need mock server fix)", | ||
| e | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Weak assertion: test passes regardless of outcome.
Similar to the expired chatstate test, this test doesn't assert that the fresh chatstate was actually delivered. If the mock server fails to deliver it, the test silently passes with a log message.
Suggested fix to assert expected behavior
match result {
Ok(event) => {
info!("B received fresh chatstate: {:?}", event);
}
Err(e) => {
- info!(
- "B did not receive chatstate within timeout: {} (may need mock server fix)",
- e
- );
+ panic!(
+ "Fresh chatstate (within TTL) should be delivered on reconnect. Error: {}",
+ e
+ );
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| match result { | |
| Ok(event) => { | |
| info!("B received fresh chatstate: {:?}", event); | |
| } | |
| Err(e) => { | |
| info!( | |
| "B did not receive chatstate within timeout: {} (may need mock server fix)", | |
| e | |
| ); | |
| } | |
| } | |
| match result { | |
| Ok(event) => { | |
| info!("B received fresh chatstate: {:?}", event); | |
| } | |
| Err(e) => { | |
| panic!( | |
| "Fresh chatstate (within TTL) should be delivered on reconnect. Error: {}", | |
| e | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/offline_groups.rs` around lines 465 - 475, The test currently
only logs whether `result` matched Ok or Err so it never fails; change the match
on `result` to assert success instead: replace the match with either `let event
= result.expect("Expected fresh chatstate to be delivered to B")` (or
`assert!(result.is_ok(), "B did not receive chatstate within timeout: {:?}",
result.err())`), then keep the `info!("B received fresh chatstate: {:?}",
event);` line using the unwrapped `event`; remove the silent-accepting `Err(e)`
branch so the test fails when `result` is an error.
Comment on lines
+34
to
+37
| // Drop client_b's connection (triggers auto-reconnect) | ||
| client_b.client.reconnect().await; | ||
| info!("Client B connection dropped, will auto-reconnect"); | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; |
There was a problem hiding this comment.
Synchronize the offline boundary instead of sleeping.
A fixed 100 ms sleep here does not prove whether the client is still offline, already reconnected, or whether the server has observed the disconnect. That makes the offline queue assertions timing-dependent and brittle once these tests run in parallel. Please wait on an explicit disconnected/reconnected state before sending or asserting.
Also applies to: 88-90, 189-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/offline_messages.rs` around lines 34 - 37, The fixed 100ms
sleep after dropping client_b.connection (client_b.client.reconnect().await) is
racing; replace sleeps with explicit waits for connection state transitions:
after calling client_b.client.reconnect().await, await the client-side
"disconnected" confirmation (e.g., a helper like
client_b.wait_for_disconnected() or polling client_b.connection_state) and then
await the server-observed disconnect if available, and similarly await
client_b.wait_for_reconnected() before proceeding. Update the three places
mentioned (the reconnect block around client_b.client.reconnect(), and the other
occurrences at the blocks currently using tokio::time::sleep) to use these
explicit disconnected/reconnected wait helpers or explicit state checks so
offline-queue assertions are deterministic.
Comment on lines
+30
to
+33
| // Disconnect client_b fully (stops run loop — no reconnect) | ||
| client_b.disconnect().await; | ||
| info!("Client B fully disconnected"); | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; |
There was a problem hiding this comment.
Replace the 100 ms sleeps with state-based waits.
These tests depend on exact offline/online transitions, but right now they just sleep and assume the transition has completed. That makes the receipt assertions timing-dependent and flaky under load or parallel CI. Prefer waiting for an explicit disconnected/reconnected state from the harness before sending or asserting.
Also applies to: 90-93, 104-107, 175-178, 283-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/offline_receipts.rs` around lines 30 - 33, Replace the
brittle tokio::time::sleep calls (e.g., the sleep after
client_b.disconnect().await) with explicit state-based waits that observe the
harness/client state (for example, poll or await a
harness.wait_for_disconnected(client_b) or loop until client_b.is_disconnected()
returns true) before proceeding; do the same for reconnect waits (use
harness.wait_for_reconnected(client_x) or client_x.is_connected()) wherever
tokio::time::sleep is used (notably the blocks around lines 90-93, 104-107,
175-178, 283-286) so the test only proceeds when the actual
disconnected/reconnected state is reached rather than relying on fixed delays.
| client_b.client.reconnect().await; | ||
| info!("B disconnected (will auto-reconnect)"); | ||
| tokio::time::sleep(Duration::from_millis(500)).await; | ||
| tokio::time::sleep(Duration::from_millis(100)).await; |
There was a problem hiding this comment.
Reduced sleep (100ms) may cause flakiness in slower CI environments.
The 100ms sleep after reconnect() assumes localhost TCP close detection is near-instant. Per src/client.rs:744-754, disconnect() awaits transport teardown but doesn't guarantee server-side session cleanup. If the mock server hasn't registered the offline state before send_message executes, the message may be delivered immediately instead of queued.
The generous event timeouts (15-30s) provide a safety net, but consider parameterizing this delay via an environment variable for CI tuning if flakiness appears.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/receipts.rs` at line 150, The 100ms hardcoded pause after
reconnect() in tests/e2e/tests/receipts.rs is too short and can cause flakiness;
change the sleep to a configurable delay (e.g., read from an env var like
E2E_RECONNECT_DELAY_MS with a sensible default such as 1000ms) and use that
value for tokio::time::sleep(Duration::from_millis(...)). This ensures the test
waits long enough for server-side session cleanup (disconnect() in
src/client.rs) before invoking send_message, while allowing CI tuning without
modifying source.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
tests/e2e/tests/offline_receipts.rs (1)
293-314:⚠️ Potential issue | 🟠 MajorThis still doesn't prove coalescing picked the latest update.
The first wait accepts any
Event::Presence, so the test can pass on the wrong sender or on the stale unavailable transition. Tighten the assertion to A's presence and the final available state before relying on the “no second event” check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/offline_receipts.rs` around lines 293 - 314, The test currently accepts any Event::Presence; tighten it by asserting the received presence_event is from user A and reflects the final Available state before checking for no second event: when matching Event::Presence(presence) (from the client_b.wait_for_event call stored in presence_event), assert presence.sender == expected_a_id and assert presence.status/state == the Available variant (e.g., PresenceState::Available), then proceed to call the second wait_for_event and assert it times out as before; use the same identifiers presence_event, client_b.wait_for_event, Event::Presence, and second to locate and update the checks.tests/e2e/tests/offline_messages.rs (1)
34-37:⚠️ Potential issue | 🟠 MajorThese sleeps still leave the offline boundary racy.
A fixed 100 ms delay doesn't guarantee that B is offline yet or that the server has started queueing, so these message assertions remain timing-dependent under CI load. Please wait on explicit disconnected/reconnected state in the harness instead of sleeping.
Also applies to: 88-90, 189-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/offline_messages.rs` around lines 34 - 37, Replace the fixed tokio::time::sleep delays after calling client_b.client.reconnect() with explicit waits that observe the client's connection state from the test harness (e.g., wait until client_b reports disconnected before sending offline messages, and wait until it reports reconnected before asserting delivery); update the code paths around the reconnect() calls and the message assertions (the blocks using client_b.client.reconnect() and subsequent sleeps at the shown occurrence and the other two occurrences) to call the harness’ connection-state wait helpers (or add small async helpers like wait_for_disconnected(client_b) / wait_for_reconnected(client_b) that poll the client's is_connected/is_online flag or listen for connection events) so the test no longer relies on hardcoded timeouts.
🧹 Nitpick comments (1)
agent_docs/e2e_testing.md (1)
52-69: Please don't standardize fixed 100 ms offline sleeps here.A fixed delay doesn't prove the server saw the disconnect or that auto-reconnect hasn't already completed, so this guidance will bake the current timing race into new tests. Prefer documenting explicit disconnected/reconnected wait helpers instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent_docs/e2e_testing.md` around lines 52 - 69, Replace the fixed 100ms tokio::time::sleep(Duration::from_millis(100)).await calls after client_b.client.reconnect().await and client_b.disconnect().await with explicit wait helpers that assert connection state; implement and use helpers such as wait_for_disconnected(client_b).await and wait_for_reconnected(client_b).await (or reuse an existing wait_for_event-like helper) that poll or listen for the connection-state event from the client/server, then update the example code to call those helpers instead of sleeping so tests deterministically wait for the server to observe the disconnect/reconnect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent_docs/e2e_testing.md`:
- Around line 9-13: The doc incorrectly states "tests within a file run
sequentially"; update the wording to clarify that Rust's test harness will run
tests within the same integration-test binary concurrently by default (e.g.,
multiple #[tokio::test] functions or test functions when running cargo test -p
e2e-tests), unless overridden with --test-threads=1, so authors must avoid
shared mutable state in same-file tests or explicitly serialize them; change the
paragraph and heading text to recommend splitting tests only for parallelism
clarity and to add a note that tests in the same file can run concurrently and
should be written safe for parallel execution.
In `@tests/e2e/src/lib.rs`:
- Around line 197-205: The current reconnect sequence drains shared broadcast
events via self.event_rx.try_recv(), which can drop important Message/Receipt
events and mishandle TryRecvError::Lagged; instead obtain a fresh
subscription/receiver for the reconnect flow (e.g., call the broadcast channel's
resubscribe() or create a new receiver before reconnect), use that new receiver
to drain only Connected events or to await the Connected event, then call
self.client.reconnect().await and use wait_for_event (or an equivalent helper)
against the new receiver to detect Event::Connected(_) so you do not consume
events from the shared self.event_rx used elsewhere.
In `@tests/e2e/tests/chatstate_ttl.rs`:
- Around line 83-96: The test currently may not exercise offline queue delivery
because client_b.client.reconnect_immediately().await plus
tokio::time::sleep(100ms) can let B come back online before
client_a.client.chatstate().send_composing(&jid_b) is queued; change the
sequence to guarantee B is offline when A sends: ensure you explicitly
stop/disable B's auto-reconnect or call a disconnect method (or wait for an
offline confirmation event from client_b) before invoking
client_a.client.chatstate().send_composing(&jid_b), then trigger
reconnect_immediately() (or re-enable reconnect) and use
client_b.wait_for_event(...) to assert the queued delivery; reference
client_b.client.reconnect_immediately(), client_b.wait_for_event(...),
client_a.client.chatstate().send_composing(&jid_b) when making the change.
- Around line 21-23: The test test_expired_chatstate_not_delivered currently
assumes CHATSTATE_TTL_SECS is < reconnect window; make that explicit by reading
the environment variable CHATSTATE_TTL_SECS at the start of the test
(std::env::var), parse it to an integer, and then either assert it is less than
the reconnect-wait duration used in the test (the ~4s window) or bail with a
clear anyhow::bail/anyhow::Result error explaining the mismatch; update the same
guard for the related assertions around lines 33-54 so the test either derives
its expectation from the parsed TTL or fails fast with a clear message when the
mock-server TTL is too large.
In `@tests/e2e/tests/offline_groups.rs`:
- Around line 50-61: Drain client_c's buffered group-create notification before
taking it offline by awaiting its matching event so the later waiter only sees
the add-member notification; specifically call client_c.wait_for_event(10, |e|
matches!(e, Event::Notification(node) if node.attrs().optional_string("type") ==
Some("w:gp2"))) .await? (same pattern used for client_b) to consume the initial
w:gp2, then proceed to client_c.client.reconnect().await and the sleep.
- Around line 220-272: The test currently buckets events into messages_received
and notifications_received, losing order; change it to collect a single ordered
Vec of received events (push the raw Event from client_c.wait_for_event into
something like received_events) using the same matching predicate
(Event::Message with conversation or Event::Notification where
node.attrs().optional_string("type") == Some("w:gp2")), then assert the ordered
sequence matches message(text_1) -> notification(gp2) -> message(text_2) (i.e.,
check received_events[0] is a Message with conversation == text_1,
received_events[1] is a Notification with type "w:gp2", and received_events[2]
is a Message with conversation == text_2); keep the same timeout/loop and error
handling around client_c.wait_for_event and remove the separate
messages_received/notifications_received assertions.
---
Duplicate comments:
In `@tests/e2e/tests/offline_messages.rs`:
- Around line 34-37: Replace the fixed tokio::time::sleep delays after calling
client_b.client.reconnect() with explicit waits that observe the client's
connection state from the test harness (e.g., wait until client_b reports
disconnected before sending offline messages, and wait until it reports
reconnected before asserting delivery); update the code paths around the
reconnect() calls and the message assertions (the blocks using
client_b.client.reconnect() and subsequent sleeps at the shown occurrence and
the other two occurrences) to call the harness’ connection-state wait helpers
(or add small async helpers like wait_for_disconnected(client_b) /
wait_for_reconnected(client_b) that poll the client's is_connected/is_online
flag or listen for connection events) so the test no longer relies on hardcoded
timeouts.
In `@tests/e2e/tests/offline_receipts.rs`:
- Around line 293-314: The test currently accepts any Event::Presence; tighten
it by asserting the received presence_event is from user A and reflects the
final Available state before checking for no second event: when matching
Event::Presence(presence) (from the client_b.wait_for_event call stored in
presence_event), assert presence.sender == expected_a_id and assert
presence.status/state == the Available variant (e.g., PresenceState::Available),
then proceed to call the second wait_for_event and assert it times out as
before; use the same identifiers presence_event, client_b.wait_for_event,
Event::Presence, and second to locate and update the checks.
---
Nitpick comments:
In `@agent_docs/e2e_testing.md`:
- Around line 52-69: Replace the fixed 100ms
tokio::time::sleep(Duration::from_millis(100)).await calls after
client_b.client.reconnect().await and client_b.disconnect().await with explicit
wait helpers that assert connection state; implement and use helpers such as
wait_for_disconnected(client_b).await and wait_for_reconnected(client_b).await
(or reuse an existing wait_for_event-like helper) that poll or listen for the
connection-state event from the client/server, then update the example code to
call those helpers instead of sleeping so tests deterministically wait for the
server to observe the disconnect/reconnect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b45c0a3f-c103-4647-8600-1a71363066e7
📒 Files selected for processing (9)
.github/workflows/e2e.ymlagent_docs/e2e_testing.mdagent_docs/protocol_architecture.mdsrc/client.rstests/e2e/src/lib.rstests/e2e/tests/chatstate_ttl.rstests/e2e/tests/offline_groups.rstests/e2e/tests/offline_messages.rstests/e2e/tests/offline_receipts.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- agent_docs/protocol_architecture.md
Comment on lines
+9
to
+13
| - Tests across files run **in parallel**; tests within a file run **sequentially**. | ||
|
|
||
| ## File Organization for Parallelism | ||
|
|
||
| Split test files by domain so they run concurrently. Cargo runs each test binary (file) in parallel, but tests within a binary run sequentially. A single large file becomes the bottleneck. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== serialization knobs in repo ==="
rg -n 'RUST_TEST_THREADS|test-threads|serial_test|serial\(' . || true
echo
echo "=== e2e workflow invocation ==="
rg -n 'cargo test -p e2e-tests' .github/workflows tests/e2e || trueRepository: jlucaso1/whatsapp-rust
Length of output: 703
🏁 Script executed:
cat -n agent_docs/e2e_testing.mdRepository: jlucaso1/whatsapp-rust
Length of output: 4888
Lines 9 and 13: The parallelism claim is inaccurate. Tests within the same integration-test binary do NOT run sequentially by default. Rust's test harness runs multiple #[tokio::test] functions concurrently unless forced to --test-threads=1. Since the workflow runs plain cargo test -p e2e-tests with no serialization flag, same-file tests will run in parallel. Update the documentation to reflect that concurrent tests within a file should not assume shared state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent_docs/e2e_testing.md` around lines 9 - 13, The doc incorrectly states
"tests within a file run sequentially"; update the wording to clarify that
Rust's test harness will run tests within the same integration-test binary
concurrently by default (e.g., multiple #[tokio::test] functions or test
functions when running cargo test -p e2e-tests), unless overridden with
--test-threads=1, so authors must avoid shared mutable state in same-file tests
or explicitly serialize them; change the paragraph and heading text to recommend
splitting tests only for parallelism clarity and to add a note that tests in the
same file can run concurrently and should be written safe for parallel
execution.
Comment on lines
+197
to
+205
| // Drain any buffered Connected events from prior connections | ||
| while let Ok(event) = self.event_rx.try_recv() { | ||
| if matches!(event, Event::Connected(_)) { | ||
| continue; | ||
| } | ||
| } | ||
| self.client.reconnect().await; | ||
| self.wait_for_event(10, |e| matches!(e, Event::Connected(_))) | ||
| .await?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'pub struct TestClient|event_rx:|reconnect_and_wait|wait_for_event|try_recv|recv\(\)' tests/e2e/src/lib.rsRepository: jlucaso1/whatsapp-rust
Length of output: 1880
Use a fresh broadcast subscription instead of draining event_rx.
The try_recv() loop consumes every buffered event until hitting a non-Connected event or an error, destroying Message/Receipt events that later assertions via wait_for_event() may still need. The loop also stops on TryRecvError::Lagged, so it fails to reliably drain all stale Connected events despite what the method's doc comment claims. Use resubscribe() or create a dedicated receiver for this reconnect sequence to avoid dropping shared state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/src/lib.rs` around lines 197 - 205, The current reconnect sequence
drains shared broadcast events via self.event_rx.try_recv(), which can drop
important Message/Receipt events and mishandle TryRecvError::Lagged; instead
obtain a fresh subscription/receiver for the reconnect flow (e.g., call the
broadcast channel's resubscribe() or create a new receiver before reconnect),
use that new receiver to drain only Connected events or to await the Connected
event, then call self.client.reconnect().await and use wait_for_event (or an
equivalent helper) against the new receiver to detect Event::Connected(_) so you
do not consume events from the shared self.event_rx used elsewhere.
Comment on lines
+21
to
+23
| /// Requires mock server with CHATSTATE_TTL_SECS=3 (so TTL expires before the ~4s reconnect). | ||
| #[tokio::test] | ||
| async fn test_expired_chatstate_not_delivered() -> anyhow::Result<()> { |
There was a problem hiding this comment.
Make the TTL prerequisite executable, not just documented.
This assertion only holds when CHATSTATE_TTL_SECS is shorter than the reconnect window. Against the mock-server default (30 s), B reconnects before expiry and this becomes a false failure. Read the env in the test and either derive the expectation from it or fail fast with a clear message.
Also applies to: 33-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/chatstate_ttl.rs` around lines 21 - 23, The test
test_expired_chatstate_not_delivered currently assumes CHATSTATE_TTL_SECS is <
reconnect window; make that explicit by reading the environment variable
CHATSTATE_TTL_SECS at the start of the test (std::env::var), parse it to an
integer, and then either assert it is less than the reconnect-wait duration used
in the test (the ~4s window) or bail with a clear anyhow::bail/anyhow::Result
error explaining the mismatch; update the same guard for the related assertions
around lines 33-54 so the test either derives its expectation from the parsed
TTL or fails fast with a clear message when the mock-server TTL is too large.
Comment on lines
+50
to
+61
| // Wait for B to get the create notification (confirms group is set up) | ||
| let _notif_b = client_b | ||
| .wait_for_event(10, |e| { | ||
| matches!(e, Event::Notification(node) if node.attrs().optional_string("type") == Some("w:gp2")) | ||
| }) | ||
| .await?; | ||
| info!("B received group create notification"); | ||
|
|
||
| // Step 2: C goes offline via reconnect() | ||
| client_c.client.reconnect().await; | ||
| info!("C disconnected (will auto-reconnect)"); | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; |
There was a problem hiding this comment.
Drain C's initial w:gp2 before taking it offline.
client_c never consumes the group-create notification, so the waiter at Line 85 can succeed on that already-buffered event instead of the later add-member notification. As written, this only proves that C eventually saw some w:gp2.
Also applies to: 84-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/offline_groups.rs` around lines 50 - 61, Drain client_c's
buffered group-create notification before taking it offline by awaiting its
matching event so the later waiter only sees the add-member notification;
specifically call client_c.wait_for_event(10, |e| matches!(e,
Event::Notification(node) if node.attrs().optional_string("type") ==
Some("w:gp2"))) .await? (same pattern used for client_b) to consume the initial
w:gp2, then proceed to client_c.client.reconnect().await and the sleep.
Comment on lines
+220
to
+272
| // Step 4: C reconnects and should receive events | ||
| // We collect all events C receives — should include both messages and notification | ||
| let mut messages_received = Vec::new(); | ||
| let mut notifications_received = 0; | ||
|
|
||
| // Collect events for up to 30s — we expect at least 2 messages and 1 notification | ||
| for _ in 0..5 { | ||
| let result = client_c | ||
| .wait_for_event(10, |e| { | ||
| matches!(e, Event::Message(msg, _) if msg.conversation.is_some()) | ||
| || matches!(e, Event::Notification(node) if node.attrs().optional_string("type") == Some("w:gp2")) | ||
| }) | ||
| .await; | ||
|
|
||
| match result { | ||
| Ok(Event::Message(msg, _)) => { | ||
| let text = msg.conversation.unwrap_or_default(); | ||
| info!("C received message: {text}"); | ||
| messages_received.push(text); | ||
| } | ||
| Ok(Event::Notification(_)) => { | ||
| info!("C received group notification"); | ||
| notifications_received += 1; | ||
| } | ||
| Ok(_) => {} | ||
| Err(_) => break, // timeout — no more events | ||
| } | ||
| } | ||
|
|
||
| info!( | ||
| "C received {} messages and {} notifications", | ||
| messages_received.len(), | ||
| notifications_received | ||
| ); | ||
|
|
||
| // Verify both messages arrived | ||
| assert!( | ||
| messages_received.iter().any(|m| m == text_1), | ||
| "C should receive first message. Got: {:?}", | ||
| messages_received | ||
| ); | ||
| assert!( | ||
| messages_received.iter().any(|m| m == text_2), | ||
| "C should receive second message. Got: {:?}", | ||
| messages_received | ||
| ); | ||
|
|
||
| // Verify at least one group notification (the add) | ||
| assert!( | ||
| notifications_received >= 1, | ||
| "C should receive at least one group notification, got {}", | ||
| notifications_received | ||
| ); |
There was a problem hiding this comment.
The ordering test never checks mixed-event order.
You split messages and notifications into separate buckets and only assert presence. A reordered stream like [second message, gp2, first message] still passes. Record a single ordered event sequence and compare it to the expected message -> notification -> message pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/offline_groups.rs` around lines 220 - 272, The test currently
buckets events into messages_received and notifications_received, losing order;
change it to collect a single ordered Vec of received events (push the raw Event
from client_c.wait_for_event into something like received_events) using the same
matching predicate (Event::Message with conversation or Event::Notification
where node.attrs().optional_string("type") == Some("w:gp2")), then assert the
ordered sequence matches message(text_1) -> notification(gp2) -> message(text_2)
(i.e., check received_events[0] is a Message with conversation == text_1,
received_events[1] is a Notification with type "w:gp2", and received_events[2]
is a Message with conversation == text_2); keep the same timeout/loop and error
handling around client_c.wait_for_event and remove the separate
messages_received/notifications_received assertions.
jlucaso1
force-pushed
the
tests-split-to-improve-paralelism
branch
from
March 11, 2026 18:26
fbae23d to
37374d5
Compare
This was referenced Mar 22, 2026
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.
Summary by CodeRabbit
Documentation
New Features
Tests