Add browser interoperability test suite#175
Open
ai-and-i wants to merge 18 commits intomoq-dev:mainfrom
Open
Conversation
ai-and-i
force-pushed
the
fix/browser-tests
branch
from
10b30fa to
7b621fb
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new workspace crate 🚥 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![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
🧹 Nitpick comments (7)
justfile (1)
57-60: Consider runningbrowser-testin at least one CI lane.Since the crate is excluded from default checks, adding a scheduled or dedicated CI job for this recipe will help prevent drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@justfile` around lines 57 - 60, Add a CI job that runs the just recipe "browser-test" (the recipe name and its command `cargo test -p web-transport-browser-tests`) in at least one pipeline lane or a scheduled workflow to prevent drift; update your CI YAML to invoke `just browser-test` (or install/run just and Chromium) in that lane and mark it as required or scheduled so the excluded crate is exercised regularly.web-transport-browser-tests/tests/smoke.rs (1)
16-16: Use shared timeout constants for consistency.At Line 16, consider using
tests/common/mod.rs::TIMEOUTinstead of a hardcoded value to keep timeout tuning centralized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-transport-browser-tests/tests/smoke.rs` at line 16, Replace the hardcoded Duration::from_secs(10) in the smoke test with the shared timeout constant tests::common::TIMEOUT: locate the occurrence of Duration::from_secs(10) in tests/smoke.rs and import or reference the central constant (tests::common::TIMEOUT) instead so the test uses the centralized timeout value for consistency with other tests.web-transport-browser-tests/src/server.rs (1)
61-69: Abort outstanding handler tasks duringDropcleanup.Currently
Droponly aborts the main listener task. Long-lived handler tasks can survive early-failure paths and continue running.Proposed change
impl Drop for TestServer { fn drop(&mut self) { if let Some(tx) = self.shutdown_tx.take() { let _ = tx.send(()); } if let Some(task) = self.task.take() { task.abort(); } + if let Ok(mut handles) = self.handler_tasks.lock() { + for handle in handles.drain(..) { + handle.abort(); + } + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-transport-browser-tests/src/server.rs` around lines 61 - 69, The Drop impl for TestServer only aborts the main listener (self.task) and misses long-lived per-connection handler tasks; update TestServer to track those handler JoinHandles (e.g., a Vec<JoinHandle> or similar field like handler_tasks) where handlers are spawned, push each handler handle there, and in impl Drop for TestServer iterate over those handles, calling abort() (and optionally take() them first) so all outstanding handler tasks are aborted during cleanup alongside shutdown_tx and self.task; also ensure any places that spawn handlers add the handle to this new field.web-transport-browser-tests/src/cert.rs (1)
27-29: Backdate certificatenot_beforeslightly to reduce timing flakes.At Line 28, setting validity to exact current time can intermittently fail as “not yet valid” around clock/processing boundaries. A small skew margin is safer for CI.
Proposed change
- params.not_before = now; + params.not_before = now - time::Duration::minutes(1); params.not_after = now + time::Duration::days(10);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-transport-browser-tests/src/cert.rs` around lines 27 - 29, Set params.not_before slightly earlier than the exact current time to avoid "not yet valid" race conditions: when computing now via time::OffsetDateTime::now_utc(), subtract a small skew (e.g., a few seconds or a minute using time::Duration::seconds or ::minutes) before assigning to params.not_before; keep params.not_after computed relative to the original now (or now + skew) as appropriate so the certificate lifetime remains correct. Ensure the change updates the initialization that uses time::OffsetDateTime::now_utc() and assigns params.not_before and params.not_after.web-transport-browser-tests/src/browser.rs (1)
100-119: Watchdog shell command may fail silently on non-Unix platforms.The cleanup watchdog spawns a
shshell process withkill -9which is Unix-specific. On Windows or other platforms withoutsh, this will silently fail (returningNone), leaving Chrome processes orphaned on abnormal exit.Consider documenting this limitation or adding platform-specific handling:
fn spawn_cleanup_watchdog( chrome_pid: u32, data_dir: &std::path::Path, ) -> Option<std::process::ChildStdin> { + #[cfg(not(unix))] + { + tracing::warn!("cleanup watchdog not supported on this platform"); + return None; + } + + #[cfg(unix)] use std::process::{Command, Stdio};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-transport-browser-tests/src/browser.rs` around lines 100 - 119, The watchdog uses a Unix-only "sh" + "kill -9" command in spawn_cleanup_watchdog which fails on Windows; update spawn_cleanup_watchdog to perform platform-specific handling: on Unix keep the existing shell command using chrome_pid and data_dir, and on Windows use "cmd" /C with "taskkill /PID <chrome_pid> /F" (or call the WinAPI) and equivalent removal of data_dir, or implement the cleanup in Rust (spawn a thread that waits for stdin then kills the process by pid and removes data_dir) so the logic works cross-platform and does not silently return None.web-transport-browser-tests/tests/bidi_stream.rs (2)
298-300: Guardreader.read()before dereferencingstream.At Line 298,
doneis not checked. If no stream arrives, this becomes a JS TypeError instead of a clear test failure message.Suggested patch
- const { value: stream } = await reader.read(); + const { value: stream, done } = await reader.read(); + if (done) return { success: false, message: "no incoming stream" };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-transport-browser-tests/tests/bidi_stream.rs` around lines 298 - 300, The code dereferences `stream` after `const { value: stream } = await reader.read()` without checking `done`; change the read call to capture `done` (e.g., `const { done, value: stream } = await reader.read()`), then guard it (throw or assert if `done` is true with a clear test-failure message) before using `stream.readable.getReader()` (the `sr` creation) so a missing stream yields a clear test failure instead of a TypeError.
12-47: Extract a shared helper for the repeated harness lifecycle.The setup/run/teardown/unwrap/assert flow repeats across most tests (e.g., Lines 15–47, 52–91, 96–123, etc.). A local helper would reduce duplication and keep failure handling consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-transport-browser-tests/tests/bidi_stream.rs` around lines 12 - 47, The test bidi_stream_echo duplicates harness lifecycle code (setup, run_js, teardown, unwrap/assert); extract a shared helper (e.g., a async fn run_harness_test<F, T>(handler: HandlerType, js: &str, timeout: Duration) -> TestResult) and replace the repeated sequence in bidi_stream_echo with a call to that helper; the helper should call harness::setup(handler).await.unwrap(), invoke harness.run_js(js, timeout).await, call harness.teardown().await in a finally/ensure-style path, unwrap the result and return it so callers like bidi_stream_echo can simply assert on the returned TestResult. Use the existing symbols harness::setup, run_js, teardown, and bidi_stream_echo to locate and refactor the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-transport-browser-tests/src/server.rs`:
- Around line 38-40: The shutdown method currently swallows the accept-loop
JoinError by awaiting the task with let _ = self.task.take().await; instead
handle the JoinError from the spawned accept-loop task (the task stored in
self.task) instead of dropping it: await the JoinHandle and propagate or surface
the error (e.g., return a Result from shutdown or panic/log with context) so
panics inside the accept loop are not masked; update shutdown's signature/return
path as needed and match on the JoinError from task.await (or call expect/unwrap
with a clear message) to make failures visible.
In `@web-transport-browser-tests/tests/bidi_stream.rs`:
- Around line 501-503: The test currently calls messages.sort() which destroys
arrival order so the assertion only checks set membership; remove the sort()
call and instead assert that messages (the vector/array built in the test)
equals the expected ordered sequence ["prio0","prio1","prio2"] exactly (e.g.,
compare messages directly to expected or assert JSON string equality without
sorting) so the test verifies priority ordering; update the code that references
messages.sort(), messages, and expected accordingly.
---
Nitpick comments:
In `@justfile`:
- Around line 57-60: Add a CI job that runs the just recipe "browser-test" (the
recipe name and its command `cargo test -p web-transport-browser-tests`) in at
least one pipeline lane or a scheduled workflow to prevent drift; update your CI
YAML to invoke `just browser-test` (or install/run just and Chromium) in that
lane and mark it as required or scheduled so the excluded crate is exercised
regularly.
In `@web-transport-browser-tests/src/browser.rs`:
- Around line 100-119: The watchdog uses a Unix-only "sh" + "kill -9" command in
spawn_cleanup_watchdog which fails on Windows; update spawn_cleanup_watchdog to
perform platform-specific handling: on Unix keep the existing shell command
using chrome_pid and data_dir, and on Windows use "cmd" /C with "taskkill /PID
<chrome_pid> /F" (or call the WinAPI) and equivalent removal of data_dir, or
implement the cleanup in Rust (spawn a thread that waits for stdin then kills
the process by pid and removes data_dir) so the logic works cross-platform and
does not silently return None.
In `@web-transport-browser-tests/src/cert.rs`:
- Around line 27-29: Set params.not_before slightly earlier than the exact
current time to avoid "not yet valid" race conditions: when computing now via
time::OffsetDateTime::now_utc(), subtract a small skew (e.g., a few seconds or a
minute using time::Duration::seconds or ::minutes) before assigning to
params.not_before; keep params.not_after computed relative to the original now
(or now + skew) as appropriate so the certificate lifetime remains correct.
Ensure the change updates the initialization that uses
time::OffsetDateTime::now_utc() and assigns params.not_before and
params.not_after.
In `@web-transport-browser-tests/src/server.rs`:
- Around line 61-69: The Drop impl for TestServer only aborts the main listener
(self.task) and misses long-lived per-connection handler tasks; update
TestServer to track those handler JoinHandles (e.g., a Vec<JoinHandle> or
similar field like handler_tasks) where handlers are spawned, push each handler
handle there, and in impl Drop for TestServer iterate over those handles,
calling abort() (and optionally take() them first) so all outstanding handler
tasks are aborted during cleanup alongside shutdown_tx and self.task; also
ensure any places that spawn handlers add the handle to this new field.
In `@web-transport-browser-tests/tests/bidi_stream.rs`:
- Around line 298-300: The code dereferences `stream` after `const { value:
stream } = await reader.read()` without checking `done`; change the read call to
capture `done` (e.g., `const { done, value: stream } = await reader.read()`),
then guard it (throw or assert if `done` is true with a clear test-failure
message) before using `stream.readable.getReader()` (the `sr` creation) so a
missing stream yields a clear test failure instead of a TypeError.
- Around line 12-47: The test bidi_stream_echo duplicates harness lifecycle code
(setup, run_js, teardown, unwrap/assert); extract a shared helper (e.g., a async
fn run_harness_test<F, T>(handler: HandlerType, js: &str, timeout: Duration) ->
TestResult) and replace the repeated sequence in bidi_stream_echo with a call to
that helper; the helper should call harness::setup(handler).await.unwrap(),
invoke harness.run_js(js, timeout).await, call harness.teardown().await in a
finally/ensure-style path, unwrap the result and return it so callers like
bidi_stream_echo can simply assert on the returned TestResult. Use the existing
symbols harness::setup, run_js, teardown, and bidi_stream_echo to locate and
refactor the code.
In `@web-transport-browser-tests/tests/smoke.rs`:
- Line 16: Replace the hardcoded Duration::from_secs(10) in the smoke test with
the shared timeout constant tests::common::TIMEOUT: locate the occurrence of
Duration::from_secs(10) in tests/smoke.rs and import or reference the central
constant (tests::common::TIMEOUT) instead so the test uses the centralized
timeout value for consistency with other tests.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
Cargo.tomljustfileweb-transport-browser-tests/Cargo.tomlweb-transport-browser-tests/src/browser.rsweb-transport-browser-tests/src/cert.rsweb-transport-browser-tests/src/harness.rsweb-transport-browser-tests/src/js.rsweb-transport-browser-tests/src/lib.rsweb-transport-browser-tests/src/server.rsweb-transport-browser-tests/tests/bidi_stream.rsweb-transport-browser-tests/tests/common/mod.rsweb-transport-browser-tests/tests/concurrent.rsweb-transport-browser-tests/tests/connection.rsweb-transport-browser-tests/tests/datagram.rsweb-transport-browser-tests/tests/smoke.rsweb-transport-browser-tests/tests/stream_error.rsweb-transport-browser-tests/tests/uni_stream.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
web-transport-browser-tests/tests/bidi_stream.rs (1)
501-503:⚠️ Potential issue | 🟠 MajorRemove sorting so the priority test actually checks ordering.
At Line 501, sorting
messagesdestroys arrival order, so this assertion won’t catch priority regressions.Suggested fix
- messages.sort(); const expected = ["prio0", "prio1", "prio2"]; const ok = JSON.stringify(messages) === JSON.stringify(expected);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-transport-browser-tests/tests/bidi_stream.rs` around lines 501 - 503, The test currently calls messages.sort() which destroys arrival order and prevents the priority behavior from being validated; remove the call to messages.sort() so the subsequent comparison against expected (const expected = ["prio0","prio1","prio2"]; and the ok check JSON.stringify(messages) === JSON.stringify(expected)) verifies actual arrival ordering, leaving the rest of the test (messages array, expected, and ok assertion) unchanged.web-transport-browser-tests/src/server.rs (1)
38-40:⚠️ Potential issue | 🟠 MajorDo not swallow accept-loop task failures in shutdown.
At Line 39,
let _ = task.await;dropsJoinError, which can mask panic/failure in the accept loop and produce misleading test outcomes.Suggested fix
- if let Some(task) = self.task.take() { - let _ = task.await; - } + if let Some(task) = self.task.take() { + match task.await { + Ok(()) => {} + Err(e) if e.is_panic() => std::panic::resume_unwind(e.into_panic()), + Err(e) => panic!("server accept task failed: {e}"), + } + }#!/bin/bash # Verify the current JoinError-swallowing pattern in shutdown. # Expected: one match in web-transport-browser-tests/src/server.rs around shutdown(). rg -n 'let _ = task\.await;' web-transport-browser-tests/src/server.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-transport-browser-tests/src/server.rs` around lines 38 - 40, The shutdown code currently swallows the accept-loop task's JoinError by doing `let _ = task.await;`; update the shutdown logic (the block where `self.task.take()` is awaited) to inspect the `JoinHandle` result instead of ignoring it—e.g., match on `task.await` and either propagate the error from the shutdown method (return a Result) or at minimum log/raise a clear failure (include the JoinError) so panics in the accept loop are not silently dropped; change the signature of the shutdown method if necessary to return Result to propagate the JoinError from the accept-loop task.
🧹 Nitpick comments (2)
web-transport-browser-tests/src/browser.rs (1)
77-81: Consider a more graceful fallback whenHOMEis not set.Using
expect("HOME not set")will panic in environments whereHOMEisn't defined (some CI containers, minimal Docker images). A softer fallback to a temp directory would improve robustness.♻️ Suggested fallback
let base = std::env::var("XDG_CACHE_HOME") .map(PathBuf::from) .unwrap_or_else(|_| { - PathBuf::from(std::env::var("HOME").expect("HOME not set")).join(".cache") + std::env::var("HOME") + .map(|h| PathBuf::from(h).join(".cache")) + .unwrap_or_else(|_| std::env::temp_dir().join("chromiumoxide-cache")) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-transport-browser-tests/src/browser.rs` around lines 77 - 81, The current construction that builds base uses expect("HOME not set") and will panic if HOME is missing; change the fallback in the unwrap_or_else closure to avoid panicking by using std::env::var_os("HOME") (or std::env::temp_dir()) and fall back to std::env::temp_dir() when HOME is absent, then join(".cache") as needed so base is always a valid PathBuf; update the closure that constructs base (the code around the base variable and the unwrap_or_else) to use this softer fallback.web-transport-browser-tests/tests/smoke.rs (1)
1-1: Unused import and hardcoded timeout.The
Durationimport is unused because the timeout is hardcoded on line 16. Consider using theTIMEOUTconstant fromcommonfor consistency with other tests.♻️ Suggested fix
-use std::time::Duration; - use web_transport_browser_tests::harness; + +mod common; +use common::TIMEOUT;And on line 16:
- Duration::from_secs(10), + TIMEOUT,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-transport-browser-tests/tests/smoke.rs` at line 1, Remove the unused Duration import and replace the hardcoded timeout literal on line 16 with the shared TIMEOUT constant from common; specifically delete the use std::time::Duration; line and change the timeout argument to use common::TIMEOUT (or import TIMEOUT) so the test uses the consistent TIMEOUT constant instead of a magic value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-transport-browser-tests/src/browser.rs`:
- Around line 106-112: The code builds a shell command with
Command::new("sh").arg("-c").arg(format!(...)) interpolating data_dir.display(),
risking shell injection; replace this by avoiding shell interpolation: stop
using the single shell Command invocation and instead perform the cleanup with
safe primitives — call kill on chrome_pid via a direct process/signal API (or
spawn Command::new("kill").arg("-9").arg(chrome_pid.to_string())) and remove the
directory using std::fs::remove_dir_all(data_dir) (or spawn
Command::new("rm").arg("-rf").arg(data_dir.as_os_str()) if you must call
external rm), eliminating format! with data_dir.display(); update the code
around the Command::new("sh") block to use these safe alternatives.
In `@web-transport-browser-tests/tests/stream_error.rs`:
- Around line 141-147: The test incorrectly expects wt.closed to reject after
the server calls session.close(0, b""); instead change the assertions in the
blocks that currently try { await wt.closed; throw ... } catch (e) { ... } to
await the resolved value (const info = await wt.closed) and assert the
graceful-close fields (e.g., info.closeCode === 0 and info.reason matches
expected) — update both the first block referencing wt.closed and the similar
block later (currently checking for WebTransportError and e.source ===
"session") to verify the resolved close info rather than expecting a rejection.
---
Duplicate comments:
In `@web-transport-browser-tests/src/server.rs`:
- Around line 38-40: The shutdown code currently swallows the accept-loop task's
JoinError by doing `let _ = task.await;`; update the shutdown logic (the block
where `self.task.take()` is awaited) to inspect the `JoinHandle` result instead
of ignoring it—e.g., match on `task.await` and either propagate the error from
the shutdown method (return a Result) or at minimum log/raise a clear failure
(include the JoinError) so panics in the accept loop are not silently dropped;
change the signature of the shutdown method if necessary to return Result to
propagate the JoinError from the accept-loop task.
In `@web-transport-browser-tests/tests/bidi_stream.rs`:
- Around line 501-503: The test currently calls messages.sort() which destroys
arrival order and prevents the priority behavior from being validated; remove
the call to messages.sort() so the subsequent comparison against expected (const
expected = ["prio0","prio1","prio2"]; and the ok check JSON.stringify(messages)
=== JSON.stringify(expected)) verifies actual arrival ordering, leaving the rest
of the test (messages array, expected, and ok assertion) unchanged.
---
Nitpick comments:
In `@web-transport-browser-tests/src/browser.rs`:
- Around line 77-81: The current construction that builds base uses expect("HOME
not set") and will panic if HOME is missing; change the fallback in the
unwrap_or_else closure to avoid panicking by using std::env::var_os("HOME") (or
std::env::temp_dir()) and fall back to std::env::temp_dir() when HOME is absent,
then join(".cache") as needed so base is always a valid PathBuf; update the
closure that constructs base (the code around the base variable and the
unwrap_or_else) to use this softer fallback.
In `@web-transport-browser-tests/tests/smoke.rs`:
- Line 1: Remove the unused Duration import and replace the hardcoded timeout
literal on line 16 with the shared TIMEOUT constant from common; specifically
delete the use std::time::Duration; line and change the timeout argument to use
common::TIMEOUT (or import TIMEOUT) so the test uses the consistent TIMEOUT
constant instead of a magic value.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
Cargo.tomljustfileweb-transport-browser-tests/Cargo.tomlweb-transport-browser-tests/src/browser.rsweb-transport-browser-tests/src/cert.rsweb-transport-browser-tests/src/harness.rsweb-transport-browser-tests/src/js.rsweb-transport-browser-tests/src/lib.rsweb-transport-browser-tests/src/server.rsweb-transport-browser-tests/tests/bidi_stream.rsweb-transport-browser-tests/tests/common/mod.rsweb-transport-browser-tests/tests/concurrent.rsweb-transport-browser-tests/tests/connection.rsweb-transport-browser-tests/tests/datagram.rsweb-transport-browser-tests/tests/smoke.rsweb-transport-browser-tests/tests/stream_error.rsweb-transport-browser-tests/tests/uni_stream.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- web-transport-browser-tests/src/js.rs
- web-transport-browser-tests/src/cert.rs
- web-transport-browser-tests/tests/concurrent.rs
- justfile
3 tasks
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-transport-browser-tests/src/browser.rs`:
- Around line 160-163: The code currently falls back to PID 0 with let
chrome_pid = browser.get_mut_child().and_then(|c| c.inner.id()).unwrap_or(0);
which causes kill -9 0 to target the whole process group; instead preserve the
Option by removing unwrap_or(0) and propagate an Option<u32> (e.g. let
chrome_pid = browser.get_mut_child().and_then(|c| c.inner.id());), then change
the watchdog cleanup logic that uses chrome_pid to only execute the kill command
when chrome_pid.is_some() (or match Some(pid) => run kill(pid), None => skip
cleanup). Reference chrome_pid, browser.get_mut_child(), and c.inner.id() when
locating the change.
In `@web-transport-browser-tests/src/server.rs`:
- Around line 35-37: The shutdown send currently calls tx.send(()).expect(...),
which can panic if the receiver was dropped and mask the real accept-loop error
from task.await.expect(...); change the send to handle the Result without
panicking (e.g., ignore Err or log it) so failing to notify shutdown does not
abort the test—locate the shutdown_tx.take() / tx.send call in the shutdown path
and replace the expect-based send with a non-panicking result-handling (keeping
task.await.expect for surfacing accept-loop failures).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web-transport-browser-tests/src/browser.rsweb-transport-browser-tests/src/server.rsweb-transport-browser-tests/tests/bidi_stream.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- web-transport-browser-tests/tests/bidi_stream.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web-transport-browser-tests/tests/stream_error.rs (1)
900-905: Replace fixed delay with an explicit stream handshake.This sleep-based synchronization is prone to timing flakes. Prefer waiting for the incoming server-initiated stream and asserting its reset, then closing the session.
♻️ Proposed deterministic JS flow
- // Give the server time to open a stream, reset it, and attempt write - await new Promise(r => setTimeout(r, 500)); - wt.close(); - return { success: true, message: "server tested write after reset" }; + const incoming = wt.incomingBidirectionalStreams.getReader(); + const { value: stream, done } = await incoming.read(); + if (done) { + return { success: false, message: "expected incoming server stream" }; + } + + const reader = stream.readable.getReader(); + try { + await reader.read(); + return { success: false, message: "expected reset on incoming stream" }; + } catch (e) { + if (!(e instanceof WebTransportError) || e.source !== "stream" || e.streamErrorCode !== 42) { + throw e; + } + } + + wt.close(); + await wt.closed; + return { success: true, message: "server tested write after reset" };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-transport-browser-tests/tests/stream_error.rs` around lines 900 - 905, The test uses a fixed delay (setTimeout) after connectWebTransport() which causes flakiness; replace that sleep with an explicit handshake that awaits the server-initiated incoming stream and verifies it was reset before closing the session. Modify the JS block that calls connectWebTransport() (the wt variable) to await the incoming stream from wt.incomingBidirectionalStreams (or wt.incomingUnidirectionalStreams as appropriate), assert the stream's reset/error state (e.g., read/closed/reset result), then call wt.close() and return success; remove the setTimeout-based wait entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-transport-browser-tests/src/browser.rs`:
- Around line 215-237: When create_browser_context in TestContext::new succeeds
but new_page(...) fails, ensure you dispose the created context to avoid leaks:
after obtaining context_id, wrap the new_page call so that on any error you call
the browser context teardown API (e.g., delete_browser_context /
close_browser_context or the appropriate method on shared.browser) using the
same context_id (via shared.browser.lock().await) before returning the error;
update the error path in TestContext::new around create_browser_context/new_page
to perform this cleanup.
In `@web-transport-browser-tests/src/server.rs`:
- Around line 61-69: The Drop impl for TestServer only aborts the accept loop
task and sends shutdown_tx, but it must also abort any spawned per-session
handler tasks to avoid them lingering after panics; update the TestServer struct
to track handler JoinHandles (e.g. a Vec or HashMap of JoinHandle) when you
spawn session handlers, then modify impl Drop for TestServer so that after
taking self.task it also iterates over and aborts each stored handler handle
(and clear/take the collection) similar to how shutdown_tx and task are handled;
ensure you reference the existing fields shutdown_tx, task, and the new handler
collection in the Drop code and abort each JoinHandle to guarantee all spawned
handlers are stopped.
---
Nitpick comments:
In `@web-transport-browser-tests/tests/stream_error.rs`:
- Around line 900-905: The test uses a fixed delay (setTimeout) after
connectWebTransport() which causes flakiness; replace that sleep with an
explicit handshake that awaits the server-initiated incoming stream and verifies
it was reset before closing the session. Modify the JS block that calls
connectWebTransport() (the wt variable) to await the incoming stream from
wt.incomingBidirectionalStreams (or wt.incomingUnidirectionalStreams as
appropriate), assert the stream's reset/error state (e.g., read/closed/reset
result), then call wt.close() and return success; remove the setTimeout-based
wait entirely.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web-transport-browser-tests/src/browser.rsweb-transport-browser-tests/src/server.rsweb-transport-browser-tests/tests/stream_error.rs
Comment on lines
+215
to
+237
| let context_id = shared | ||
| .browser | ||
| .lock() | ||
| .await | ||
| .create_browser_context(CreateBrowserContextParams::default()) | ||
| .await | ||
| .context("failed to create browser context")?; | ||
|
|
||
| // Navigate to a localhost HTTP page so the JS context has a secure | ||
| // origin with the WebTransport API available. | ||
| let page = shared | ||
| .browser | ||
| .lock() | ||
| .await | ||
| .new_page( | ||
| CreateTargetParams::builder() | ||
| .url(&shared.page_url) | ||
| .browser_context_id(context_id.clone()) | ||
| .build() | ||
| .unwrap(), | ||
| ) | ||
| .await | ||
| .context("failed to create page")?; |
There was a problem hiding this comment.
Dispose the created browser context when page creation fails.
TestContext::new can leak an isolated context if new_page(...) errors after create_browser_context(...) succeeds. This can accumulate stale contexts and make later tests noisier.
🔧 Proposed fix
- let page = shared
- .browser
- .lock()
- .await
- .new_page(
- CreateTargetParams::builder()
- .url(&shared.page_url)
- .browser_context_id(context_id.clone())
- .build()
- .unwrap(),
- )
- .await
- .context("failed to create page")?;
+ let page = match shared
+ .browser
+ .lock()
+ .await
+ .new_page(
+ CreateTargetParams::builder()
+ .url(&shared.page_url)
+ .browser_context_id(context_id.clone())
+ .build()
+ .unwrap(),
+ )
+ .await
+ {
+ Ok(page) => page,
+ Err(err) => {
+ let _ = shared
+ .browser
+ .lock()
+ .await
+ .dispose_browser_context(context_id.clone())
+ .await;
+ return Err(err).context("failed to create page");
+ }
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-transport-browser-tests/src/browser.rs` around lines 215 - 237, When
create_browser_context in TestContext::new succeeds but new_page(...) fails,
ensure you dispose the created context to avoid leaks: after obtaining
context_id, wrap the new_page call so that on any error you call the browser
context teardown API (e.g., delete_browser_context / close_browser_context or
the appropriate method on shared.browser) using the same context_id (via
shared.browser.lock().await) before returning the error; update the error path
in TestContext::new around create_browser_context/new_page to perform this
cleanup.
Comment on lines
+61
to
+69
| impl Drop for TestServer { | ||
| fn drop(&mut self) { | ||
| if let Some(tx) = self.shutdown_tx.take() { | ||
| let _ = tx.send(()); | ||
| } | ||
| if let Some(task) = self.task.take() { | ||
| task.abort(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Abort tracked handler tasks in Drop as well.
When shutdown() is not reached (panic/early unwind), Drop currently aborts only the accept loop. Already-spawned per-session handlers can continue running and interfere with later tests.
🔧 Proposed fix
impl Drop for TestServer {
fn drop(&mut self) {
if let Some(tx) = self.shutdown_tx.take() {
let _ = tx.send(());
}
if let Some(task) = self.task.take() {
task.abort();
}
+ if let Ok(mut handles) = self.handler_tasks.lock() {
+ for handle in handles.drain(..) {
+ handle.abort();
+ }
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-transport-browser-tests/src/server.rs` around lines 61 - 69, The Drop
impl for TestServer only aborts the accept loop task and sends shutdown_tx, but
it must also abort any spawned per-session handler tasks to avoid them lingering
after panics; update the TestServer struct to track handler JoinHandles (e.g. a
Vec or HashMap of JoinHandle) when you spawn session handlers, then modify impl
Drop for TestServer so that after taking self.task it also iterates over and
aborts each stored handler handle (and clear/take the collection) similar to how
shutdown_tx and task are handled; ensure you reference the existing fields
shutdown_tx, task, and the new handler collection in the Drop code and abort
each JoinHandle to guarantee all spawned handlers are stopped.
Collaborator
|
Thanks for this, I'll have a chance to look soon. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-transport-browser-tests/tests/concurrent_accept.rs (1)
134-194: Task return values are discarded.The bi and uni tasks return meaningful values (
format!("bi-done-{i}")and the received data string), but these are not collected or verified. The test only checks that all tasks completed. This is likely intentional since the focus is on the waker bug, but verifying the returned data would strengthen the test's correctness assertions.♻️ Optional: Collect and verify task results
let deadline = tokio::time::Instant::now() + Duration::from_secs(5); - let mut completed = 0; + let mut results = Vec::new(); while let Some(result) = tokio::time::timeout_at(deadline, tasks.join_next()) .await .ok() .flatten() { - if let Err(e) = result { - if e.is_panic() { - std::panic::resume_unwind(e.into_panic()); + match result { + Ok(s) => results.push(s), + Err(e) => { + if e.is_panic() { + std::panic::resume_unwind(e.into_panic()); + } } } - completed += 1; } assert_eq!( - completed, + results.len(), N * 2, - "only {completed}/{} accept tasks completed", + "only {}/{} accept tasks completed", + results.len(), N * 2 );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-transport-browser-tests/tests/concurrent_accept.rs` around lines 134 - 194, The spawned bi/uni tasks inside the ServerHandler currently return strings (e.g., format!("bi-done-{i}") and the UTF-8 payload) but those return values are never collected; modify the tasks.join_next() loop in the handler to capture successful JoinSet outputs, push the Ok join values into a Vec<String> (or two Vecs for bi/uni), and after the loop assert that you received N bi results matching "bi-done-{i}" and N uni results matching the sent payloads (or at least verify total count and expected patterns). Locate the loop around tasks.join_next(), update the branch that handles Ok(result) to extract the returned String from the join and store/verify it before counting completion; keep panic handling (is_panic/resume_unwind) as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-transport-browser-tests/tests/concurrent_accept.rs`:
- Around line 134-194: The spawned bi/uni tasks inside the ServerHandler
currently return strings (e.g., format!("bi-done-{i}") and the UTF-8 payload)
but those return values are never collected; modify the tasks.join_next() loop
in the handler to capture successful JoinSet outputs, push the Ok join values
into a Vec<String> (or two Vecs for bi/uni), and after the loop assert that you
received N bi results matching "bi-done-{i}" and N uni results matching the sent
payloads (or at least verify total count and expected patterns). Locate the loop
around tasks.join_next(), update the branch that handles Ok(result) to extract
the returned String from the join and store/verify it before counting
completion; keep panic handling (is_panic/resume_unwind) as-is.
Add a new unpublished workspace crate `web-transport-browser-tests` that provides infrastructure for running WebTransport tests against headless Chromium. The crate launches a shared browser singleton, creates isolated incognito contexts per test, generates short-lived self-signed certs, and wraps user JS snippets with a `connectWebTransport()` helper. Includes a smoke test proving end-to-end browser-to-server connectivity. Browser tests are excluded from the normal `just check`/`just test` workflows and run separately via `just browser-test`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Expand the browser test infrastructure to support handler panic propagation, expected handler counts, pre-accept request handling, and robust Chrome lifecycle management (dedicated runtime, keepalive page, cleanup watchdog). Add 43 integration tests covering bidirectional/unidirectional streams, datagrams, connection lifecycle, stream error codes, concurrency, and large data transfers against a real Chromium instance. Co-Authored-By: Claude Code <noreply@anthropic.com>
Now that CloseWebTransportSession capsule is implemented, update the browser test suite to reflect proper close behavior: remove #[ignore] annotations from passing tests, await session.closed() to ensure capsule delivery, keep stream references to prevent early cancellation, and improve JS error reporting with stack traces. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Assert WebTransportError::Closed(_, _) directly in all browser tests instead of using the is_session_closed() helper, ensuring the close capsule info is accurately surfaced through every error path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…handling Tests cover half-close semantics, stream priorities, rapid stream creation, concurrent bidi/uni streams, session stats, datagram edge cases (max size, oversized, empty, high water marks), and post-close/reset/stop error behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wrap Browser in tokio::sync::Mutex and ChildStdin in std::sync::Mutex so SharedBrowser derives Sync naturally without unsafe. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace `let _ = task.await` with `.expect()` so that panics inside the spawned accept loop are propagated instead of silently swallowed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test was sorting messages before comparing, which only verified set membership rather than actual priority/arrival ordering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pass chrome_pid and data_dir as shell positional arguments ($0, $1) instead of interpolating them into the command string with format!(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ailable Remove unwrap_or(0) fallback that caused `kill -9 0` to target the whole process group. Instead, preserve the Option<u32> and only spawn the cleanup watchdog when a Chrome PID is available. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace expect-based tx.send(()) with let _ = tx.send(()) so that a dropped receiver does not mask the real accept-loop error surfaced by task.await.expect(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ream_error tests Tests were unreliable because they used wt.close()/wt.closed for synchronization, which races with the stream error signals. Instead, use server-to-client writes and second-stream reads as synchronization points to ensure the server has observed the error before the connection tears down. Also increase immediate_close_handler delay to 1000ms. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Spawning multiple tasks that each call accept_bi() or accept_uni() on the same session causes all but one caller to hang indefinitely. The unfold stream only stores one waker, so concurrent pollers get their wakers overwritten and are never woken again. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test assertion now expects SessionError::ConnectionError(LocallyClosed) instead of SessionError::SendDatagramError(ConnectionLost(LocallyClosed)), matching the upcoming session error propagation behavior where connection-level errors are replaced with the stored session error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ai-and-i
force-pushed
the
fix/browser-tests
branch
from
ce12526 to
27696fa
Compare
… tests Eliminate flaky timing dependencies by using stream reads/writes as synchronization points instead of arbitrary sleeps. Also fix close assertions to be less strict where the exact error variant is not the focus of the test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kixelated
reviewed
Mar 5, 2026
Collaborator
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
This is really cool and we sorely need more automated testing, but IDK if I want to maintain this. A separate repo maybe?
Contributor
Author
|
Sure, separate repo makes sense too. It only depends on the public API of the library, so easy to keep separate. But it is really helpful to have in some form (perhaps even run in CI on every commit or every release of the library). It already found a few issues, and having it gives quite a bit of extra confidence in the library's compliance with the spec. Would you be open to have it as a separate repo under moq-dev, and perhaps have CI for web-transport run it in github actions? |
kixelated
reviewed
Mar 10, 2026
| check: | ||
| cargo check --workspace --all-targets --all-features | ||
| cargo clippy --workspace --all-targets --all-features -- -D warnings | ||
| cargo check --workspace --all-targets --all-features --exclude web-transport-browser-tests |
Collaborator
There was a problem hiding this comment.
This is the main thing that worries me. It makes sense not to run everything locally, but not even type checking the implementation seems sus.
Collaborator
Hmm, yeah I'm just worried that it's a bit heavy. If we're not running in locally, and only in CI, that really screams separate repo or manual testing. |
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
2 participants
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
This PR adds tests to ensure web-transport-quinn works smoothly with browsers (focusing on Chromium). Currently, a few of the tests fail - I'm preparing the 2nd PR that fixes it.
web-transport-browser-testscrate with comprehensive end-to-end tests that exercise the WebTransport implementation against a real Chromium browserjust browser-testcommandTest plan
cargo test -p web-transport-browser-teststo execute the full browser test suitejust checkandjust teststill pass (browser tests are excluded)just browser-testas a convenience command🤖 Generated with Claude Code