From 9e6350c16be1bf78378f5bade90f772eccfbff6c Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 25 Mar 2026 22:35:10 -0700 Subject: [PATCH] fix: box apply_patch test harness futures --- codex-rs/Cargo.lock | 10 -- codex-rs/Cargo.toml | 2 - codex-rs/core/Cargo.toml | 1 - codex-rs/core/tests/common/test_codex.rs | 8 +- codex-rs/core/tests/suite/apply_patch_cli.rs | 61 ++++---- codex-rs/test-macros/BUILD.bazel | 7 - codex-rs/test-macros/Cargo.toml | 16 -- codex-rs/test-macros/src/lib.rs | 155 ------------------- 8 files changed, 35 insertions(+), 225 deletions(-) delete mode 100644 codex-rs/test-macros/BUILD.bazel delete mode 100644 codex-rs/test-macros/Cargo.toml delete mode 100644 codex-rs/test-macros/src/lib.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 02625d90c2..782cd6fd46 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1920,7 +1920,6 @@ dependencies = [ "codex-shell-escalation", "codex-state", "codex-terminal-detection", - "codex-test-macros", "codex-utils-absolute-path", "codex-utils-cache", "codex-utils-cargo-bin", @@ -2696,15 +2695,6 @@ dependencies = [ "tracing", ] -[[package]] -name = "codex-test-macros" -version = "0.0.0" -dependencies = [ - "proc-macro2", - "quote", - "syn 2.0.114", -] - [[package]] name = "codex-tui" version = "0.0.0" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index f19fb650f7..4d325777a7 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -80,7 +80,6 @@ members = [ "state", "terminal-detection", "codex-experimental-api-macros", - "test-macros", "package-manager", "plugin", "artifacts", @@ -152,7 +151,6 @@ codex-shell-escalation = { path = "shell-escalation" } codex-skills = { path = "skills" } codex-state = { path = "state" } codex-stdio-to-uds = { path = "stdio-to-uds" } -codex-test-macros = { path = "test-macros" } codex-terminal-detection = { path = "terminal-detection" } codex-tui = { path = "tui" } codex-tui-app-server = { path = "tui_app_server" } diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 25e44d7631..4274a6bbb2 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -149,7 +149,6 @@ codex-arg0 = { workspace = true } codex-otel = { workspace = true, features = [ "disable-default-metrics-exporter", ] } -codex-test-macros = { workspace = true } codex-utils-cargo-bin = { workspace = true } core_test_support = { workspace = true } ctor = { workspace = true } diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index 5ed5e8a731..2f90911d36 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -786,7 +786,7 @@ impl TestCodexHarness { } pub async fn submit(&self, prompt: &str) -> Result<()> { - self.test.submit_turn(prompt).await + Box::pin(self.test.submit_turn(prompt)).await } pub async fn submit_with_policy( @@ -839,12 +839,14 @@ impl TestCodexHarness { output_type: ApplyPatchModelOutput, ) -> String { match output_type { - ApplyPatchModelOutput::Freeform => self.custom_tool_call_output(call_id).await, + ApplyPatchModelOutput::Freeform => { + Box::pin(self.custom_tool_call_output(call_id)).await + } ApplyPatchModelOutput::Function | ApplyPatchModelOutput::Shell | ApplyPatchModelOutput::ShellViaHeredoc | ApplyPatchModelOutput::ShellCommandViaHeredoc => { - self.function_call_stdout(call_id).await + Box::pin(self.function_call_stdout(call_id)).await } } } diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index d0cefcb616..f00d1467ae 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -3,7 +3,6 @@ use anyhow::Result; use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; -use codex_test_macros::large_stack_test; use core_test_support::responses::ev_apply_patch_call; use core_test_support::responses::ev_apply_patch_custom_tool_call; use core_test_support::responses::ev_shell_command_call; @@ -51,7 +50,7 @@ async fn apply_patch_harness_with( let builder = configure(test_codex()).with_config(|config| { config.include_apply_patch_tool = true; }); - TestCodexHarness::with_builder(builder).await + Box::pin(TestCodexHarness::with_builder(builder)).await } pub async fn mount_apply_patch( @@ -87,7 +86,7 @@ fn apply_patch_responses( ] } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -134,7 +133,7 @@ D delete.txt Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -161,7 +160,7 @@ async fn apply_patch_cli_multiple_chunks(model_output: ApplyPatchModelOutput) -> Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -190,7 +189,7 @@ async fn apply_patch_cli_moves_file_to_new_directory( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -218,7 +217,7 @@ async fn apply_patch_cli_updates_file_appends_trailing_newline( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -244,7 +243,7 @@ async fn apply_patch_cli_insert_only_hunk_modifies_file( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -275,7 +274,7 @@ async fn apply_patch_cli_move_overwrites_existing_destination( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -338,7 +337,7 @@ async fn apply_patch_cli_move_without_content_change_has_no_turn_diff( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -364,7 +363,7 @@ async fn apply_patch_cli_add_overwrites_existing_file( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -396,7 +395,7 @@ async fn apply_patch_cli_rejects_invalid_hunk_header( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -430,7 +429,7 @@ async fn apply_patch_cli_reports_missing_context( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -466,7 +465,7 @@ async fn apply_patch_cli_reports_missing_target_file( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -503,7 +502,7 @@ async fn apply_patch_cli_delete_missing_file_reports_error( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -528,7 +527,7 @@ async fn apply_patch_cli_rejects_empty_patch(model_output: ApplyPatchModelOutput Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -555,7 +554,7 @@ async fn apply_patch_cli_delete_directory_reports_verification_error( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -609,7 +608,7 @@ async fn apply_patch_cli_rejects_path_traversal_outside_workspace( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -664,7 +663,7 @@ async fn apply_patch_cli_rejects_move_path_traversal_outside_workspace( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -701,7 +700,7 @@ async fn apply_patch_cli_verification_failure_has_no_side_effects( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn apply_patch_shell_command_heredoc_with_cd_updates_relative_workdir() -> Result<()> { skip_if_no_network!(Ok(())); @@ -739,7 +738,7 @@ async fn apply_patch_shell_command_heredoc_with_cd_updates_relative_workdir() -> Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn apply_patch_cli_can_use_shell_command_output_as_patch_input() -> Result<()> { skip_if_no_network!(Ok(())); @@ -879,7 +878,7 @@ async fn apply_patch_cli_can_use_shell_command_output_as_patch_input() -> Result Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn apply_patch_shell_command_heredoc_with_cd_emits_turn_diff() -> Result<()> { skip_if_no_network!(Ok(())); @@ -964,7 +963,7 @@ async fn apply_patch_shell_command_heredoc_with_cd_emits_turn_diff() -> Result<( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn apply_patch_shell_command_failure_propagates_error_and_skips_diff() -> Result<()> { skip_if_no_network!(Ok(())); @@ -1042,7 +1041,7 @@ async fn apply_patch_shell_command_failure_propagates_error_and_skips_diff() -> Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::ShellViaHeredoc)] #[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)] async fn apply_patch_function_accepts_lenient_heredoc_wrapped_patch( @@ -1065,7 +1064,7 @@ async fn apply_patch_function_accepts_lenient_heredoc_wrapped_patch( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -1088,7 +1087,7 @@ async fn apply_patch_cli_end_of_file_anchor(model_output: ApplyPatchModelOutput) Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -1123,7 +1122,7 @@ async fn apply_patch_cli_missing_second_chunk_context_rejected( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -1184,7 +1183,7 @@ async fn apply_patch_emits_turn_diff_event_with_unified_diff( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] @@ -1252,7 +1251,7 @@ async fn apply_patch_turn_diff_for_rename_with_content_change( Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn apply_patch_aggregates_diff_across_multiple_tool_calls() -> Result<()> { skip_if_no_network!(Ok(())); @@ -1322,7 +1321,7 @@ async fn apply_patch_aggregates_diff_across_multiple_tool_calls() -> Result<()> Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn apply_patch_aggregates_diff_preserves_success_after_failure() -> Result<()> { skip_if_no_network!(Ok(())); @@ -1414,7 +1413,7 @@ async fn apply_patch_aggregates_diff_preserves_success_after_failure() -> Result Ok(()) } -#[large_stack_test] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] #[test_case(ApplyPatchModelOutput::Shell)] diff --git a/codex-rs/test-macros/BUILD.bazel b/codex-rs/test-macros/BUILD.bazel deleted file mode 100644 index 0ff074f5e5..0000000000 --- a/codex-rs/test-macros/BUILD.bazel +++ /dev/null @@ -1,7 +0,0 @@ -load("//:defs.bzl", "codex_rust_crate") - -codex_rust_crate( - name = "test-macros", - crate_name = "codex_test_macros", - proc_macro = True, -) diff --git a/codex-rs/test-macros/Cargo.toml b/codex-rs/test-macros/Cargo.toml deleted file mode 100644 index 275580322d..0000000000 --- a/codex-rs/test-macros/Cargo.toml +++ /dev/null @@ -1,16 +0,0 @@ -[package] -name = "codex-test-macros" -version.workspace = true -edition.workspace = true -license.workspace = true - -[lib] -proc-macro = true - -[dependencies] -proc-macro2 = "1" -quote = "1" -syn = { version = "2", features = ["full"] } - -[lints] -workspace = true diff --git a/codex-rs/test-macros/src/lib.rs b/codex-rs/test-macros/src/lib.rs deleted file mode 100644 index 16c87f8582..0000000000 --- a/codex-rs/test-macros/src/lib.rs +++ /dev/null @@ -1,155 +0,0 @@ -use proc_macro::TokenStream; -use proc_macro2::TokenStream as TokenStream2; -use quote::quote; -use syn::Attribute; -use syn::ItemFn; -use syn::parse::Nothing; -use syn::parse_macro_input; -use syn::parse_quote; - -const LARGE_STACK_TEST_STACK_SIZE_BYTES: usize = 16 * 1024 * 1024; - -/// Run a test body on a dedicated thread with a larger stack. -/// -/// For async tests, this macro creates a Tokio multi-thread runtime with two -/// worker threads and blocks on the original async body inside the large-stack -/// thread. -#[proc_macro_attribute] -pub fn large_stack_test(attr: TokenStream, item: TokenStream) -> TokenStream { - parse_macro_input!(attr as Nothing); - - let item = parse_macro_input!(item as ItemFn); - expand_large_stack_test(item).into() -} - -fn expand_large_stack_test(mut item: ItemFn) -> TokenStream2 { - let attrs = filtered_attributes(&item.attrs); - item.attrs = attrs; - - let is_async = item.sig.asyncness.take().is_some(); - let name = &item.sig.ident; - let body = &item.block; - - let thread_body = if is_async { - quote! { - { - let runtime = ::tokio::runtime::Builder::new_multi_thread() - .worker_threads(2) - .enable_all() - .build() - .unwrap_or_else(|error| { - panic!("failed to build tokio runtime for large-stack test: {error}") - }); - runtime.block_on(async move #body) - } - } - } else { - quote! { #body } - }; - - *item.block = parse_quote!({ - let handle = ::std::thread::Builder::new() - .name(::std::string::String::from(::std::stringify!(#name))) - .stack_size(#LARGE_STACK_TEST_STACK_SIZE_BYTES) - .spawn(move || #thread_body) - .unwrap_or_else(|error| { - panic!("failed to spawn large-stack test thread: {error}") - }); - - match handle.join() { - Ok(result) => result, - Err(payload) => ::std::panic::resume_unwind(payload), - } - }); - - quote! { #item } -} - -fn filtered_attributes(attrs: &[Attribute]) -> Vec { - let mut filtered = Vec::with_capacity(attrs.len() + 1); - let mut has_test_attr = false; - - for attr in attrs { - if is_tokio_test_attr(attr) { - continue; - } - if is_test_attr(attr) || is_test_case_attr(attr) { - has_test_attr = true; - } - filtered.push(attr.clone()); - } - - if !has_test_attr { - filtered.push(parse_quote!(#[test])); - } - - filtered -} - -fn is_test_attr(attr: &Attribute) -> bool { - attr.path().is_ident("test") -} - -fn is_test_case_attr(attr: &Attribute) -> bool { - attr.path().is_ident("test_case") -} - -fn is_tokio_test_attr(attr: &Attribute) -> bool { - let mut segments = attr.path().segments.iter(); - matches!( - (segments.next(), segments.next(), segments.next()), - (Some(first), Some(second), None) if first.ident == "tokio" && second.ident == "test" - ) -} - -#[cfg(test)] -mod tests { - use super::expand_large_stack_test; - use syn::ItemFn; - use syn::parse_quote; - - fn has_attr(item: &ItemFn, name: &str) -> bool { - item.attrs.iter().any(|attr| attr.path().is_ident(name)) - } - - #[test] - fn adds_test_attribute_when_missing() { - let item: ItemFn = parse_quote! { - fn sample() {} - }; - - let expanded_tokens = expand_large_stack_test(item); - let expanded: ItemFn = match syn::parse2(expanded_tokens) { - Ok(expanded) => expanded, - Err(error) => panic!("failed to parse expanded function: {error}"), - }; - - assert!(has_attr(&expanded, "test")); - let body = quote::quote!(#expanded).to_string(); - assert!(body.contains("stack_size")); - } - - #[test] - fn removes_tokio_test_and_keeps_test_case() { - let item: ItemFn = parse_quote! { - #[test_case(1)] - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn sample(value: usize) -> anyhow::Result<()> { - let _ = value; - Ok(()) - } - }; - - let expanded_tokens = expand_large_stack_test(item); - let expanded: ItemFn = match syn::parse2(expanded_tokens) { - Ok(expanded) => expanded, - Err(error) => panic!("failed to parse expanded function: {error}"), - }; - - assert!(has_attr(&expanded, "test_case")); - assert!(!has_attr(&expanded, "test")); - let body = quote::quote!(#expanded).to_string(); - assert!(body.contains("tokio :: runtime :: Builder")); - assert!(!body.contains("tokio :: test")); - } -}