-
Notifications
You must be signed in to change notification settings - Fork 51
Add browser interoperability test suite #175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ai-and-i
wants to merge
18
commits into
moq-dev:main
Choose a base branch
from
ai-and-i:fix/browser-tests
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
ff9f22b
Add browser interoperability test infrastructure
ai-and-i a07d600
Add comprehensive browser interoperability test suite
ai-and-i dfea632
Update browser tests to use W3C-compliant session close
ai-and-i 281467b
Validate close code and reason propagation in browser tests
ai-and-i 08fb257
Add extended browser test coverage for streams, datagrams, and error …
ai-and-i 2f2446e
Remove unsafe impl Sync for SharedBrowser
ai-and-i ea3f4f8
Surface accept-loop panics in TestServer::shutdown
ai-and-i 288bdcf
Remove sort() in bidi_stream_server_priority to assert arrival order
ai-and-i db71530
Fix shell injection in cleanup watchdog by using positional arguments
ai-and-i 7649691
Fix watchdog kill targeting entire process group when Chrome PID unav…
ai-and-i a87e244
Avoid panic on shutdown send when accept loop already exited
ai-and-i de3f466
Clean connection close in stream_error tests
ai-and-i 2e135a4
Replace wt.close() synchronization with stream-based handshakes in st…
ai-and-i 148b9c7
Add tests reproducing concurrent accept_bi/accept_uni lost waker bug
ai-and-i 27696fa
Update send_datagram_after_close test to expect ConnectionError
ai-and-i c18b87f
Replace sleep-based synchronization with stream handshakes in browser…
ai-and-i de3a83a
Applied cargo fmt
ai-and-i 76ac16a
Fixed one more flaky test
ai-and-i File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| [package] | ||
| name = "web-transport-browser-tests" | ||
| description = "Browser interoperability test infrastructure for web-transport" | ||
| version = "0.1.0" | ||
| edition = "2021" | ||
| publish = false | ||
|
|
||
| [dependencies] | ||
| anyhow = "1" | ||
| chromiumoxide = { version = "0.9", default-features = false, features = [ | ||
| "bytes", | ||
| "fetcher", | ||
| "rustls", | ||
| "zip8", | ||
| ] } | ||
| futures = "0.3" | ||
| rcgen = { version = "0.14", default-features = false, features = ["aws_lc_rs"] } | ||
| rustls = { version = "0.23", default-features = false, features = [ | ||
| "aws-lc-rs", | ||
| "std", | ||
| ] } | ||
| serde = { version = "1", features = ["derive"] } | ||
| serde_json = "1" | ||
| time = "0.3" | ||
| tokio = { version = "1", features = ["full"] } | ||
| tracing = "0.1" | ||
|
|
||
| [dependencies.web-transport-quinn] | ||
| path = "../web-transport-quinn" | ||
|
|
||
| [dev-dependencies] | ||
| bytes = "1" | ||
| http = "1" | ||
| tracing-subscriber = { version = "0.3", features = ["env-filter"] } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,292 @@ | ||
| use std::net::SocketAddr; | ||
| use std::path::PathBuf; | ||
| use std::sync::OnceLock; | ||
| use std::time::Duration; | ||
|
|
||
| use anyhow::{Context, Result}; | ||
| use chromiumoxide::browser::{Browser, BrowserConfig}; | ||
| use chromiumoxide::cdp::browser_protocol::browser::BrowserContextId; | ||
| use chromiumoxide::cdp::browser_protocol::target::{ | ||
| CreateBrowserContextParams, CreateTargetParams, | ||
| }; | ||
| use chromiumoxide::fetcher::{BrowserFetcher, BrowserFetcherOptions}; | ||
| use chromiumoxide::Page; | ||
| use futures::StreamExt; | ||
| use serde::Deserialize; | ||
|
|
||
| use crate::js; | ||
|
|
||
| struct SharedBrowser { | ||
| browser: tokio::sync::Mutex<Browser>, | ||
| /// URL of the blank page server (http://localhost:{port}). | ||
| page_url: String, | ||
| // A dedicated Tokio runtime that owns the browser event handler and HTTP | ||
| // server tasks. This runtime lives as long as the `SharedBrowser` (i.e. | ||
| // for the entire process) so that the tasks are not cancelled when | ||
| // individual `#[tokio::test]` runtimes shut down. | ||
| _runtime: tokio::runtime::Runtime, | ||
| // A page that stays open for the lifetime of the browser to prevent | ||
| // Chrome from exiting when all test contexts are disposed. | ||
| _keepalive_page: Page, | ||
| // Holds the stdin pipe to the cleanup watchdog process. When our process | ||
| // exits (for any reason, including SIGKILL), the OS closes this FD, | ||
| // unblocking the watchdog which then kills Chrome and removes the temp dir. | ||
| _watchdog_pipe: std::sync::Mutex<Option<std::process::ChildStdin>>, | ||
| } | ||
|
|
||
| static BROWSER: OnceLock<SharedBrowser> = OnceLock::new(); | ||
|
|
||
| /// Try to build a BrowserConfig, auto-downloading Chromium via the fetcher if | ||
| /// no local executable is detected. | ||
| async fn build_browser_config() -> BrowserConfig { | ||
| // Use a per-process data dir to avoid stale SingletonLock conflicts. | ||
| let data_dir = std::env::temp_dir().join(format!("chromiumoxide-{}", std::process::id())); | ||
|
|
||
| // Note: .arg() auto-prefixes with "--", so pass bare names. | ||
| let builder = BrowserConfig::builder() | ||
| .new_headless_mode() | ||
| .no_sandbox() | ||
| .user_data_dir(&data_dir) | ||
| .arg("allow-insecure-localhost"); | ||
|
|
||
| match builder.clone().build() { | ||
| Ok(config) => return config, | ||
| Err(_) => { | ||
| tracing::info!("no local Chrome found, downloading via fetcher..."); | ||
| } | ||
| } | ||
|
|
||
| // Auto-detection failed — download Chromium. | ||
| let exe_path = fetch_chromium().await; | ||
|
|
||
| builder | ||
| .chrome_executable(exe_path) | ||
| .build() | ||
| .expect("failed to build browser config with fetched executable") | ||
| } | ||
|
|
||
| /// Download Chromium using chromiumoxide_fetcher and return the executable path. | ||
| async fn fetch_chromium() -> PathBuf { | ||
| // Use the same cache directory as the fetcher's default | ||
| // ($XDG_CACHE_HOME or $HOME/.cache)/chromiumoxide. We must create it | ||
| // ourselves because the fetcher doesn't create parent directories. | ||
| 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") | ||
| }); | ||
| let cache_dir = base.join("chromiumoxide"); | ||
| std::fs::create_dir_all(&cache_dir).expect("failed to create browser cache directory"); | ||
|
|
||
| let options = BrowserFetcherOptions::builder() | ||
| .with_path(&cache_dir) | ||
| .build() | ||
| .expect("failed to create fetcher options"); | ||
|
|
||
| let fetcher = BrowserFetcher::new(options); | ||
| let installation = fetcher.fetch().await.expect("failed to download Chromium"); | ||
|
|
||
| tracing::info!(path = %installation.executable_path.display(), "downloaded Chromium"); | ||
| installation.executable_path | ||
| } | ||
|
|
||
| /// Spawn a background shell that blocks on stdin. When our process exits (for | ||
| /// any reason), the OS closes the pipe, `read` gets EOF, and the shell kills | ||
| /// Chrome and removes its data directory. | ||
| fn spawn_cleanup_watchdog( | ||
| chrome_pid: u32, | ||
| data_dir: &std::path::Path, | ||
| ) -> Option<std::process::ChildStdin> { | ||
| use std::process::{Command, Stdio}; | ||
|
|
||
| // Pass chrome_pid and data_dir as positional arguments ($0, $1) to avoid | ||
| // shell injection from interpolating paths into the command string. | ||
| let mut child = Command::new("sh") | ||
| .arg("-c") | ||
| .arg(r#"read _; kill -9 "$0" 2>/dev/null; rm -rf "$1""#) | ||
| .arg(chrome_pid.to_string()) | ||
| .arg(data_dir.as_os_str()) | ||
| .stdin(Stdio::piped()) | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .spawn() | ||
| .ok()?; | ||
| child.stdin.take() | ||
| } | ||
|
|
||
| fn init_browser() -> SharedBrowser { | ||
| // Create a dedicated runtime on a separate thread. We cannot call | ||
| // `block_on` from within an existing Tokio runtime (which `#[tokio::test]` | ||
| // creates), so we spawn a plain OS thread to build and initialise | ||
| // everything, then send the results back. | ||
| std::thread::spawn(|| { | ||
| let rt = tokio::runtime::Builder::new_multi_thread() | ||
| .worker_threads(2) | ||
| .enable_all() | ||
| .build() | ||
| .expect("failed to build dedicated browser runtime"); | ||
|
|
||
| let (browser, page_url, keepalive_page, chrome_pid) = rt.block_on(async { | ||
| let config = build_browser_config().await; | ||
|
|
||
| // Start the blank page server on this runtime. | ||
| let listener = tokio::net::TcpListener::bind("127.0.0.1:0") | ||
| .await | ||
| .expect("failed to bind blank page server"); | ||
| let addr: SocketAddr = listener.local_addr().unwrap(); | ||
| let page_url = format!("http://localhost:{}", addr.port()); | ||
|
|
||
| tokio::spawn(async move { | ||
| loop { | ||
| let Ok((mut stream, _)) = listener.accept().await else { | ||
| break; | ||
| }; | ||
| tokio::spawn(async move { | ||
| use tokio::io::{AsyncReadExt, AsyncWriteExt}; | ||
| let mut buf = [0u8; 1024]; | ||
| let _ = stream.read(&mut buf).await; | ||
| let response = "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: 13\r\n\r\n<html></html>"; | ||
| let _ = stream.write_all(response.as_bytes()).await; | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| let (mut browser, mut handler) = Browser::launch(config) | ||
| .await | ||
| .expect("failed to launch browser"); | ||
|
|
||
| // Extract Chrome's PID before we lose mutability, so the watchdog | ||
| // can kill it on abnormal exit. | ||
| let chrome_pid = browser.get_mut_child().and_then(|c| c.inner.id()); | ||
|
|
||
| // Spawn the CDP event handler on this runtime. | ||
| tokio::spawn(async move { | ||
| while let Some(event) = handler.next().await { | ||
| let _ = event; | ||
| } | ||
| }); | ||
|
|
||
| // Open a keepalive page in the default context so Chrome doesn't | ||
| // exit when all test browser contexts are disposed. | ||
| let keepalive_page = browser | ||
| .new_page(&page_url) | ||
| .await | ||
| .expect("failed to create keepalive page"); | ||
|
|
||
| (browser, page_url, keepalive_page, chrome_pid) | ||
| }); | ||
|
|
||
| // Spawn a watchdog that kills Chrome and cleans up the temp dir when | ||
| // our process exits. The data dir path matches build_browser_config(). | ||
| let data_dir = | ||
| std::env::temp_dir().join(format!("chromiumoxide-{}", std::process::id())); | ||
| let watchdog_pipe = chrome_pid.and_then(|pid| spawn_cleanup_watchdog(pid, &data_dir)); | ||
|
|
||
| SharedBrowser { | ||
| browser: tokio::sync::Mutex::new(browser), | ||
| page_url, | ||
| _runtime: rt, | ||
| _keepalive_page: keepalive_page, | ||
| _watchdog_pipe: std::sync::Mutex::new(watchdog_pipe), | ||
| } | ||
| }) | ||
| .join() | ||
| .expect("browser init thread panicked") | ||
| } | ||
|
|
||
| fn get_browser() -> &'static SharedBrowser { | ||
| BROWSER.get_or_init(init_browser) | ||
| } | ||
|
|
||
| /// An isolated browser context for a single test. | ||
| /// | ||
| /// Each context has its own cookies, cache, and page — preventing | ||
| /// cross-test interference. | ||
| pub struct TestContext { | ||
| page: Page, | ||
| context_id: Option<BrowserContextId>, | ||
| } | ||
|
|
||
| impl TestContext { | ||
| /// Create a new isolated browser context. | ||
| pub async fn new() -> Result<Self> { | ||
| let shared = get_browser(); | ||
|
|
||
| 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")?; | ||
|
Comment on lines
+215
to
+237
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dispose the created browser context when page creation fails.
🔧 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 |
||
|
|
||
| Ok(Self { | ||
| page, | ||
| context_id: Some(context_id), | ||
| }) | ||
| } | ||
|
|
||
| /// Evaluate a JavaScript test snippet in the browser. | ||
| /// | ||
| /// The `js_code` is wrapped with `connectWebTransport()` and error handling | ||
| /// via [`js::wrap_test_js`]. | ||
| pub async fn run_js_test( | ||
| &self, | ||
| server_url: &str, | ||
| certificate_hash: &[u8], | ||
| js_code: &str, | ||
| timeout: Duration, | ||
| ) -> Result<JsTestResult> { | ||
| let wrapped = js::wrap_test_js(server_url, certificate_hash, js_code); | ||
|
|
||
| let result: String = tokio::time::timeout(timeout, self.page.evaluate(wrapped)) | ||
| .await | ||
| .context("JS test timed out")? | ||
| .context("JS evaluation failed")? | ||
| .into_value() | ||
| .context("failed to extract JS result value")?; | ||
|
|
||
| let parsed: JsTestResult = | ||
| serde_json::from_str(&result).context("failed to parse JS test result")?; | ||
|
|
||
| Ok(parsed) | ||
| } | ||
|
|
||
| /// Dispose of the browser context. | ||
| pub async fn dispose(mut self) { | ||
| if let Some(id) = self.context_id.take() { | ||
| let shared = get_browser(); | ||
| let _ = shared | ||
| .browser | ||
| .lock() | ||
| .await | ||
| .dispose_browser_context(id) | ||
| .await; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// The result of a JavaScript test evaluation. | ||
| #[derive(Debug, Deserialize)] | ||
| pub struct JsTestResult { | ||
| pub success: bool, | ||
| pub message: String, | ||
| #[serde(default)] | ||
| pub details: serde_json::Value, | ||
| } | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.