Improve feature test coverage parity

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::<Vec<_>>()
         })
@@ -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::<Vec<_>>()
         })
         .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);
+    }
 }

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();

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::<String>();
+
+    let leading_unsafe_len = normalized
+        .chars()
+        .take_while(|character| character.is_ascii_digit() || *character == '_')
+        .map(char::len_utf8)
+        .sum::<usize>();
+    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");
+    }
+}

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<String, Value>) {
     }
 }
 
-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"));
+    }
+}