mirror of
https://github.com/openai/codex.git
synced 2026-08-23 13:09:46 +00:00
test: add TestAppServer builder (#31425)
## Why Test callers need one composable way to create app-server fixtures instead of a growing family of overlapping constructor implementations. ## What - add a feature-complete TestAppServer::builder() - make the default builder own a temporary CODEX_HOME and select the automatic test environment - expose builder knobs for no automatic environment, explicit CODEX_HOME, program, arguments, plugin startup tasks, environment overrides, managed config, and JSON logging - keep the existing public constructor surface, but route every constructor through the builder so the new path is exercised immediately - remove the redundant private constructor ladders; caller migration and public constructor removal live in the optional cleanup stack ## Validation - just test -p codex-app-server (940/941 before updating the expected builder error wording) - just test -p codex-app-server auto_env_rejects_explicit_environment_config - just fix -p codex-app-server - just fmt ## Follow-up stacks Cleanup, optional for the benchmark work: 1. [#31451 test: migrate TestAppServer callers to builder](https://github.com/openai/codex/pull/31451) 2. [#31452 test: remove TestAppServer constructors](https://github.com/openai/codex/pull/31452) Benchmark infrastructure: 1. [#31427 test: add delayed exec-server transport](https://github.com/openai/codex/pull/31427) 2. [#31295 bench: add cold skill load macrobenchmark](https://github.com/openai/codex/pull/31295) 3. [#31428 bench: add e2e benchmark entrypoints](https://github.com/openai/codex/pull/31428) 4. [#31429 ci: smoke Bazel e2e benchmarks](https://github.com/openai/codex/pull/31429)
This commit is contained in:
committed by
GitHub
parent
2589f7a52d
commit
922249310f
1
codex-rs/Cargo.lock
generated
1
codex-rs/Cargo.lock
generated
@@ -412,6 +412,7 @@ dependencies = [
|
||||
"serde",
|
||||
"serde_json",
|
||||
"shlex",
|
||||
"tempfile",
|
||||
"tokio",
|
||||
"uuid",
|
||||
"wiremock",
|
||||
|
||||
@@ -27,6 +27,7 @@ codex-protocol = { workspace = true }
|
||||
codex-utils-cargo-bin = { workspace = true }
|
||||
serde = { workspace = true }
|
||||
serde_json = { workspace = true }
|
||||
tempfile = { workspace = true }
|
||||
tokio = { workspace = true, features = [
|
||||
"io-std",
|
||||
"macros",
|
||||
|
||||
@@ -49,6 +49,7 @@ use serde::de::DeserializeOwned;
|
||||
pub use test_app_server::DEFAULT_CLIENT_NAME;
|
||||
pub use test_app_server::DISABLE_PLUGIN_STARTUP_TASKS_ARG;
|
||||
pub use test_app_server::TestAppServer;
|
||||
pub use test_app_server::TestAppServerBuilder;
|
||||
|
||||
pub fn to_response<T: DeserializeOwned>(response: JSONRPCResponse) -> anyhow::Result<T> {
|
||||
let value = serde_json::to_value(response.result)?;
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use std::collections::VecDeque;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::process::ExitStatus;
|
||||
use std::process::Stdio;
|
||||
use std::sync::atomic::AtomicI64;
|
||||
@@ -122,6 +123,7 @@ use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR;
|
||||
use codex_login::default_client::CODEX_INTERNAL_ORIGINATOR_OVERRIDE_ENV_VAR;
|
||||
use core_test_support::test_codex::TestEnv;
|
||||
use core_test_support::test_codex::test_env;
|
||||
use tempfile::TempDir;
|
||||
use tokio::process::Command;
|
||||
|
||||
use crate::json_logging::JsonLogCapture;
|
||||
@@ -138,6 +140,8 @@ pub struct TestAppServer {
|
||||
pending_messages: VecDeque<JSONRPCMessage>,
|
||||
auto_env: Option<TestEnv>,
|
||||
json_logs: JsonLogCapture,
|
||||
codex_home: PathBuf,
|
||||
_owned_codex_home: Option<TempDir>,
|
||||
}
|
||||
|
||||
pub const DEFAULT_CLIENT_NAME: &str = "codex-app-server-tests";
|
||||
@@ -145,12 +149,28 @@ pub const DISABLE_PLUGIN_STARTUP_TASKS_ARG: &str = "--disable-plugin-startup-tas
|
||||
const DISABLE_MANAGED_CONFIG_ENV_VAR: &str = "CODEX_APP_SERVER_DISABLE_MANAGED_CONFIG";
|
||||
|
||||
impl TestAppServer {
|
||||
/// Starts building a server with a temporary CODEX_HOME and the standard
|
||||
/// automatic test environment.
|
||||
pub fn builder() -> TestAppServerBuilder {
|
||||
TestAppServerBuilder {
|
||||
codex_home: None,
|
||||
environment: TestAppServerEnvironment::Auto,
|
||||
program: None,
|
||||
env_overrides: Vec::new(),
|
||||
args: vec![DISABLE_PLUGIN_STARTUP_TASKS_ARG.to_string()],
|
||||
}
|
||||
}
|
||||
|
||||
pub async fn wait_for_exit(&mut self) -> std::io::Result<ExitStatus> {
|
||||
self.process.wait().await
|
||||
}
|
||||
|
||||
pub async fn new(codex_home: &Path) -> anyhow::Result<Self> {
|
||||
Self::new_with_env_and_args(codex_home, &[], &[DISABLE_PLUGIN_STARTUP_TASKS_ARG]).await
|
||||
Self::builder()
|
||||
.with_codex_home(codex_home)
|
||||
.without_auto_env()
|
||||
.build()
|
||||
.await
|
||||
}
|
||||
|
||||
/// Starts an app server with the standard test environment and retains it
|
||||
@@ -164,7 +184,7 @@ impl TestAppServer {
|
||||
/// URL-based configuration, this helper rejects a `codex_home` containing
|
||||
/// that file.
|
||||
pub async fn new_with_auto_env(codex_home: &Path) -> anyhow::Result<Self> {
|
||||
Self::new_with_auto_env_and_env(codex_home, &[]).await
|
||||
Self::builder().with_codex_home(codex_home).build().await
|
||||
}
|
||||
|
||||
/// Starts an auto-environment app server that emits JSON logs.
|
||||
@@ -174,47 +194,11 @@ impl TestAppServer {
|
||||
codex_home: &Path,
|
||||
rust_log: impl Into<String>,
|
||||
) -> anyhow::Result<Self> {
|
||||
let rust_log = rust_log.into();
|
||||
Self::new_with_auto_env_and_env(
|
||||
codex_home,
|
||||
&[
|
||||
("LOG_FORMAT", Some("json")),
|
||||
("RUST_LOG", Some(rust_log.as_str())),
|
||||
],
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
async fn new_with_auto_env_and_env(
|
||||
codex_home: &Path,
|
||||
extra_env_overrides: &[(&str, Option<&str>)],
|
||||
) -> anyhow::Result<Self> {
|
||||
let environments_toml = codex_home.join("environments.toml");
|
||||
ensure!(
|
||||
!environments_toml
|
||||
.try_exists()
|
||||
.with_context(|| format!("check whether {} exists", environments_toml.display()))?,
|
||||
"new_with_auto_env cannot be used when {} exists",
|
||||
environments_toml.display()
|
||||
);
|
||||
|
||||
let auto_env = test_env().await?;
|
||||
// Noise registry configuration takes precedence over the URL-based
|
||||
// provider, so clear inherited values to keep the selection hermetic.
|
||||
let mut env_overrides = vec![
|
||||
(
|
||||
CODEX_EXEC_SERVER_URL_ENV_VAR,
|
||||
auto_env.environment().exec_server_url(),
|
||||
),
|
||||
(CODEX_EXEC_SERVER_NOISE_REGISTRY_URL_ENV_VAR, None),
|
||||
(CODEX_EXEC_SERVER_NOISE_ENVIRONMENT_ID_ENV_VAR, None),
|
||||
(CODEX_EXEC_SERVER_NOISE_AUTH_TOKEN_ENV_VAR, None),
|
||||
(CODEX_EXEC_SERVER_NOISE_CHATGPT_ACCOUNT_ID_ENV_VAR, None),
|
||||
];
|
||||
env_overrides.extend_from_slice(extra_env_overrides);
|
||||
let mut app_server = Self::new_with_env(codex_home, &env_overrides).await?;
|
||||
app_server.auto_env = Some(auto_env);
|
||||
Ok(app_server)
|
||||
Self::builder()
|
||||
.with_codex_home(codex_home)
|
||||
.with_json_logging(rust_log)
|
||||
.build()
|
||||
.await
|
||||
}
|
||||
|
||||
/// Returns the automatically selected test environment retained by this server.
|
||||
@@ -238,6 +222,11 @@ impl TestAppServer {
|
||||
})
|
||||
}
|
||||
|
||||
/// Returns the effective CODEX_HOME used by the child app-server.
|
||||
pub fn codex_home(&self) -> &Path {
|
||||
&self.codex_home
|
||||
}
|
||||
|
||||
/// Waits for a JSON stderr event whose structured `event.name` field matches.
|
||||
pub async fn wait_for_json_log_event(
|
||||
&self,
|
||||
@@ -261,33 +250,56 @@ impl TestAppServer {
|
||||
}
|
||||
|
||||
pub async fn new_without_managed_config(codex_home: &Path) -> anyhow::Result<Self> {
|
||||
Self::new_with_env(codex_home, &[(DISABLE_MANAGED_CONFIG_ENV_VAR, Some("1"))]).await
|
||||
Self::builder()
|
||||
.with_codex_home(codex_home)
|
||||
.without_auto_env()
|
||||
.without_managed_config()
|
||||
.build()
|
||||
.await
|
||||
}
|
||||
|
||||
pub async fn new_without_managed_config_with_env(
|
||||
codex_home: &Path,
|
||||
env_overrides: &[(&str, Option<&str>)],
|
||||
) -> anyhow::Result<Self> {
|
||||
let mut all_env_overrides = vec![(DISABLE_MANAGED_CONFIG_ENV_VAR, Some("1"))];
|
||||
all_env_overrides.extend_from_slice(env_overrides);
|
||||
Self::new_with_env(codex_home, &all_env_overrides).await
|
||||
Self::builder()
|
||||
.with_codex_home(codex_home)
|
||||
.without_auto_env()
|
||||
.without_managed_config()
|
||||
.with_env_overrides(env_overrides)
|
||||
.build()
|
||||
.await
|
||||
}
|
||||
|
||||
pub async fn new_with_plugin_startup_tasks(codex_home: &Path) -> anyhow::Result<Self> {
|
||||
Self::new_with_env_and_args(codex_home, &[], &[]).await
|
||||
Self::builder()
|
||||
.with_codex_home(codex_home)
|
||||
.without_auto_env()
|
||||
.with_plugin_startup_tasks()
|
||||
.build()
|
||||
.await
|
||||
}
|
||||
|
||||
pub async fn new_with_env_and_plugin_startup_tasks(
|
||||
codex_home: &Path,
|
||||
env_overrides: &[(&str, Option<&str>)],
|
||||
) -> anyhow::Result<Self> {
|
||||
Self::new_with_env_and_args(codex_home, env_overrides, &[]).await
|
||||
Self::builder()
|
||||
.with_codex_home(codex_home)
|
||||
.without_auto_env()
|
||||
.with_plugin_startup_tasks()
|
||||
.with_env_overrides(env_overrides)
|
||||
.build()
|
||||
.await
|
||||
}
|
||||
|
||||
pub async fn new_with_args(codex_home: &Path, args: &[&str]) -> anyhow::Result<Self> {
|
||||
let mut all_args = vec![DISABLE_PLUGIN_STARTUP_TASKS_ARG];
|
||||
all_args.extend_from_slice(args);
|
||||
Self::new_with_env_and_args(codex_home, &[], &all_args).await
|
||||
Self::builder()
|
||||
.with_codex_home(codex_home)
|
||||
.without_auto_env()
|
||||
.with_args(args)
|
||||
.build()
|
||||
.await
|
||||
}
|
||||
|
||||
/// Creates a new MCP process, allowing tests to override or remove
|
||||
@@ -299,12 +311,12 @@ impl TestAppServer {
|
||||
codex_home: &Path,
|
||||
env_overrides: &[(&str, Option<&str>)],
|
||||
) -> anyhow::Result<Self> {
|
||||
Self::new_with_env_and_args(
|
||||
codex_home,
|
||||
env_overrides,
|
||||
&[DISABLE_PLUGIN_STARTUP_TASKS_ARG],
|
||||
)
|
||||
.await
|
||||
Self::builder()
|
||||
.with_codex_home(codex_home)
|
||||
.without_auto_env()
|
||||
.with_env_overrides(env_overrides)
|
||||
.build()
|
||||
.await
|
||||
}
|
||||
|
||||
pub async fn new_with_program_and_env(
|
||||
@@ -312,23 +324,13 @@ impl TestAppServer {
|
||||
program: &Path,
|
||||
env_overrides: &[(&str, Option<&str>)],
|
||||
) -> anyhow::Result<Self> {
|
||||
Self::new_with_program_env_and_args(
|
||||
codex_home,
|
||||
program,
|
||||
env_overrides,
|
||||
&[DISABLE_PLUGIN_STARTUP_TASKS_ARG],
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
async fn new_with_env_and_args(
|
||||
codex_home: &Path,
|
||||
env_overrides: &[(&str, Option<&str>)],
|
||||
args: &[&str],
|
||||
) -> anyhow::Result<Self> {
|
||||
let program = codex_utils_cargo_bin::cargo_bin("codex-app-server")
|
||||
.context("should find binary for codex-app-server")?;
|
||||
Self::new_with_program_env_and_args(codex_home, &program, env_overrides, args).await
|
||||
Self::builder()
|
||||
.with_codex_home(codex_home)
|
||||
.without_auto_env()
|
||||
.with_program(program)
|
||||
.with_env_overrides(env_overrides)
|
||||
.build()
|
||||
.await
|
||||
}
|
||||
|
||||
async fn new_with_program_env_and_args(
|
||||
@@ -399,6 +401,8 @@ impl TestAppServer {
|
||||
pending_messages: VecDeque::new(),
|
||||
auto_env: None,
|
||||
json_logs,
|
||||
codex_home: codex_home.to_path_buf(),
|
||||
_owned_codex_home: None,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -1800,6 +1804,164 @@ impl TestAppServer {
|
||||
}
|
||||
}
|
||||
|
||||
/// Builder for TestAppServer.
|
||||
pub struct TestAppServerBuilder {
|
||||
codex_home: Option<PathBuf>,
|
||||
environment: TestAppServerEnvironment,
|
||||
program: Option<PathBuf>,
|
||||
env_overrides: Vec<(String, Option<String>)>,
|
||||
args: Vec<String>,
|
||||
}
|
||||
|
||||
enum TestAppServerEnvironment {
|
||||
Auto,
|
||||
None,
|
||||
}
|
||||
|
||||
impl TestAppServerBuilder {
|
||||
/// Uses this existing CODEX_HOME instead of a temporary one.
|
||||
pub fn with_codex_home(mut self, codex_home: &Path) -> Self {
|
||||
self.codex_home = Some(codex_home.to_path_buf());
|
||||
self
|
||||
}
|
||||
|
||||
/// Starts app-server without the standard automatic test environment.
|
||||
pub fn without_auto_env(mut self) -> Self {
|
||||
self.environment = TestAppServerEnvironment::None;
|
||||
self
|
||||
}
|
||||
|
||||
/// Uses this app-server binary instead of the standard test binary.
|
||||
pub fn with_program(mut self, program: &Path) -> Self {
|
||||
self.program = Some(program.to_path_buf());
|
||||
self
|
||||
}
|
||||
|
||||
/// Adds command-line arguments after the default test arguments.
|
||||
pub fn with_args(mut self, args: &[&str]) -> Self {
|
||||
self.args
|
||||
.extend(args.iter().map(|argument| (*argument).to_string()));
|
||||
self
|
||||
}
|
||||
|
||||
/// Enables startup tasks that the default test arguments disable.
|
||||
pub fn with_plugin_startup_tasks(mut self) -> Self {
|
||||
self.args
|
||||
.retain(|argument| argument != DISABLE_PLUGIN_STARTUP_TASKS_ARG);
|
||||
self
|
||||
}
|
||||
|
||||
/// Adds child-process environment overrides.
|
||||
///
|
||||
/// Some values set variables and None values remove inherited variables.
|
||||
pub fn with_env_overrides(mut self, env_overrides: &[(&str, Option<&str>)]) -> Self {
|
||||
self.env_overrides
|
||||
.extend(env_overrides.iter().map(|(key, value)| {
|
||||
(
|
||||
(*key).to_string(),
|
||||
value.map(std::string::ToString::to_string),
|
||||
)
|
||||
}));
|
||||
self
|
||||
}
|
||||
|
||||
/// Prevents the child from loading managed configuration.
|
||||
pub fn without_managed_config(self) -> Self {
|
||||
self.with_env_overrides(&[(DISABLE_MANAGED_CONFIG_ENV_VAR, Some("1"))])
|
||||
}
|
||||
|
||||
/// Configures the child to emit JSON logs at the requested Rust log level.
|
||||
pub fn with_json_logging(self, rust_log: impl Into<String>) -> Self {
|
||||
let rust_log = rust_log.into();
|
||||
let mut builder = self.with_env_overrides(&[("LOG_FORMAT", Some("json"))]);
|
||||
builder
|
||||
.env_overrides
|
||||
.push(("RUST_LOG".to_string(), Some(rust_log)));
|
||||
builder
|
||||
}
|
||||
|
||||
/// Builds a server with a temporary CODEX_HOME and automatic environment
|
||||
/// by default.
|
||||
pub async fn build(self) -> anyhow::Result<TestAppServer> {
|
||||
let Self {
|
||||
codex_home,
|
||||
environment,
|
||||
program,
|
||||
mut env_overrides,
|
||||
args,
|
||||
} = self;
|
||||
let (codex_home, owned_codex_home) = match codex_home {
|
||||
Some(codex_home) => (codex_home, None),
|
||||
None => {
|
||||
let owned_codex_home = TempDir::new()?;
|
||||
(
|
||||
owned_codex_home.path().to_path_buf(),
|
||||
Some(owned_codex_home),
|
||||
)
|
||||
}
|
||||
};
|
||||
let auto_env = match environment {
|
||||
TestAppServerEnvironment::Auto => {
|
||||
let environments_toml = codex_home.join("environments.toml");
|
||||
ensure!(
|
||||
!environments_toml.try_exists().with_context(|| format!(
|
||||
"check whether {} exists",
|
||||
environments_toml.display()
|
||||
))?,
|
||||
"automatic environment cannot be used when {} exists",
|
||||
environments_toml.display()
|
||||
);
|
||||
let auto_env = test_env().await?;
|
||||
// Noise registry configuration takes precedence over the URL-based
|
||||
// provider, so clear inherited values to keep the selection hermetic.
|
||||
let mut auto_env_overrides = vec![
|
||||
(
|
||||
CODEX_EXEC_SERVER_URL_ENV_VAR.to_string(),
|
||||
auto_env.environment().exec_server_url().map(str::to_string),
|
||||
),
|
||||
(
|
||||
CODEX_EXEC_SERVER_NOISE_REGISTRY_URL_ENV_VAR.to_string(),
|
||||
None,
|
||||
),
|
||||
(
|
||||
CODEX_EXEC_SERVER_NOISE_ENVIRONMENT_ID_ENV_VAR.to_string(),
|
||||
None,
|
||||
),
|
||||
(CODEX_EXEC_SERVER_NOISE_AUTH_TOKEN_ENV_VAR.to_string(), None),
|
||||
(
|
||||
CODEX_EXEC_SERVER_NOISE_CHATGPT_ACCOUNT_ID_ENV_VAR.to_string(),
|
||||
None,
|
||||
),
|
||||
];
|
||||
auto_env_overrides.append(&mut env_overrides);
|
||||
env_overrides = auto_env_overrides;
|
||||
Some(auto_env)
|
||||
}
|
||||
TestAppServerEnvironment::None => None,
|
||||
};
|
||||
let program = match program {
|
||||
Some(program) => program,
|
||||
None => codex_utils_cargo_bin::cargo_bin("codex-app-server")
|
||||
.context("should find binary for codex-app-server")?,
|
||||
};
|
||||
let env_overrides = env_overrides
|
||||
.iter()
|
||||
.map(|(key, value)| (key.as_str(), value.as_deref()))
|
||||
.collect::<Vec<_>>();
|
||||
let args = args.iter().map(String::as_str).collect::<Vec<_>>();
|
||||
let mut app_server = TestAppServer::new_with_program_env_and_args(
|
||||
&codex_home,
|
||||
&program,
|
||||
&env_overrides,
|
||||
&args,
|
||||
)
|
||||
.await?;
|
||||
app_server.auto_env = auto_env;
|
||||
app_server._owned_codex_home = owned_codex_home;
|
||||
Ok(app_server)
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for TestAppServer {
|
||||
fn drop(&mut self) {
|
||||
// These tests spawn a `codex-app-server` child process.
|
||||
|
||||
@@ -117,7 +117,7 @@ async fn auto_env_rejects_explicit_environment_config() -> Result<()> {
|
||||
assert_eq!(
|
||||
err.to_string(),
|
||||
format!(
|
||||
"new_with_auto_env cannot be used when {} exists",
|
||||
"automatic environment cannot be used when {} exists",
|
||||
codex_home.path().join("environments.toml").display()
|
||||
)
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user