test: add tests e2e with bartender - #303
Merged
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds end-to-end test infrastructure and GitHub Actions workflow, introduces a PresenceHandler that parses and dispatches presence stanzas, removes newsletter types and the rand_core dependency, and adjusts workspace manifests and build/test CI to exclude e2e-tests from standard runs. Changes
Sequence Diagram(s)sequenceDiagram
participant Stanza as "Stanza (incoming <presence>)"
participant PresenceHandler as "PresenceHandler"
participant EventBus as "Event Bus"
participant Legacy as "Legacy ChatState Callbacks"
Stanza->>PresenceHandler: handle(stanza)
PresenceHandler->>PresenceHandler: parse from, type, last
PresenceHandler->>EventBus: dispatch PresenceUpdate(from, unavailable, last_seen)
PresenceHandler->>Legacy: create ChatStateEvent and call callbacks
EventBus-->>Legacy: (subscribers receive PresenceUpdate)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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
jlucaso1
force-pushed
the
feat-e2e-test-bartender
branch
from
March 8, 2026 00:15
e313324 to
6737e72
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cargo.toml (1)
12-18:⚠️ Potential issue | 🟠 MajorDon't make workspace-wide test runs depend on bartender.
Adding
tests/e2eas a normal workspace member meanscargo test --allnow pulls in tests that default towss://127.0.0.1:8080/ws/chatand wait up to 30 seconds for pairing/connection intests/e2e/src/lib.rs. On machines or CI jobs without the mock server, the standard workspace test command will now hang/fail. Please keep these tests opt-in (for example via ignored tests/env gating or a separate manifest invocation) instead of making them part of the default workspace test surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 12 - 18, Remove the end-to-end tests from the default workspace test surface: delete "tests/e2e" from the workspace members list in Cargo.toml (the workspace members array) and instead make the e2e tests opt-in by either adding #[ignore] or an environment-gated skip in tests/e2e/src/lib.rs (for example check a CI env var like RUN_E2E and early-return/skip when not set), or run them via a separate manifest invocation; ensure references to the e2e crate name ("tests/e2e") are removed from the members list so cargo test --all no longer executes those network-dependent 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 @.github/workflows/e2e.yml:
- Around line 39-43: The "Wait for mock server" step's readiness loop currently
always succeeds (exits 0) even if curl never succeeds; update the shell snippet
that runs the for loop with curl -k https://localhost:8080/ so that if the loop
completes without a successful probe it exits with a non-zero status (e.g., test
the loop result and call exit 1 or append a failing command), ensuring the
GitHub Actions step fails when the mock server never became ready.
In `@tests/e2e/src/lib.rs`:
- Around line 53-96: The connect helper currently leaves the background bot task
running when the wait loop times out or event_rx.recv() errors; ensure you clean
up by aborting the run_handle and disconnecting the client before returning or
panicking. In connect, on the timeout error path (the map_err result) and inside
the event_rx Err branch, call run_handle.abort() (or stop/await it if the handle
type requires) and call client.disconnect() (or await if async) to tear down the
bot before returning the error or panicking; reference symbols: connect,
bot.run(), run_handle, client, and event_rx.
In `@tests/e2e/tests/connection.rs`:
- Around line 47-70: The event loop inside the tokio::time::timeout currently
only breaks in the Event::Connected arm, so if Event::Connected arrives before
Event::PairSuccess the loop never exits; update the handlers for
Event::PairSuccess and Event::Connected (the match on event_rx.recv() inside the
async loop) to set their respective flags and then check if both
got_pair_success and got_connected are true and break when they are, ensuring
either arm can trigger the exit when the other flag is already set.
In `@tests/e2e/tests/messaging.rs`:
- Around line 100-105: The test currently unwraps the awaited event with a
conditional matching Event::Message and asserts msg.conversation but omits an
else branch, allowing non-Message events to silently pass; update the block that
handles the result of client_b.wait_for_event (the match on Event::Message) to
include an else arm that fails the test (e.g., panic! or assert!(false) with a
clear message) when the event is not Event::Message so the test fails when an
unexpected event type is returned; reference the wait_for_event call, the
Event::Message pattern, msg.conversation and text_a when adding the failing
branch.
- Around line 120-125: The match arm for client_a.wait_for_event(...) only
handles Event::Message but lacks an else branch causing silent test false
positives; update the block that awaits client_a.wait_for_event(30, |e|
matches!(e, Event::Message(_, _))).await? to include an else branch (e.g.,
panic! or assert!(false, ...)) when the event is not Event::Message and include
the received event in the failure message, then assert
msg.conversation.as_deref() == Some(text_b) as before; apply the identical
change to the symmetric B→A wait_for_event block so both directions fail loudly
with diagnostics when the pattern doesn't match.
- Around line 14-19: The variable jid_a is computed but never used; remove the
unused binding by either omitting the assignment (await
client_a.client.get_pn().await... .to_non_ad(); if side effects needed) or
change the binding to a discard (e.g., _jid_a) so the call remains but the
compiler won't warn; update the code around the expression using
client_a.client.get_pn() and to_non_ad() accordingly to avoid the
unused-variable warning.
---
Outside diff comments:
In `@Cargo.toml`:
- Around line 12-18: Remove the end-to-end tests from the default workspace test
surface: delete "tests/e2e" from the workspace members list in Cargo.toml (the
workspace members array) and instead make the e2e tests opt-in by either adding
#[ignore] or an environment-gated skip in tests/e2e/src/lib.rs (for example
check a CI env var like RUN_E2E and early-return/skip when not set), or run them
via a separate manifest invocation; ensure references to the e2e crate name
("tests/e2e") are removed from the members list so cargo test --all no longer
executes those network-dependent tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59831df5-cf17-4e29-a35f-8c0d6561010d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.github/workflows/e2e.ymlCargo.tomlsrc/client.rssrc/handlers/mod.rssrc/handlers/presence.rstests/e2e/Cargo.tomltests/e2e/src/lib.rstests/e2e/tests/connection.rstests/e2e/tests/messaging.rstests/e2e/tests/presence.rswacore/Cargo.tomlwacore/binary/Cargo.tomlwacore/noise/Cargo.tomlwacore/src/types/newsletter.rs
💤 Files with no reviewable changes (3)
- wacore/noise/Cargo.toml
- wacore/Cargo.toml
- wacore/src/types/newsletter.rs
Comment on lines
+47
to
+70
| let result = tokio::time::timeout(timeout, async { | ||
| loop { | ||
| match event_rx.recv().await { | ||
| Ok(Event::PairSuccess(ps)) => { | ||
| info!("Received PairSuccess: {:?}", ps); | ||
| got_pair_success = true; | ||
| } | ||
| Ok(Event::Connected(_)) => { | ||
| info!("Received Connected event"); | ||
| got_connected = true; | ||
| if got_pair_success { | ||
| break; | ||
| } | ||
| } | ||
| Ok(event) => { | ||
| info!("Received event: {:?}", event); | ||
| } | ||
| Err(e) => { | ||
| panic!("Event channel error: {}", e); | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| .await; |
There was a problem hiding this comment.
Event loop may timeout if Connected arrives before PairSuccess.
The loop only checks for break in the Connected arm. If events arrive in order Connected → PairSuccess, the loop sets both flags but never breaks—it will timeout waiting for another event.
🐛 Proposed fix
Ok(Event::PairSuccess(ps)) => {
info!("Received PairSuccess: {:?}", ps);
got_pair_success = true;
+ if got_connected {
+ break;
+ }
}
Ok(Event::Connected(_)) => {
info!("Received Connected event");
got_connected = true;
if got_pair_success {
break;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/connection.rs` around lines 47 - 70, The event loop inside
the tokio::time::timeout currently only breaks in the Event::Connected arm, so
if Event::Connected arrives before Event::PairSuccess the loop never exits;
update the handlers for Event::PairSuccess and Event::Connected (the match on
event_rx.recv() inside the async loop) to set their respective flags and then
check if both got_pair_success and got_connected are true and break when they
are, ensuring either arm can trigger the exit when the other flag is already
set.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/tests/messaging.rs (1)
14-19: Consider removing or prefixingjid_aif only used for logging.The variable
jid_ais computed but only used in the log statement at line 27. If the logging isn't essential for debugging, consider removing this binding or prefixing with_to indicate it's intentionally unused for the core test logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/messaging.rs` around lines 14 - 19, The binding jid_a is only used for logging and should be made clearly optional: either remove the jid_a variable entirely and log nothing, or prefix it with an underscore (e.g., _jid_a) to mark it as intentionally unused while keeping the log; update the log call to use the new name if keeping it (or remove the log statement if dropping the binding). Locate the code where client_a.client.get_pn().await.expect(...).to_non_ad() is assigned to jid_a and apply one of these two changes to eliminate the unused-binding warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/tests/messaging.rs`:
- Around line 186-193: The test currently only checks the revoke protocol
message inside an if let Event::Message(msg, _) = event { ... } block and lacks
an else branch to explicitly fail when the event is not Event::Message; add an
else branch to the if let that calls panic! or assert!(false, ...) with a clear
message (e.g., "Expected Event::Message with revoke protocol message, got
{event:?}") so the test fails loudly when the pattern doesn't match; locate the
block using Event::Message, proto, and the assert_eq comparing proto.r#type() to
wa::message::protocol_message::Type::Revoke and add the explicit else branch
there.
---
Nitpick comments:
In `@tests/e2e/tests/messaging.rs`:
- Around line 14-19: The binding jid_a is only used for logging and should be
made clearly optional: either remove the jid_a variable entirely and log
nothing, or prefix it with an underscore (e.g., _jid_a) to mark it as
intentionally unused while keeping the log; update the log call to use the new
name if keeping it (or remove the log statement if dropping the binding). Locate
the code where client_a.client.get_pn().await.expect(...).to_non_ad() is
assigned to jid_a and apply one of these two changes to eliminate the
unused-binding warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f0ab3b4-af88-47cc-ad47-abb001a97d77
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/e2e.yml.github/workflows/main.ymlCargo.tomlsrc/client.rssrc/handlers/mod.rssrc/handlers/presence.rstests/e2e/Cargo.tomltests/e2e/src/lib.rstests/e2e/tests/connection.rstests/e2e/tests/messaging.rstests/e2e/tests/presence.rswacore/Cargo.tomlwacore/binary/Cargo.tomlwacore/noise/Cargo.tomlwacore/src/types/newsletter.rs
💤 Files with no reviewable changes (3)
- wacore/Cargo.toml
- wacore/noise/Cargo.toml
- wacore/src/types/newsletter.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/handlers/mod.rs
- src/handlers/presence.rs
- Cargo.toml
This was referenced Mar 10, 2026
Merged
2 tasks
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
New Features
Tests
Chores