Add e2e signalling tests#1234
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if let Ok(Some(event)) = timeout(window, events.recv()).await { | ||
| match event { | ||
| SignalEvent::Close(reason) => { | ||
| panic!("connection closed while it should stay alive: {reason}") | ||
| } | ||
| SignalEvent::Message(_) => { /* server-initiated messages are fine */ } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Keep-alive test exits early on the first server reply instead of waiting the full timeout window
The stay-connected assertion exits after receiving the first pong reply (if let Ok(Some(event)) at livekit-api/src/signal_client/signal_test.rs:179) instead of looping for the full timeout window, so the test always passes even if the connection dies right after the first pong.
Impact: The test provides false assurance that keepalive works — a broken implementation would still pass.
Mechanism: signal_task forwards PongResp as a Message event, which the single-shot recv matches immediately
The signal_task (livekit-api/src/signal_client/mod.rs:639-706) forwards every server message — including PongResp — as SignalEvent::Message to the events channel (line 675). In "happy" mode the mock server responds to pings with pongs, so the first event the test receives is a PongResp wrapped in Message. The if let Ok(Some(event)) at line 179 matches this single event, the Message(_) arm does nothing, and the test falls through to client.close() — typically within the first ping interval (~2-5 s), well before the full ping_timeout + 2 s window expires.
To actually verify the connection stays alive, the test should loop over events for the entire window duration, panicking only if a Close event arrives:
let deadline = tokio::time::Instant::now() + window;
loop {
match timeout_at(deadline, events.recv()).await {
Err(_) => break, // full window elapsed, no Close — success
Ok(Some(SignalEvent::Close(reason))) => {
panic!("connection closed while it should stay alive: {reason}");
}
Ok(Some(SignalEvent::Message(_))) => { /* keep draining */ }
Ok(None) => panic!("event stream closed unexpectedly"),
}
}| if let Ok(Some(event)) = timeout(window, events.recv()).await { | |
| match event { | |
| SignalEvent::Close(reason) => { | |
| panic!("connection closed while it should stay alive: {reason}") | |
| } | |
| SignalEvent::Message(_) => { /* server-initiated messages are fine */ } | |
| } | |
| } | |
| let deadline = tokio::time::Instant::now() + window; | |
| loop { | |
| match timeout(deadline.saturating_duration_since(tokio::time::Instant::now()), events.recv()).await { | |
| Err(_) => break, // full window elapsed with no Close — success | |
| Ok(Some(SignalEvent::Close(reason))) => { | |
| panic!("connection closed while it should stay alive: {reason}") | |
| } | |
| Ok(Some(SignalEvent::Message(_))) => { /* server-initiated messages are fine; keep draining */ } | |
| Ok(None) => panic!("signal event stream closed unexpectedly"), | |
| } | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
lukasIO
added
the
internal
1 participant
Before you submit your PR
Make sure the following is true before submitting your PR:
PR description
Describe the changes in this PR. Explain what the PR is meant to solve and how to reproduce the issue in the first place.
Breaking changes
If this PR introduces breaking changes, list them here and document the rationale for introducing such a change.
MSRV
If the PR modifies the crate's MSRV (Minimum Supported Rust Version), document it here.
Testing
Ideally, unit test the code you add, but ensure you're not repeating existing test cases. Use as many already written scaffolding, utilities as possible; write your own, when needed. If external services, APIs, tokens are required (e.g., running an LK server instance), provide the necessary information. Make sure your tests perform useful, context-aware assertions and do not simply emulate "happy paths".
Async
We want the project to be runtime-agnostic, so please reuse what's already in livekit-runtime and feel free to add anything missing. It's ok to use Tokio directly, when writing unit tests, if necessary. When testing, do not use artificial delays for the state to "catch up"; instead, respect the event flow and subscribe properly using channels or other mechanisms.