From d644ca1e95bb462d5a0ab92ca624005b2f6946f6 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 5 Apr 2026 12:17:29 -0400 Subject: [PATCH] test: standardize no-proxy localhost HTTP clients --- AGENTS.md | 2 ++ files-internal/testing-strategy.md | 15 +++++++++++ lib/crates/fabro-github/tests/integration.rs | 16 +++++------- lib/crates/fabro-llm/tests/integration.rs | 2 +- lib/crates/fabro-test/src/lib.rs | 2 +- test/twin/github/src/handlers/app.rs | 18 ++++++++----- test/twin/github/src/handlers/branches.rs | 6 ++--- test/twin/github/src/handlers/git.rs | 2 +- test/twin/github/src/handlers/graphql.rs | 6 ++--- .../twin/github/src/handlers/installations.rs | 12 ++++----- test/twin/github/src/handlers/manifests.rs | 4 +-- test/twin/github/src/handlers/pulls.rs | 4 +-- test/twin/github/src/handlers/releases.rs | 25 ++++++++++--------- test/twin/github/src/server.rs | 4 ++- test/twin/github/src/test_support.rs | 5 ++++ test/twin/openai/tests/common/mod.rs | 12 +++++++-- test/twin/openai/tests/debug_ui.rs | 2 +- test/twin/openai/tests/failure_modes.rs | 1 + 18 files changed, 87 insertions(+), 51 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 42a7ab8f9..9845119bd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -107,3 +107,5 @@ Never run `cargo insta accept` without first checking what's pending — it acce - `fabro run ` — run a workflow by name (resolves `fabro/workflows//workflow.toml`), e.g. `fabro run repl` - Use `--no-retro` to skip the retro step and finish faster - `#[e2e_test(twin, live("VAR"))]` — dual-mode test that runs against twin-openai or real API. `#[e2e_test(twin)]` for twin-only tests (e.g., scripted failures). `#[e2e_test(live("VAR"))]` for live-only tests requiring secrets. `#[e2e_test()]` for sandbox tests with no API deps. Behavior is controlled by `FABRO_TEST_MODE` (`live`, `strict`; default is `twin`), and `cargo nextest run --profile e2e ...` implies `strict`. Use `fabro_test::e2e_openai!()` in twin/dual-mode tests to get `(base_url, api_key)`. +- Local test HTTP clients must use `.no_proxy()`. Prefer shared helpers like `fabro_test::test_http_client()` or crate-local equivalents instead of `reqwest::Client::new()`, bare `Client::builder().build()`, or `reqwest::get(...)`. +- This is not cosmetic: macOS proxy discovery adds hidden startup overhead to repeated localhost reqwest clients and can surface as misleading nextest timeouts. diff --git a/files-internal/testing-strategy.md b/files-internal/testing-strategy.md index 1cc34bb9e..e1fcd006d 100644 --- a/files-internal/testing-strategy.md +++ b/files-internal/testing-strategy.md @@ -227,6 +227,7 @@ Shared integration-test helpers may: - normalize output - compact structured events - poll for stable command-created conditions +- centralize localhost HTTP client construction Shared integration-test helpers should not: @@ -234,6 +235,20 @@ Shared integration-test helpers should not: - write runtime files the engine is supposed to own - hide broad scenario setup behind opaque helper functions +### Local HTTP clients + +When test code talks to a local server or twin over HTTP, always create the client through a shared test helper that calls `.no_proxy()`. + +Do not open-code localhost clients with: + +- `reqwest::Client::new()` +- bare `Client::builder().build()` +- `reqwest::get(...)` + +Use a crate-local helper or a shared helper such as `fabro_test::test_http_client()` instead. + +This rule exists because macOS proxy discovery adds hidden startup overhead to repeated reqwest client creation. The result looks like random nextest timeouts even when the server under test is only talking to `127.0.0.1`. + ### Fixtures Prefer checked-in fixtures when they express a reusable workflow or scenario shape. diff --git a/lib/crates/fabro-github/tests/integration.rs b/lib/crates/fabro-github/tests/integration.rs index effbc5f9e..8410cde49 100644 --- a/lib/crates/fabro-github/tests/integration.rs +++ b/lib/crates/fabro-github/tests/integration.rs @@ -168,17 +168,13 @@ async fn enable_auto_merge_persists() { .unwrap(); let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem).unwrap(); - let token = create_installation_access_token_for_pr( - &reqwest::Client::new(), - &jwt, - "acme", - "widgets", - &twin.base_url, - ) - .await - .unwrap(); + let client = fabro_test::test_http_client(); + let token = + create_installation_access_token_for_pr(&client, &jwt, "acme", "widgets", &twin.base_url) + .await + .unwrap(); - let detail: serde_json::Value = reqwest::Client::new() + let detail: serde_json::Value = fabro_test::test_http_client() .get(format!( "{}/repos/acme/widgets/pulls/{}", twin.base_url, created.number diff --git a/lib/crates/fabro-llm/tests/integration.rs b/lib/crates/fabro-llm/tests/integration.rs index d01a74c25..d11f62ee7 100644 --- a/lib/crates/fabro-llm/tests/integration.rs +++ b/lib/crates/fabro-llm/tests/integration.rs @@ -79,7 +79,7 @@ async fn openai_server_error() { .strip_suffix("/v1") .expect("OpenAI base URL should end with /v1"); - reqwest::Client::new() + fabro_test::test_http_client() .post(format!("{admin_url}/__admin/scenarios")) .bearer_auth(&api_key) .json(&serde_json::json!({ diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 3db30c751..6e9171163 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -873,7 +873,7 @@ pub struct TwinGitHub { server: twin_github::TestServer, } -fn test_http_client() -> reqwest::Client { +pub fn test_http_client() -> reqwest::Client { reqwest::Client::builder().no_proxy().build().unwrap() } diff --git a/test/twin/github/src/handlers/app.rs b/test/twin/github/src/handlers/app.rs index 72a2d4391..f3b287ab4 100644 --- a/test/twin/github/src/handlers/app.rs +++ b/test/twin/github/src/handlers/app.rs @@ -113,7 +113,7 @@ pub async fn patch_webhook_config( mod tests { use crate::server::TestServer; use crate::state::{AppOptions, AppState}; - use crate::test_support::{sign_test_jwt, test_rsa_private_key}; + use crate::test_support::{sign_test_jwt, test_http_client, test_rsa_private_key}; #[tokio::test] async fn get_app_returns_app_info() { @@ -130,7 +130,7 @@ mod tests { let server = TestServer::start(state).await; let jwt = sign_test_jwt("12345", pem); - let client = reqwest::Client::new(); + let client = test_http_client(); let resp = client .get(&format!("{}/app", server.url())) .header("Authorization", format!("Bearer {jwt}")) @@ -161,7 +161,7 @@ mod tests { }); let server = TestServer::start(state).await; - let client = reqwest::Client::new(); + let client = test_http_client(); let resp = client .get(&format!("{}/app", server.url())) .header("Authorization", "Bearer invalid-jwt") @@ -187,12 +187,16 @@ mod tests { }); let server = TestServer::start(state).await; - let resp = reqwest::get(&format!("{}/apps/my-app", server.url())) + let resp = test_http_client() + .get(format!("{}/apps/my-app", server.url())) + .send() .await .unwrap(); assert_eq!(resp.status(), 200); - let resp404 = reqwest::get(&format!("{}/apps/nonexistent", server.url())) + let resp404 = test_http_client() + .get(format!("{}/apps/nonexistent", server.url())) + .send() .await .unwrap(); assert_eq!(resp404.status(), 404); @@ -214,7 +218,9 @@ mod tests { }); let server = TestServer::start(state).await; - let resp = reqwest::get(&format!("{}/apps/private-app", server.url())) + let resp = test_http_client() + .get(format!("{}/apps/private-app", server.url())) + .send() .await .unwrap(); assert_eq!(resp.status(), 404); diff --git a/test/twin/github/src/handlers/branches.rs b/test/twin/github/src/handlers/branches.rs index bd1e79356..fa55d9577 100644 --- a/test/twin/github/src/handlers/branches.rs +++ b/test/twin/github/src/handlers/branches.rs @@ -93,7 +93,7 @@ pub async fn get_branch( mod tests { use crate::server::TestServer; use crate::state::{AppOptions, AppState}; - use crate::test_support::{sign_test_jwt, test_rsa_private_key}; + use crate::test_support::{sign_test_jwt, test_http_client, test_rsa_private_key}; async fn get_installation_token( client: &reqwest::Client, @@ -151,7 +151,7 @@ mod tests { let server = TestServer::start(state).await; let jwt = sign_test_jwt("100", pem); - let client = reqwest::Client::new(); + let client = test_http_client(); let token = get_installation_token(&client, &jwt, "owner", "repo", server.url()).await; let resp = client @@ -189,7 +189,7 @@ mod tests { let server = TestServer::start(state).await; let jwt = sign_test_jwt("100", pem); - let client = reqwest::Client::new(); + let client = test_http_client(); let token = get_installation_token(&client, &jwt, "owner", "repo", server.url()).await; let resp = client diff --git a/test/twin/github/src/handlers/git.rs b/test/twin/github/src/handlers/git.rs index eb0637d4c..63426bfac 100644 --- a/test/twin/github/src/handlers/git.rs +++ b/test/twin/github/src/handlers/git.rs @@ -383,7 +383,7 @@ mod tests { state.add_repository("owner", "repo", vec!["main".to_string()], false); let server = TestServer::start(state).await; - let client = reqwest::Client::new(); + let client = crate::test_support::test_http_client(); let resp = client .get(format!( "{}/owner/repo.git/info/refs?service=git-upload-pack", diff --git a/test/twin/github/src/handlers/graphql.rs b/test/twin/github/src/handlers/graphql.rs index e98f42661..38942a60f 100644 --- a/test/twin/github/src/handlers/graphql.rs +++ b/test/twin/github/src/handlers/graphql.rs @@ -614,7 +614,7 @@ fn extract_unquoted_value(s: &str, key: &str) -> Option { mod tests { use crate::server::TestServer; use crate::state::{AppOptions, AppState, PullRequest}; - use crate::test_support::{sign_test_jwt, test_rsa_private_key}; + use crate::test_support::{sign_test_jwt, test_http_client, test_rsa_private_key}; async fn get_installation_token( client: &reqwest::Client, @@ -674,7 +674,7 @@ mod tests { let server = TestServer::start(state.clone()).await; let jwt = sign_test_jwt("100", pem); - let client = reqwest::Client::new(); + let client = test_http_client(); let token = get_installation_token(&client, &jwt, "owner", "repo", server.url()).await; (server, client, token) @@ -770,7 +770,7 @@ mod tests { let state = AppState::new(); let server = TestServer::start(state).await; - let client = reqwest::Client::new(); + let client = test_http_client(); let resp = client .post(&format!("{}/graphql", server.url())) .header("Authorization", "Bearer invalid-token") diff --git a/test/twin/github/src/handlers/installations.rs b/test/twin/github/src/handlers/installations.rs index 9b61b2a04..7cddeb22a 100644 --- a/test/twin/github/src/handlers/installations.rs +++ b/test/twin/github/src/handlers/installations.rs @@ -178,7 +178,7 @@ pub async fn create_access_token( mod tests { use crate::server::TestServer; use crate::state::{AppOptions, AppState}; - use crate::test_support::{sign_test_jwt, test_rsa_private_key}; + use crate::test_support::{sign_test_jwt, test_http_client, test_rsa_private_key}; #[tokio::test] async fn get_installation_returns_id() { @@ -197,7 +197,7 @@ mod tests { let server = TestServer::start(state).await; let jwt = sign_test_jwt("100", pem); - let client = reqwest::Client::new(); + let client = test_http_client(); let resp = client .get(&format!("{}/repos/owner/repo/installation", server.url())) .header("Authorization", format!("Bearer {jwt}")) @@ -228,7 +228,7 @@ mod tests { let server = TestServer::start(state).await; let jwt = sign_test_jwt("100", pem); - let client = reqwest::Client::new(); + let client = test_http_client(); let resp = client .get(&format!("{}/repos/owner/repo/installation", server.url())) .header("Authorization", format!("Bearer {jwt}")) @@ -256,7 +256,7 @@ mod tests { let server = TestServer::start(state).await; let jwt = sign_test_jwt("100", pem); - let client = reqwest::Client::new(); + let client = test_http_client(); let resp = client .get(&format!("{}/repos/owner/repo/installation", server.url())) .header("Authorization", format!("Bearer {jwt}")) @@ -284,7 +284,7 @@ mod tests { let server = TestServer::start(state).await; let jwt = sign_test_jwt("100", pem); - let client = reqwest::Client::new(); + let client = test_http_client(); let resp = client .post(&format!( "{}/app/installations/{install_id}/access_tokens", @@ -322,7 +322,7 @@ mod tests { let server = TestServer::start(state).await; let jwt = sign_test_jwt("100", pem); - let client = reqwest::Client::new(); + let client = test_http_client(); let resp = client .post(&format!( "{}/app/installations/{install_id}/access_tokens", diff --git a/test/twin/github/src/handlers/manifests.rs b/test/twin/github/src/handlers/manifests.rs index 6221c6b91..7974c6ffb 100644 --- a/test/twin/github/src/handlers/manifests.rs +++ b/test/twin/github/src/handlers/manifests.rs @@ -56,7 +56,7 @@ mod tests { ); let server = TestServer::start(state).await; - let client = reqwest::Client::new(); + let client = crate::test_support::test_http_client(); let resp = client .post(&format!( "{}/app-manifests/test-code/conversions", @@ -82,7 +82,7 @@ mod tests { let state = AppState::new(); let server = TestServer::start(state).await; - let client = reqwest::Client::new(); + let client = crate::test_support::test_http_client(); let resp = client .post(&format!( "{}/app-manifests/unknown/conversions", diff --git a/test/twin/github/src/handlers/pulls.rs b/test/twin/github/src/handlers/pulls.rs index b558c75d8..46611b802 100644 --- a/test/twin/github/src/handlers/pulls.rs +++ b/test/twin/github/src/handlers/pulls.rs @@ -303,7 +303,7 @@ fn pr_to_json(pr: &PullRequest) -> serde_json::Value { mod tests { use crate::server::TestServer; use crate::state::{AppOptions, AppState}; - use crate::test_support::{sign_test_jwt, test_rsa_private_key}; + use crate::test_support::{sign_test_jwt, test_http_client, test_rsa_private_key}; async fn get_installation_token( client: &reqwest::Client, @@ -361,7 +361,7 @@ mod tests { let server = TestServer::start(state.clone()).await; let jwt = sign_test_jwt("100", pem); - let client = reqwest::Client::new(); + let client = test_http_client(); let token = get_installation_token(&client, &jwt, "owner", "repo", server.url()).await; (server, client, token) diff --git a/test/twin/github/src/handlers/releases.rs b/test/twin/github/src/handlers/releases.rs index 7cf78d16b..c1463b39a 100644 --- a/test/twin/github/src/handlers/releases.rs +++ b/test/twin/github/src/handlers/releases.rs @@ -44,12 +44,14 @@ mod tests { ); let server = TestServer::start(state).await; - let resp = reqwest::get(&format!( - "{}/repos/test-org/test-project/releases/latest", - server.url() - )) - .await - .unwrap(); + let resp = crate::test_support::test_http_client() + .get(format!( + "{}/repos/test-org/test-project/releases/latest", + server.url() + )) + .send() + .await + .unwrap(); assert_eq!(resp.status(), 200); let body: serde_json::Value = resp.json().await.unwrap(); @@ -62,12 +64,11 @@ mod tests { let state = AppState::new(); let server = TestServer::start(state).await; - let resp = reqwest::get(&format!( - "{}/repos/owner/repo/releases/latest", - server.url() - )) - .await - .unwrap(); + let resp = crate::test_support::test_http_client() + .get(format!("{}/repos/owner/repo/releases/latest", server.url())) + .send() + .await + .unwrap(); assert_eq!(resp.status(), 404); server.shutdown().await; diff --git a/test/twin/github/src/server.rs b/test/twin/github/src/server.rs index 08f4e26ba..f1033ed35 100644 --- a/test/twin/github/src/server.rs +++ b/test/twin/github/src/server.rs @@ -75,7 +75,9 @@ mod tests { async fn server_starts_and_responds() { let state = AppState::new(); let server = TestServer::start(state).await; - let resp = reqwest::get(&format!("{}/nonexistent", server.url())) + let resp = crate::test_support::test_http_client() + .get(format!("{}/nonexistent", server.url())) + .send() .await .unwrap(); assert_eq!(resp.status(), 404); diff --git a/test/twin/github/src/test_support.rs b/test/twin/github/src/test_support.rs index 4271032ae..1fec7d85b 100644 --- a/test/twin/github/src/test_support.rs +++ b/test/twin/github/src/test_support.rs @@ -1,3 +1,8 @@ +#[cfg(test)] +pub fn test_http_client() -> reqwest::Client { + reqwest::Client::builder().no_proxy().build().unwrap() +} + #[cfg(test)] pub fn test_rsa_private_key() -> &'static str { include_str!("testdata/rsa_private.pem") diff --git a/test/twin/openai/tests/common/mod.rs b/test/twin/openai/tests/common/mod.rs index 5f2b0c106..b9406ca19 100644 --- a/test/twin/openai/tests/common/mod.rs +++ b/test/twin/openai/tests/common/mod.rs @@ -61,6 +61,10 @@ pub struct ParsedSseEvent { static NEXT_BEARER_TOKEN: AtomicU64 = AtomicU64::new(1); +pub fn test_http_client() -> Result { + Client::builder().no_proxy().build().map_err(Into::into) +} + pub async fn spawn_server() -> Result { let listener = TcpListener::bind("127.0.0.1:0").await?; let addr: SocketAddr = listener.local_addr()?; @@ -90,6 +94,7 @@ fn authorization_header_value(bearer_token: &str) -> String { fn build_authenticated_client(bearer_token: &str) -> Result { Client::builder() + .no_proxy() .default_headers( [( reqwest::header::AUTHORIZATION, @@ -113,7 +118,10 @@ impl ApiClient { ) -> Result { Ok(Self { base_url: base_url.into(), - client: Client::builder().timeout(Duration::from_secs(30)).build()?, + client: Client::builder() + .no_proxy() + .timeout(Duration::from_secs(30)) + .build()?, bearer_token, organization, project, @@ -183,7 +191,7 @@ impl ApiClient { impl TestServer { fn new(base_url: String, bearer_token: String) -> Result { - let client = Client::builder().build()?; + let client = test_http_client()?; let auth_client = build_authenticated_client(&bearer_token)?; Ok(Self { diff --git a/test/twin/openai/tests/debug_ui.rs b/test/twin/openai/tests/debug_ui.rs index e3d3ed662..5af411f52 100644 --- a/test/twin/openai/tests/debug_ui.rs +++ b/test/twin/openai/tests/debug_ui.rs @@ -224,7 +224,7 @@ async fn debug_routes_not_accessible_when_admin_disabled() { }); let base_url = format!("http://{addr}"); - let client = reqwest::Client::new(); + let client = common::test_http_client().expect("test client"); let html_response = client .get(format!("{base_url}/__debug")) diff --git a/test/twin/openai/tests/failure_modes.rs b/test/twin/openai/tests/failure_modes.rs index f98972f6a..26c0536c7 100644 --- a/test/twin/openai/tests/failure_modes.rs +++ b/test/twin/openai/tests/failure_modes.rs @@ -106,6 +106,7 @@ async fn scripted_hang_times_out_client_side() { .await; let client = reqwest::Client::builder() + .no_proxy() .timeout(Duration::from_millis(150)) .default_headers( [(