mirror of
https://github.com/openai/codex.git
synced 2026-09-15 12:08:01 +00:00
Remove test-support feature from codex-core and replace it with explicit test toggles
## Why `codex-core` still had a `test-support` crate feature that was enabled by multiple consumers (`core_test_support`, `app_test_support`, and `codex-tui` dev-deps). That introduced a second feature-resolved shape of `codex-core`, which increases compile cost and cache fragmentation across the workspace. The same root issue previously affected `deterministic_process_ids`: we were using crate features as a test/runtime switch, and Cargo had to build extra variants of a very large crate. ## What Changed ### 1) Remove `test-support` as a crate feature - Deleted the feature declaration from `core/Cargo.toml`. - Removed `features = ["test-support"]` from: - `core/tests/common/Cargo.toml` - `app-server/tests/common/Cargo.toml` - `tui/Cargo.toml` (dev-dependency) - Removed Bazel `crate_features = ["test-support"]` from `core/BUILD.bazel`. ### 2) Keep test behavior without feature-gated crate variants - Converted test-only behavior toggles to **explicit runtime switches** backed by `AtomicBool`: - thread-manager test mode toggle - deterministic unified-exec process-id toggle - Enabled those toggles from `core_test_support` ctor so integration tests keep deterministic and test-friendly behavior. ### 3) Replace feature-gated helper APIs with always-compiled hidden helpers - APIs that were previously behind `#[cfg(feature = "test-support")]` are now available as `#[doc(hidden)]` test helpers, avoiding feature-split builds while preserving existing test call sites. ## Expected Benefits ### Concrete dependency-graph effect `cargo tree -p codex-core -e features` now shows only the default feature path for `codex-core`; there are no `test-support` or `deterministic_process_ids` feature edges remaining. ### Build-performance impact - Eliminates feature-driven duplicate `codex-core` compilation variants. - Improves cache reuse across test-support consumers that previously forced separate feature resolution. - Reduces rebuild churn when switching between targets that did and did not depend on the feature-enabled `codex-core` shape. ## Safety / behavior notes - Production behavior remains unchanged by default. - Test-only behavior is now explicit and opt-in via dedicated test toggles, with docstrings clarifying these must stay at default values in production. ## Validation - `just fmt` - `cargo test -p codex-core unified_exec::` - `cargo test -p codex-core --test all unified_exec -- --test-threads=1` - `cargo check -p app_test_support` - `cargo check -p codex-tui --tests` - `cargo check -p codex-core` - `cargo tree -p codex-core -e features`
This commit is contained in:
@@ -12,7 +12,7 @@ anyhow = { workspace = true }
|
||||
base64 = { workspace = true }
|
||||
chrono = { workspace = true }
|
||||
codex-app-server-protocol = { workspace = true }
|
||||
codex-core = { workspace = true, features = ["test-support"] }
|
||||
codex-core = { workspace = true }
|
||||
codex-protocol = { workspace = true }
|
||||
codex-utils-cargo-bin = { workspace = true }
|
||||
serde = { workspace = true }
|
||||
|
||||
@@ -3,10 +3,6 @@ load("//:defs.bzl", "codex_rust_crate")
|
||||
codex_rust_crate(
|
||||
name = "core",
|
||||
crate_name = "codex_core",
|
||||
# TODO(mbolin): Eliminate the use of features in the version of the
|
||||
# rust_library() that is used by rust_binary() rules for release artifacts
|
||||
# such as the Codex CLI.
|
||||
crate_features = ["test-support"],
|
||||
compile_data = glob(
|
||||
include = ["**"],
|
||||
exclude = [
|
||||
|
||||
@@ -113,10 +113,6 @@ which = { workspace = true }
|
||||
wildmatch = { workspace = true }
|
||||
zip = { workspace = true }
|
||||
|
||||
[features]
|
||||
test-support = []
|
||||
|
||||
|
||||
[target.'cfg(target_os = "linux")'.dependencies]
|
||||
keyring = { workspace = true, features = ["linux-native-async-persistent"] }
|
||||
landlock = { workspace = true }
|
||||
|
||||
@@ -981,8 +981,8 @@ impl AuthManager {
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
/// Create an AuthManager with a specific CodexAuth, for testing only.
|
||||
#[doc(hidden)]
|
||||
pub fn from_auth_for_testing(auth: CodexAuth) -> Arc<Self> {
|
||||
let cached = CachedAuth {
|
||||
auth: Some(auth),
|
||||
@@ -998,8 +998,8 @@ impl AuthManager {
|
||||
})
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
/// Create an AuthManager with a specific CodexAuth and codex home, for testing only.
|
||||
#[doc(hidden)]
|
||||
pub fn from_auth_for_testing_with_home(auth: CodexAuth, codex_home: PathBuf) -> Arc<Self> {
|
||||
let cached = CachedAuth {
|
||||
auth: Some(auth),
|
||||
|
||||
@@ -143,9 +143,11 @@ pub use exec_policy::check_execpolicy_for_warnings;
|
||||
pub use exec_policy::load_exec_policy;
|
||||
pub use file_watcher::FileWatcherEvent;
|
||||
pub use safety::get_platform_sandbox;
|
||||
#[doc(hidden)]
|
||||
pub use thread_manager::set_thread_manager_test_mode_for_tests;
|
||||
pub use tools::spec::parse_tool_input_schema;
|
||||
pub use turn_metadata::build_turn_metadata_header;
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
#[doc(hidden)]
|
||||
pub use unified_exec::set_deterministic_process_ids_for_tests;
|
||||
// Re-export the protocol types from the standalone `codex-protocol` crate so existing
|
||||
// `codex_core::protocol::...` references continue to work across the workspace.
|
||||
|
||||
@@ -13,7 +13,7 @@ pub(super) fn builtin_collaboration_mode_presets() -> Vec<CollaborationModeMask>
|
||||
vec![plan_preset(), default_preset()]
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
#[doc(hidden)]
|
||||
pub fn test_builtin_collaboration_mode_presets() -> Vec<CollaborationModeMask> {
|
||||
builtin_collaboration_mode_presets()
|
||||
}
|
||||
|
||||
@@ -336,8 +336,8 @@ impl ModelsManager {
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
/// Construct a manager with a specific provider for testing.
|
||||
#[doc(hidden)]
|
||||
pub fn with_provider(
|
||||
codex_home: PathBuf,
|
||||
auth_manager: Arc<AuthManager>,
|
||||
@@ -355,8 +355,8 @@ impl ModelsManager {
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
/// Get model identifier without consulting remote state or cache.
|
||||
#[doc(hidden)]
|
||||
pub fn get_model_offline(model: Option<&str>) -> String {
|
||||
if let Some(model) = model {
|
||||
return model.to_string();
|
||||
@@ -370,8 +370,8 @@ impl ModelsManager {
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
/// Build `ModelInfo` without consulting remote state or cache.
|
||||
#[doc(hidden)]
|
||||
pub fn construct_model_info_offline(model: &str, config: &Config) -> ModelInfo {
|
||||
model_info::with_config_overrides(model_info::model_info_from_slug(model), config)
|
||||
}
|
||||
|
||||
@@ -4,7 +4,6 @@ pub mod manager;
|
||||
pub mod model_info;
|
||||
pub mod model_presets;
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
pub use collaboration_mode_presets::test_builtin_collaboration_mode_presets;
|
||||
|
||||
/// Convert the client version string to a whole version string (e.g. "1.2.3-alpha.4" -> "1.2.3").
|
||||
|
||||
@@ -359,7 +359,7 @@ pub(super) fn builtin_model_presets(_auth_mode: Option<AuthMode>) -> Vec<ModelPr
|
||||
PRESETS.iter().cloned().collect()
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
#[doc(hidden)]
|
||||
pub fn all_model_presets() -> &'static Vec<ModelPreset> {
|
||||
&PRESETS
|
||||
}
|
||||
|
||||
@@ -1,7 +1,5 @@
|
||||
use crate::AuthManager;
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
use crate::CodexAuth;
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
use crate::ModelProviderInfo;
|
||||
use crate::agent::AgentControl;
|
||||
use crate::codex::Codex;
|
||||
@@ -31,25 +29,48 @@ use codex_protocol::protocol::SessionSource;
|
||||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
use tempfile::TempDir;
|
||||
use std::sync::atomic::AtomicBool;
|
||||
use std::sync::atomic::Ordering;
|
||||
use tokio::runtime::Handle;
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
use tokio::runtime::RuntimeFlavor;
|
||||
use tokio::sync::RwLock;
|
||||
use tokio::sync::broadcast;
|
||||
use tracing::warn;
|
||||
|
||||
const THREAD_CREATED_CHANNEL_CAPACITY: usize = 1024;
|
||||
/// Test-only override for enabling thread-manager behaviors used by integration
|
||||
/// tests.
|
||||
///
|
||||
/// In production builds this value should remain at its default (`false`) and
|
||||
/// must not be toggled.
|
||||
static FORCE_TEST_THREAD_MANAGER_BEHAVIOR: AtomicBool = AtomicBool::new(false);
|
||||
|
||||
#[doc(hidden)]
|
||||
pub fn set_thread_manager_test_mode_for_tests(enabled: bool) {
|
||||
FORCE_TEST_THREAD_MANAGER_BEHAVIOR.store(enabled, Ordering::Relaxed);
|
||||
}
|
||||
|
||||
fn should_use_test_thread_manager_behavior() -> bool {
|
||||
FORCE_TEST_THREAD_MANAGER_BEHAVIOR.load(Ordering::Relaxed)
|
||||
}
|
||||
|
||||
struct TempCodexHomeGuard {
|
||||
path: PathBuf,
|
||||
}
|
||||
|
||||
impl Drop for TempCodexHomeGuard {
|
||||
fn drop(&mut self) {
|
||||
let _ = std::fs::remove_dir_all(&self.path);
|
||||
}
|
||||
}
|
||||
|
||||
fn build_file_watcher(codex_home: PathBuf, skills_manager: Arc<SkillsManager>) -> Arc<FileWatcher> {
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
if let Ok(handle) = Handle::try_current()
|
||||
if should_use_test_thread_manager_behavior()
|
||||
&& let Ok(handle) = Handle::try_current()
|
||||
&& handle.runtime_flavor() == RuntimeFlavor::CurrentThread
|
||||
{
|
||||
// The real watcher spins background tasks that can starve the
|
||||
// current-thread test runtime and cause event waits to time out.
|
||||
// Integration tests compile with the `test-support` feature.
|
||||
warn!("using noop file watcher under current-thread test runtime");
|
||||
return Arc::new(FileWatcher::noop());
|
||||
}
|
||||
@@ -95,8 +116,7 @@ pub struct NewThread {
|
||||
/// them in memory.
|
||||
pub struct ThreadManager {
|
||||
state: Arc<ThreadManagerState>,
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
_test_codex_home_guard: Option<TempDir>,
|
||||
_test_codex_home_guard: Option<TempCodexHomeGuard>,
|
||||
}
|
||||
|
||||
/// Shared, `Arc`-owned state for [`ThreadManager`]. This `Arc` is required to have a single
|
||||
@@ -110,10 +130,8 @@ pub(crate) struct ThreadManagerState {
|
||||
skills_manager: Arc<SkillsManager>,
|
||||
file_watcher: Arc<FileWatcher>,
|
||||
session_source: SessionSource,
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
#[allow(dead_code)]
|
||||
// Captures submitted ops for testing purpose.
|
||||
ops_log: Arc<std::sync::Mutex<Vec<(ThreadId, Op)>>>,
|
||||
// Captures submitted ops for testing purpose when test mode is enabled.
|
||||
ops_log: Option<Arc<std::sync::Mutex<Vec<(ThreadId, Op)>>>>,
|
||||
}
|
||||
|
||||
impl ThreadManager {
|
||||
@@ -134,33 +152,38 @@ impl ThreadManager {
|
||||
file_watcher,
|
||||
auth_manager,
|
||||
session_source,
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
ops_log: Arc::new(std::sync::Mutex::new(Vec::new())),
|
||||
ops_log: should_use_test_thread_manager_behavior()
|
||||
.then(|| Arc::new(std::sync::Mutex::new(Vec::new()))),
|
||||
}),
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
_test_codex_home_guard: None,
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
/// Construct with a dummy AuthManager containing the provided CodexAuth.
|
||||
/// Used for integration tests: should not be used by ordinary business logic.
|
||||
#[doc(hidden)]
|
||||
pub fn with_models_provider(auth: CodexAuth, provider: ModelProviderInfo) -> Self {
|
||||
let temp_dir = tempfile::tempdir().unwrap_or_else(|err| panic!("temp codex home: {err}"));
|
||||
let codex_home = temp_dir.path().to_path_buf();
|
||||
let mut manager = Self::with_models_provider_and_home(auth, provider, codex_home);
|
||||
manager._test_codex_home_guard = Some(temp_dir);
|
||||
set_thread_manager_test_mode_for_tests(true);
|
||||
let codex_home = std::env::temp_dir().join(format!(
|
||||
"codex-thread-manager-test-{}",
|
||||
uuid::Uuid::new_v4()
|
||||
));
|
||||
std::fs::create_dir_all(&codex_home)
|
||||
.unwrap_or_else(|err| panic!("temp codex home dir create failed: {err}"));
|
||||
let mut manager = Self::with_models_provider_and_home(auth, provider, codex_home.clone());
|
||||
manager._test_codex_home_guard = Some(TempCodexHomeGuard { path: codex_home });
|
||||
manager
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
/// Construct with a dummy AuthManager containing the provided CodexAuth and codex home.
|
||||
/// Used for integration tests: should not be used by ordinary business logic.
|
||||
#[doc(hidden)]
|
||||
pub fn with_models_provider_and_home(
|
||||
auth: CodexAuth,
|
||||
provider: ModelProviderInfo,
|
||||
codex_home: PathBuf,
|
||||
) -> Self {
|
||||
set_thread_manager_test_mode_for_tests(true);
|
||||
let auth_manager = AuthManager::from_auth_for_testing(auth);
|
||||
let (thread_created_tx, _) = broadcast::channel(THREAD_CREATED_CHANNEL_CAPACITY);
|
||||
let skills_manager = Arc::new(SkillsManager::new(codex_home.clone()));
|
||||
@@ -178,8 +201,8 @@ impl ThreadManager {
|
||||
file_watcher,
|
||||
auth_manager,
|
||||
session_source: SessionSource::Exec,
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
ops_log: Arc::new(std::sync::Mutex::new(Vec::new())),
|
||||
ops_log: should_use_test_thread_manager_behavior()
|
||||
.then(|| Arc::new(std::sync::Mutex::new(Vec::new()))),
|
||||
}),
|
||||
_test_codex_home_guard: None,
|
||||
}
|
||||
@@ -340,13 +363,13 @@ impl ThreadManager {
|
||||
AgentControl::new(Arc::downgrade(&self.state))
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
#[doc(hidden)]
|
||||
#[allow(dead_code)]
|
||||
pub(crate) fn captured_ops(&self) -> Vec<(ThreadId, Op)> {
|
||||
self.state
|
||||
.ops_log
|
||||
.lock()
|
||||
.map(|log| log.clone())
|
||||
.as_ref()
|
||||
.and_then(|ops_log| ops_log.lock().ok().map(|log| log.clone()))
|
||||
.unwrap_or_default()
|
||||
}
|
||||
}
|
||||
@@ -364,11 +387,10 @@ impl ThreadManagerState {
|
||||
/// Send an operation to a thread by ID.
|
||||
pub(crate) async fn send_op(&self, thread_id: ThreadId, op: Op) -> CodexResult<String> {
|
||||
let thread = self.get_thread(thread_id).await?;
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
if let Some(ops_log) = &self.ops_log
|
||||
&& let Ok(mut log) = ops_log.lock()
|
||||
{
|
||||
if let Ok(mut log) = self.ops_log.lock() {
|
||||
log.push((thread_id, op.clone()));
|
||||
}
|
||||
log.push((thread_id, op.clone()));
|
||||
}
|
||||
thread.submit(op).await
|
||||
}
|
||||
|
||||
@@ -42,7 +42,6 @@ mod head_tail_buffer;
|
||||
mod process;
|
||||
mod process_manager;
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
pub fn set_deterministic_process_ids_for_tests(enabled: bool) {
|
||||
process_manager::set_deterministic_process_ids_for_tests(enabled);
|
||||
}
|
||||
|
||||
@@ -62,28 +62,20 @@ const UNIFIED_EXEC_ENV: [(&str, &str); 10] = [
|
||||
("CODEX_CI", "1"),
|
||||
];
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
/// Test-only override for deterministic unified exec process IDs.
|
||||
///
|
||||
/// In production builds this value should remain at its default (`false`) and
|
||||
/// must not be toggled.
|
||||
static FORCE_DETERMINISTIC_PROCESS_IDS: AtomicBool = AtomicBool::new(false);
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
pub(super) fn set_deterministic_process_ids_for_tests(enabled: bool) {
|
||||
FORCE_DETERMINISTIC_PROCESS_IDS.store(enabled, Ordering::Relaxed);
|
||||
}
|
||||
|
||||
#[cfg(any(test, feature = "test-support"))]
|
||||
fn deterministic_process_ids_forced_for_tests() -> bool {
|
||||
FORCE_DETERMINISTIC_PROCESS_IDS.load(Ordering::Relaxed)
|
||||
}
|
||||
|
||||
#[cfg(not(any(test, feature = "test-support")))]
|
||||
fn deterministic_process_ids_forced_for_tests() -> bool {
|
||||
false
|
||||
}
|
||||
|
||||
fn should_use_deterministic_process_ids() -> bool {
|
||||
cfg!(test) || deterministic_process_ids_forced_for_tests()
|
||||
}
|
||||
|
||||
@@ -11,7 +11,7 @@ path = "lib.rs"
|
||||
anyhow = { workspace = true }
|
||||
assert_cmd = { workspace = true }
|
||||
base64 = { workspace = true }
|
||||
codex-core = { workspace = true, features = ["test-support"] }
|
||||
codex-core = { workspace = true }
|
||||
codex-protocol = { workspace = true }
|
||||
codex-utils-absolute-path = { workspace = true }
|
||||
codex-utils-cargo-bin = { workspace = true }
|
||||
|
||||
@@ -20,6 +20,7 @@ pub mod test_codex_exec;
|
||||
|
||||
#[ctor]
|
||||
fn enable_deterministic_unified_exec_process_ids_for_tests() {
|
||||
codex_core::set_thread_manager_test_mode_for_tests(true);
|
||||
codex_core::set_deterministic_process_ids_for_tests(true);
|
||||
}
|
||||
|
||||
|
||||
@@ -120,7 +120,7 @@ arboard = { workspace = true }
|
||||
|
||||
[dev-dependencies]
|
||||
codex-cli = { workspace = true }
|
||||
codex-core = { workspace = true, features = ["test-support"] }
|
||||
codex-core = { workspace = true }
|
||||
codex-utils-cargo-bin = { workspace = true }
|
||||
codex-utils-pty = { workspace = true }
|
||||
assert_matches = { workspace = true }
|
||||
|
||||
Reference in New Issue
Block a user