From 7a9a6629c5707906365d7bf43518e06eb2fe3cbd Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Thu, 7 May 2026 07:45:07 +0200 Subject: [PATCH] Improve feature test coverage parity --- .../collections/feature_tests/materialize.rs | 70 +++++++--- .../collections/tests/feature_tests.rs | 120 ++++++++++++++++++ cmd/devcontainer/src/commands/common.rs | 41 ++++++ .../configuration/features/options.rs | 72 +++++++++-- 4 files changed, 272 insertions(+), 31 deletions(-) diff --git a/cmd/devcontainer/src/commands/collections/feature_tests/materialize.rs b/cmd/devcontainer/src/commands/collections/feature_tests/materialize.rs index 83c0a8197..6986da6b4 100644 --- a/cmd/devcontainer/src/commands/collections/feature_tests/materialize.rs +++ b/cmd/devcontainer/src/commands/collections/feature_tests/materialize.rs @@ -117,9 +117,12 @@ fn feature_option_values_from_manifest(manifest: &Value, value: &Value) -> Vec<( options .iter() .filter_map(|(key, option)| { - option - .get("default") - .map(|default| (feature_option_env_name(key), json_value_to_env(default))) + option.get("default").map(|default| { + ( + common::feature_option_env_name(key), + json_value_to_env(default), + ) + }) }) .collect::>() }) @@ -129,7 +132,12 @@ fn feature_option_values_from_manifest(manifest: &Value, value: &Value) -> Vec<( .map(|options| { options .iter() - .map(|(key, option)| (feature_option_env_name(key), json_value_to_env(option))) + .map(|(key, option)| { + ( + common::feature_option_env_name(key), + json_value_to_env(option), + ) + }) .collect::>() }) .unwrap_or_default(); @@ -155,7 +163,7 @@ pub(super) fn alternate_feature_option_values( let mut values = Vec::new(); for (key, option) in options { - let env_name = feature_option_env_name(key); + let env_name = common::feature_option_env_name(key); let default = option.get("default"); let value = match option.get("type").and_then(Value::as_str) { Some("boolean") => { @@ -232,18 +240,6 @@ fn choose_alternate_string_candidate( values.get(selected_index).cloned() } -fn feature_option_env_name(key: &str) -> String { - key.chars() - .map(|character| { - if character.is_ascii_alphanumeric() { - character.to_ascii_uppercase() - } else { - '_' - } - }) - .collect() -} - pub(super) fn write_feature_test_dockerfile( build_context_dir: &Path, base_image: &str, @@ -383,7 +379,8 @@ mod tests { use serde_json::json; use super::{ - alternate_feature_option_values, choose_alternate_string_candidate, unique_feature_test_dir, + alternate_feature_option_values, choose_alternate_string_candidate, feature_option_values, + unique_feature_test_dir, }; #[test] @@ -440,4 +437,41 @@ mod tests { assert_eq!(values, vec![("COLOR".to_string(), "green".to_string())]); let _ = fs::remove_dir_all(feature_dir); } + + #[test] + fn feature_test_option_env_names_match_upstream_safe_id_cases() { + let feature_dir = unique_feature_test_dir(); + fs::create_dir_all(&feature_dir).expect("feature dir"); + fs::write( + feature_dir.join("devcontainer-feature.json"), + r#"{ + "id": "demo", + "version": "1.0.0", + "options": { + "1name": { + "type": "string", + "default": "default-value" + }, + "option-name": { + "type": "string", + "default": "default-option" + } + } +}"#, + ) + .expect("manifest"); + + let values = feature_option_values( + &feature_dir, + &json!({ + "1name": "override-value" + }), + ) + .expect("values"); + + assert!(values.contains(&("_NAME".to_string(), "override-value".to_string()))); + assert!(values.contains(&("OPTION_NAME".to_string(), "default-option".to_string()))); + assert!(!values.iter().any(|(key, _)| key == "1NAME")); + let _ = fs::remove_dir_all(feature_dir); + } } diff --git a/cmd/devcontainer/src/commands/collections/tests/feature_tests.rs b/cmd/devcontainer/src/commands/collections/tests/feature_tests.rs index d8d7eef27..33e0e28eb 100644 --- a/cmd/devcontainer/src/commands/collections/tests/feature_tests.rs +++ b/cmd/devcontainer/src/commands/collections/tests/feature_tests.rs @@ -111,6 +111,88 @@ fn features_test_discovers_named_and_autogenerated_scenarios() { let _ = fs::remove_dir_all(root); } +#[test] +fn features_test_global_scenarios_only_excludes_feature_scoped_cases() { + let root = unique_temp_dir(); + let src = root.join("src").join("demo"); + let test = root.join("test").join("demo"); + let global = root.join("test").join("_global"); + fs::create_dir_all(&src).expect("feature src"); + fs::create_dir_all(&test).expect("feature test"); + fs::create_dir_all(&global).expect("global test"); + fs::write( + src.join("devcontainer-feature.json"), + "{\n \"id\": \"demo\",\n \"name\": \"Demo Feature\",\n \"version\": \"1.0.0\"\n}\n", + ) + .expect("manifest"); + fs::write(test.join("test.sh"), "#!/bin/sh\n").expect("test script"); + fs::write(test.join("custom.sh"), "#!/bin/sh\n").expect("scenario script"); + fs::write( + test.join("scenarios.json"), + "{\n \"custom\": {\n \"image\": \"ubuntu:latest\"\n }\n}\n", + ) + .expect("scenarios"); + fs::write(global.join("global-scenario.sh"), "#!/bin/sh\n").expect("global scenario script"); + fs::write( + global.join("scenarios.json"), + "{\n \"global-scenario\": {\n \"image\": \"ubuntu:latest\"\n }\n}\n", + ) + .expect("global scenarios"); + + let scenarios = discover_feature_test_scenarios(&[ + "--global-scenarios-only".to_string(), + root.display().to_string(), + ]) + .expect("scenario discovery"); + + assert_eq!(scenarios, vec!["global-scenario"]); + let _ = fs::remove_dir_all(root); +} + +#[test] +fn features_test_filters_scenarios_after_feature_selection() { + let root = unique_temp_dir(); + let demo_src = root.join("src").join("demo"); + let other_src = root.join("src").join("other"); + let demo_test = root.join("test").join("demo"); + let other_test = root.join("test").join("other"); + fs::create_dir_all(&demo_src).expect("demo feature src"); + fs::create_dir_all(&other_src).expect("other feature src"); + fs::create_dir_all(&demo_test).expect("demo feature test"); + fs::create_dir_all(&other_test).expect("other feature test"); + fs::write( + demo_src.join("devcontainer-feature.json"), + "{\n \"id\": \"demo\",\n \"name\": \"Demo Feature\",\n \"version\": \"1.0.0\"\n}\n", + ) + .expect("demo manifest"); + fs::write( + other_src.join("devcontainer-feature.json"), + "{\n \"id\": \"other\",\n \"name\": \"Other Feature\",\n \"version\": \"1.0.0\"\n}\n", + ) + .expect("other manifest"); + fs::write(demo_test.join("test.sh"), "#!/bin/sh\n").expect("demo test script"); + fs::write(demo_test.join("alpha.sh"), "#!/bin/sh\n").expect("alpha scenario script"); + fs::write(demo_test.join("beta.sh"), "#!/bin/sh\n").expect("beta scenario script"); + fs::write( + demo_test.join("scenarios.json"), + "{\n \"alpha\": {\n \"image\": \"ubuntu:latest\"\n },\n \"beta\": {\n \"image\": \"ubuntu:latest\"\n }\n}\n", + ) + .expect("demo scenarios"); + fs::write(other_test.join("test.sh"), "#!/bin/sh\n").expect("other test script"); + + let scenarios = discover_feature_test_scenarios(&[ + "-f".to_string(), + "demo".to_string(), + "--filter".to_string(), + "alp".to_string(), + root.display().to_string(), + ]) + .expect("scenario discovery"); + + assert_eq!(scenarios, vec!["alpha"]); + let _ = fs::remove_dir_all(root); +} + #[test] fn features_test_executes_scripts_inside_test_containers() { let root = unique_temp_dir(); @@ -161,6 +243,44 @@ fn features_test_executes_scripts_inside_test_containers() { let _ = fs::remove_dir_all(root); } +#[test] +fn features_test_executes_with_requested_remote_user() { + let root = unique_temp_dir(); + let src = root.join("src").join("demo"); + let test = root.join("test").join("demo"); + fs::create_dir_all(&src).expect("feature src"); + fs::create_dir_all(&test).expect("feature test"); + fs::write( + src.join("devcontainer-feature.json"), + "{\n \"id\": \"demo\",\n \"name\": \"Demo Feature\",\n \"version\": \"1.0.0\"\n}\n", + ) + .expect("manifest"); + fs::write(src.join("install.sh"), "#!/bin/sh\nexit 0\n").expect("install script"); + fs::write(test.join("test.sh"), "#!/bin/sh\nexit 0\n").expect("test script"); + + let mut runtime = FakeFeatureTestRuntime::default(); + let results = execute_feature_tests_with_runtime( + &[ + "--preserve-test-containers".to_string(), + "--remote-user".to_string(), + "vscode".to_string(), + root.display().to_string(), + ], + &mut runtime, + ) + .expect("test execution"); + + assert_eq!(results.len(), 1); + assert!(results[0].passed); + assert_eq!(runtime.exec_calls.len(), 1); + assert_eq!(runtime.exec_calls[0].2.as_deref(), Some("vscode")); + + for (_, workspace_dir) in &runtime.start_calls { + let _ = fs::remove_dir_all(workspace_dir); + } + let _ = fs::remove_dir_all(root); +} + #[test] fn features_test_defaults_per_feature_scenarios_to_enclosing_feature() { let root = unique_temp_dir(); diff --git a/cmd/devcontainer/src/commands/common.rs b/cmd/devcontainer/src/commands/common.rs index d622cc3cb..a7404a920 100644 --- a/cmd/devcontainer/src/commands/common.rs +++ b/cmd/devcontainer/src/commands/common.rs @@ -23,3 +23,44 @@ pub(crate) use labels::{ DEVCONTAINER_CONFIG_FILE_LABEL, DEVCONTAINER_LOCAL_FOLDER_LABEL, }; pub(crate) use manifest::{generate_manifest_docs, parse_manifest, ManifestDocOptions}; + +pub(crate) fn feature_option_env_name(key: &str) -> String { + let mut normalized = key + .chars() + .map(|character| { + if character.is_ascii_alphanumeric() || character == '_' { + character.to_ascii_uppercase() + } else { + '_' + } + }) + .collect::(); + + let leading_unsafe_len = normalized + .chars() + .take_while(|character| character.is_ascii_digit() || *character == '_') + .map(char::len_utf8) + .sum::(); + if leading_unsafe_len > 0 { + normalized.replace_range(..leading_unsafe_len, "_"); + } + + normalized +} + +#[cfg(test)] +mod tests { + use super::feature_option_env_name; + + #[test] + fn feature_option_env_names_match_upstream_safe_id_cases() { + assert_eq!(feature_option_env_name("option-name"), "OPTION_NAME"); + assert_eq!( + feature_option_env_name("option1-name-with_dashes-"), + "OPTION1_NAME_WITH_DASHES_" + ); + assert_eq!(feature_option_env_name("myOptionName"), "MYOPTIONNAME"); + assert_eq!(feature_option_env_name("1name"), "_NAME"); + assert_eq!(feature_option_env_name("12345_option-name"), "_OPTION_NAME"); + } +} diff --git a/cmd/devcontainer/src/commands/configuration/features/options.rs b/cmd/devcontainer/src/commands/configuration/features/options.rs index bd8b953f0..6ccb268bd 100644 --- a/cmd/devcontainer/src/commands/configuration/features/options.rs +++ b/cmd/devcontainer/src/commands/configuration/features/options.rs @@ -2,6 +2,8 @@ use serde_json::{Map, Value}; +use crate::commands::common; + pub(super) fn feature_object(manifest: &Value, options: &Value, value: &Value) -> Value { let mut feature = manifest.as_object().cloned().unwrap_or_default(); feature.insert("options".to_string(), options.clone()); @@ -21,7 +23,12 @@ pub(super) fn feature_option_values_from_manifest( ) -> Vec<(String, String)> { merged_feature_options(manifest, value) .into_iter() - .map(|(key, value)| (feature_option_env_name(&key), json_value_to_env(&value))) + .map(|(key, value)| { + ( + common::feature_option_env_name(&key), + json_value_to_env(&value), + ) + }) .collect() } @@ -87,18 +94,6 @@ fn migrate_legacy_customizations(feature: &mut Map) { } } -fn feature_option_env_name(key: &str) -> String { - key.chars() - .map(|character| { - if character.is_ascii_alphanumeric() { - character.to_ascii_uppercase() - } else { - '_' - } - }) - .collect() -} - fn json_value_to_env(value: &Value) -> String { match value { Value::Null => String::new(), @@ -108,3 +103,54 @@ fn json_value_to_env(value: &Value) -> String { _ => value.to_string(), } } + +#[cfg(test)] +mod tests { + use serde_json::json; + + use crate::commands::common; + + use super::feature_option_values_from_manifest; + + #[test] + fn feature_option_env_names_match_upstream_safe_id_cases() { + assert_eq!( + common::feature_option_env_name("option-name"), + "OPTION_NAME" + ); + assert_eq!( + common::feature_option_env_name("option1-name-with_dashes-"), + "OPTION1_NAME_WITH_DASHES_" + ); + assert_eq!( + common::feature_option_env_name("myOptionName"), + "MYOPTIONNAME" + ); + assert_eq!(common::feature_option_env_name("1name"), "_NAME"); + assert_eq!( + common::feature_option_env_name("12345_option-name"), + "_OPTION_NAME" + ); + } + + #[test] + fn feature_option_values_use_safe_env_names_for_defaults_and_overrides() { + let manifest = json!({ + "id": "demo", + "options": { + "1name": { "type": "string", "default": "default-value" }, + "option-name": { "type": "string", "default": "default-option" } + } + }); + let values = feature_option_values_from_manifest( + &manifest, + &json!({ + "1name": "override-value" + }), + ); + + assert!(values.contains(&("_NAME".to_string(), "override-value".to_string()))); + assert!(values.contains(&("OPTION_NAME".to_string(), "default-option".to_string()))); + assert!(!values.iter().any(|(key, _)| key == "1NAME")); + } +}