From 2cb6235617ea374a6a1c5a1f236fd94ce88f99dc Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 22 Apr 2026 09:56:42 -0400 Subject: [PATCH] refactor(types): centralize test SettingsLayer fixture builder Add SettingsLayer::test_default() and SettingsLayer::ensure_test_auth_methods() to fabro-types behind a "test-support" feature, then collapse the five near-identical ensure_fixture_auth_methods/default_settings/test_default_settings helpers that the dev-token gating cleanup spread across fabro-config, fabro-server, and fabro-workflow. Why: the next required SettingsLayer field would otherwise need updating in five places. With the canonical helper in fabro-types, adding a required field becomes a one-line change. The cfg(any(test, feature = "test-support")) gate keeps the helpers out of production builds. Consumer crates enable the feature via dev-dependencies. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-config/Cargo.toml | 1 + .../fabro-config/tests/resolve_server.rs | 21 ++---------- lib/crates/fabro-server/Cargo.toml | 1 + lib/crates/fabro-server/src/run_manifest.rs | 23 ++----------- lib/crates/fabro-server/src/serve.rs | 30 +++-------------- lib/crates/fabro-server/src/server.rs | 19 +++-------- lib/crates/fabro-types/Cargo.toml | 1 + lib/crates/fabro-types/src/settings/layer.rs | 33 +++++++++++++++++++ lib/crates/fabro-workflow/Cargo.toml | 1 + .../fabro-workflow/src/operations/create.rs | 29 +++------------- .../fabro-workflow/src/operations/start.rs | 21 ++---------- 11 files changed, 58 insertions(+), 122 deletions(-) diff --git a/lib/crates/fabro-config/Cargo.toml b/lib/crates/fabro-config/Cargo.toml index 8034688c0..180578261 100644 --- a/lib/crates/fabro-config/Cargo.toml +++ b/lib/crates/fabro-config/Cargo.toml @@ -35,3 +35,4 @@ ulid.workspace = true [dev-dependencies] tempfile = "3" toml.workspace = true +fabro-types = { path = "../fabro-types", features = ["test-support"] } diff --git a/lib/crates/fabro-config/tests/resolve_server.rs b/lib/crates/fabro-config/tests/resolve_server.rs index d347ca111..1da6f3ed1 100644 --- a/lib/crates/fabro-config/tests/resolve_server.rs +++ b/lib/crates/fabro-config/tests/resolve_server.rs @@ -1,34 +1,19 @@ use fabro_config::parse_settings_layer; use fabro_config::user::default_storage_dir; use fabro_types::settings::server::{ - GithubIntegrationStrategy, IpAllowEntry, ObjectStoreSettings, ServerAuthMethod, - ServerListenSettings, + GithubIntegrationStrategy, IpAllowEntry, ObjectStoreSettings, ServerListenSettings, }; use fabro_types::settings::{InterpString, SettingsLayer}; use fabro_util::Home; fn parse(source: &str) -> SettingsLayer { let mut layer = parse_settings_layer(source).expect("fixture should parse"); - if layer - .server - .as_ref() - .and_then(|server| server.auth.as_ref()) - .and_then(|auth| auth.methods.as_ref()) - .is_none() - { - let server = layer.server.get_or_insert_with(Default::default); - let auth = server.auth.get_or_insert_with(Default::default); - auth.methods = Some(vec![ServerAuthMethod::DevToken]); - } + layer.ensure_test_auth_methods(); layer } fn empty_settings_with_auth_methods() -> SettingsLayer { - parse( - r" -_version = 1 -", - ) + SettingsLayer::test_default() } #[test] diff --git a/lib/crates/fabro-server/Cargo.toml b/lib/crates/fabro-server/Cargo.toml index 9e109d8d2..838287f98 100644 --- a/lib/crates/fabro-server/Cargo.toml +++ b/lib/crates/fabro-server/Cargo.toml @@ -91,3 +91,4 @@ async-trait.workspace = true tokio-util.workspace = true fabro-sandbox = { path = "../fabro-sandbox", features = ["test-support"] } fabro-test = { workspace = true } +fabro-types = { path = "../fabro-types", features = ["test-support"] } diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 017e6b2a5..ded7f88c1 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -956,31 +956,12 @@ mod tests { fn server_settings_fixture(source: &str) -> SettingsLayer { let mut layer = fabro_config::parse_settings_layer(source).expect("v2 fixture should parse"); - ensure_fixture_auth_methods(&mut layer); + layer.ensure_test_auth_methods(); layer } - fn ensure_fixture_auth_methods(layer: &mut SettingsLayer) { - use fabro_types::settings::server::{ServerAuthLayer, ServerAuthMethod, ServerLayer}; - - if layer - .server - .as_ref() - .and_then(|server| server.auth.as_ref()) - .and_then(|auth| auth.methods.as_ref()) - .is_some() - { - return; - } - let server = layer.server.get_or_insert_with(ServerLayer::default); - let auth = server.auth.get_or_insert_with(ServerAuthLayer::default); - auth.methods = Some(vec![ServerAuthMethod::DevToken]); - } - fn default_settings_fixture() -> SettingsLayer { - let mut layer = SettingsLayer::default(); - ensure_fixture_auth_methods(&mut layer); - layer + SettingsLayer::test_default() } #[test] diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 65774ddf1..53829fca4 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -912,30 +912,7 @@ mod tests { fn parse_settings(source: &str) -> SettingsLayer { let mut layer = parse_settings_layer(source).expect("v2 fixture should parse"); - ensure_fixture_auth_methods(&mut layer); - layer - } - - fn ensure_fixture_auth_methods(layer: &mut SettingsLayer) { - use fabro_types::settings::server::{ServerAuthLayer, ServerAuthMethod, ServerLayer}; - - if layer - .server - .as_ref() - .and_then(|server| server.auth.as_ref()) - .and_then(|auth| auth.methods.as_ref()) - .is_some() - { - return; - } - let server = layer.server.get_or_insert_with(ServerLayer::default); - let auth = server.auth.get_or_insert_with(ServerAuthLayer::default); - auth.methods = Some(vec![ServerAuthMethod::DevToken]); - } - - fn default_settings() -> SettingsLayer { - let mut layer = SettingsLayer::default(); - ensure_fixture_auth_methods(&mut layer); + layer.ensure_test_auth_methods(); layer } @@ -1031,7 +1008,8 @@ enabled = false #[test] fn resolve_bind_request_from_settings_defaults_to_socket_when_listen_is_absent() { - let bind = resolve_bind_request_from_settings(&default_settings(), None).expect("bind"); + let bind = + resolve_bind_request_from_settings(&SettingsLayer::test_default(), None).expect("bind"); assert_eq!(bind, BindRequest::Unix(Home::from_env().socket_path())); } @@ -1073,7 +1051,7 @@ address = "127.0.0.1:32276" #[test] fn resolve_bind_request_from_settings_preserves_host_only_cli_bind() { - let settings = default_settings(); + let settings = SettingsLayer::test_default(); let bind = resolve_bind_request_from_settings(&settings, Some("127.0.0.1")).expect("bind"); diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index c38b7e1e2..d90c16c7c 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -63,8 +63,7 @@ use fabro_store::{ }; use fabro_types::settings::run::RunMode; use fabro_types::settings::server::{ - GithubIntegrationSettings, GithubIntegrationStrategy, ServerAuthLayer, ServerAuthMethod, - ServerLayer, + GithubIntegrationSettings, GithubIntegrationStrategy, ServerAuthMethod, }; use fabro_types::settings::{ InterpString, ServerSettings as ResolvedServerSettings, SettingsLayer, @@ -2581,18 +2580,10 @@ fn default_test_app_state_config( } fn ensure_test_auth_methods(settings: &Arc>) { - let mut settings = settings.write().expect("test settings lock poisoned"); - if settings - .server - .as_ref() - .and_then(|server| server.auth.as_ref()) - .and_then(|auth| auth.methods.as_ref()) - .is_none() - { - let server = settings.server.get_or_insert_with(ServerLayer::default); - let auth = server.auth.get_or_insert_with(ServerAuthLayer::default); - auth.methods = Some(vec![ServerAuthMethod::DevToken]); - } + settings + .write() + .expect("test settings lock poisoned") + .ensure_test_auth_methods(); } pub fn create_app_state_with_store( diff --git a/lib/crates/fabro-types/Cargo.toml b/lib/crates/fabro-types/Cargo.toml index 9c1257f70..a36deeee7 100644 --- a/lib/crates/fabro-types/Cargo.toml +++ b/lib/crates/fabro-types/Cargo.toml @@ -12,6 +12,7 @@ doctest = false [features] default = [] clap = ["dep:clap"] +test-support = [] [lints] workspace = true diff --git a/lib/crates/fabro-types/src/settings/layer.rs b/lib/crates/fabro-types/src/settings/layer.rs index 81cd94c5e..188ece46d 100644 --- a/lib/crates/fabro-types/src/settings/layer.rs +++ b/lib/crates/fabro-types/src/settings/layer.rs @@ -32,3 +32,36 @@ pub struct SettingsLayer { #[serde(default, skip_serializing_if = "Option::is_none")] pub features: Option, } + +#[cfg(any(test, feature = "test-support"))] +impl SettingsLayer { + /// A default layer that resolves cleanly: populates `server.auth.methods` + /// with `["dev-token"]`. Use anywhere a test needs a starter + /// `SettingsLayer` that the strict resolver will accept. + #[must_use] + pub fn test_default() -> Self { + let mut layer = Self::default(); + layer.ensure_test_auth_methods(); + layer + } + + /// If `server.auth.methods` is unset, populate it with `["dev-token"]`. + /// Existing methods (set by a fixture) are preserved. Use to make a + /// parsed-from-TOML layer resolve cleanly without overriding test intent. + pub fn ensure_test_auth_methods(&mut self) { + use super::server::{ServerAuthLayer, ServerAuthMethod, ServerLayer as ServerLayerTy}; + + if self + .server + .as_ref() + .and_then(|server| server.auth.as_ref()) + .and_then(|auth| auth.methods.as_ref()) + .is_some() + { + return; + } + let server = self.server.get_or_insert_with(ServerLayerTy::default); + let auth = server.auth.get_or_insert_with(ServerAuthLayer::default); + auth.methods = Some(vec![ServerAuthMethod::DevToken]); + } +} diff --git a/lib/crates/fabro-workflow/Cargo.toml b/lib/crates/fabro-workflow/Cargo.toml index 11bdfc4a9..1611a7bdd 100644 --- a/lib/crates/fabro-workflow/Cargo.toml +++ b/lib/crates/fabro-workflow/Cargo.toml @@ -75,3 +75,4 @@ assert_cmd = "2" predicates = "3" fabro-macros = { path = "../fabro-macros" } fabro-test = { workspace = true } +fabro-types = { path = "../fabro-types", features = ["test-support"] } diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index a49341eee..36a995ed7 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -463,9 +463,7 @@ mod tests { } fn test_default_settings() -> SettingsLayer { - let mut layer = SettingsLayer::default(); - ensure_fixture_auth_methods(&mut layer); - layer + SettingsLayer::test_default() } fn validate_dot(dot_source: &str, settings: SettingsLayer) -> Validated { @@ -798,7 +796,7 @@ mod tests { }), ..SettingsLayer::default() }; - ensure_fixture_auth_methods(&mut layer); + layer.ensure_test_auth_methods(); layer }, cwd: dir.path().to_path_buf(), @@ -895,7 +893,7 @@ mod tests { }), ..SettingsLayer::default() }; - ensure_fixture_auth_methods(&mut layer); + layer.ensure_test_auth_methods(); layer }, cwd: dir.path().to_path_buf(), @@ -970,27 +968,10 @@ mod tests { }), ..SettingsLayer::default() }; - ensure_fixture_auth_methods(&mut layer); + layer.ensure_test_auth_methods(); layer } - pub(super) fn ensure_fixture_auth_methods(layer: &mut SettingsLayer) { - use fabro_types::settings::server::{ServerAuthLayer, ServerAuthMethod, ServerLayer}; - - if layer - .server - .as_ref() - .and_then(|server| server.auth.as_ref()) - .and_then(|auth| auth.methods.as_ref()) - .is_some() - { - return; - } - let server = layer.server.get_or_insert_with(ServerLayer::default); - let auth = server.auth.get_or_insert_with(ServerAuthLayer::default); - auth.methods = Some(vec![ServerAuthMethod::DevToken]); - } - fn dry_run_with_storage(storage_dir: &Path) -> SettingsLayer { use fabro_types::settings::run::{RunExecutionLayer, RunLayer, RunMode}; use fabro_types::settings::server::{ServerLayer, ServerStorageLayer}; @@ -1010,7 +991,7 @@ mod tests { }), ..SettingsLayer::default() }; - ensure_fixture_auth_methods(&mut layer); + layer.ensure_test_auth_methods(); layer } diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index d4a8ee6a0..6c83aec21 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -1020,23 +1020,6 @@ mod tests { )) } - fn ensure_fixture_auth_methods(layer: &mut SettingsLayer) { - use fabro_types::settings::server::{ServerAuthLayer, ServerAuthMethod, ServerLayer}; - - if layer - .server - .as_ref() - .and_then(|server| server.auth.as_ref()) - .and_then(|auth| auth.methods.as_ref()) - .is_some() - { - return; - } - let server = layer.server.get_or_insert_with(ServerLayer::default); - let auth = server.auth.get_or_insert_with(ServerAuthLayer::default); - auth.methods = Some(vec![ServerAuthMethod::DevToken]); - } - async fn persisted_workflow(dot: &str, run_dir: &Path) -> (Persisted, Arc) { let store = memory_store(); let created = crate::operations::create(&store, crate::operations::CreateRunInput { @@ -1055,7 +1038,7 @@ mod tests { }), ..SettingsLayer::default() }; - ensure_fixture_auth_methods(&mut layer); + layer.ensure_test_auth_methods(); layer }, cwd: run_dir @@ -1238,7 +1221,7 @@ mod tests { }), ..SettingsLayer::default() }; - ensure_fixture_auth_methods(&mut layer); + layer.ensure_test_auth_methods(); layer }, cwd: temp.path().to_path_buf(),