Repository: denoland/deno
Pull request: feat(internal): support variants of spec tests

Adds support for a new key `"variants"` that effectively parameterizes a test, allowing you to substitute different values for each scenario. For instance, I'm planning on using this in the tsgo PR, where I'll have tests like

```json
{
  "variants": {
    "tsgo": {
      "maybe_unstable_tsgo": "--unstable-tsgo"
    },
    "tsjs": {
      "maybe_unstable_tsgo": ""
    }
  },
  "steps": [
    {
      "args": "install",
      "output": "[WILDCARD]"
    },
    {
      "args": "check ${maybe_unstable_tsgo} main.ts",
      "output": "check.out"
    }
  ]
}
```
This effectively expands into two separate tests: one called `test_name::tsgo` which runs `check --unstable-tsgo main.ts`, and another called `test_name::tsjs` which runs `check main.ts`

I imagine this to be useful to run tests under the various `--node-modules` modes as well

The way it works is that it generates a test for each variant, and then in each variant the values specified for each variant are substituted whenever you refer to them with `${name}` (I added the curly braces to make it unambiguous when you have variables that are prefixes of one another). Right now that substitution only applies to the `"output"` key (so you can have different output expectations for different variants) and `"args"`, but we can easily expand that

Implementation is not particularly efficient, but it's good enough for now
  

Changed files:
filename: tests/specs/mod.rs
patch: @@ -5,6 +5,7 @@ use std::collections::BTreeMap;
 use std::collections::HashMap;
 use std::collections::HashSet;
 use std::panic::AssertUnwindSafe;
+use std::path::Path;
 use std::rc::Rc;
 
 use anyhow::Context;
@@ -57,13 +58,15 @@ struct MultiTestMetaData {
   pub tests: BTreeMap<String, JsonMap>,
   #[serde(default)]
   pub ignore: bool,
+  #[serde(default)]
+  pub variants: BTreeMap<String, JsonMap>,
 }
 
 impl MultiTestMetaData {
   pub fn into_collected_tests(
     mut self,
     parent_test: &CollectedTest,
-  ) -> Vec<CollectedTest<serde_json::Value>> {
+  ) -> Vec<CollectedCategoryOrTest<serde_json::Value>> {
     fn merge_json_value(
       multi_test_meta_data: &MultiTestMetaData,
       value: &mut JsonMap,
@@ -94,6 +97,18 @@ impl MultiTestMetaData {
       if multi_test_meta_data.ignore && !value.contains_key("ignore") {
         value.insert("ignore".to_string(), true.into());
       }
+      if !multi_test_meta_data.variants.is_empty() {
+        if !value.contains_key("variants") {
+          value.insert("variants".to_string(), JsonMap::default().into());
+        }
+        let variants_obj =
+          value.get_mut("variants").unwrap().as_object_mut().unwrap();
+        for (key, value) in &multi_test_meta_data.variants {
+          if !variants_obj.contains_key(key) {
+            variants_obj.insert(key.into(), value.clone().into());
+          }
+        }
+      }
     }
 
     let mut collected_tests = Vec::with_capacity(self.tests.len());
@@ -105,8 +120,24 @@ impl MultiTestMetaData {
         data: serde_json::Value::Object(json_data),
       });
     }
-
-    collected_tests
+    let mut all_tests = Vec::with_capacity(collected_tests.len());
+    for test in collected_tests {
+      if let Some(variants) = test
+        .data
+        .as_object()
+        .and_then(|o| o.get("variants"))
+        .and_then(|v| v.as_object())
+        && !variants.is_empty()
+      {
+        all_tests.push(
+          map_variants(&test.data, &test.path, &test.name, variants.iter())
+            .unwrap(),
+        );
+      } else {
+        all_tests.push(CollectedCategoryOrTest::Test(test));
+      }
+    }
+    all_tests
   }
 }
 
@@ -145,6 +176,8 @@ struct MultiStepMetaData {
   pub steps: Vec<StepMetaData>,
   #[serde(default)]
   pub ignore: bool,
+  #[serde(default)]
+  pub variants: BTreeMap<String, JsonMap>,
 }
 
 #[derive(Clone, Deserialize)]
@@ -164,6 +197,9 @@ struct SingleTestMetaData {
   pub step: StepMetaData,
   #[serde(default)]
   pub ignore: bool,
+  #[allow(dead_code)]
+  #[serde(default)]
+  pub variants: BTreeMap<String, JsonMap>,
 }
 
 impl SingleTestMetaData {
@@ -180,6 +216,7 @@ impl SingleTestMetaData {
       envs: Default::default(),
       steps: vec![self.step],
       ignore: self.ignore,
+      variants: Default::default(),
     }
   }
 }
@@ -201,6 +238,8 @@ struct StepMetaData {
   pub output: String,
   #[serde(default)]
   pub exit_code: i32,
+  #[serde(default)]
+  pub variants: BTreeMap<String, JsonMap>,
 }
 
 pub fn main() {
@@ -230,6 +269,40 @@ pub fn main() {
   );
 }
 
+fn map_variants<'a, I>(
+  test_data: &serde_json::Value,
+  test_path: &Path,
+  test_name: &str,
+  variants: I,
+) -> Result<CollectedCategoryOrTest<serde_json::Value>, CollectTestsError>
+where
+  I: IntoIterator<Item = (&'a String, &'a serde_json::Value)>,
+{
+  let mut children = Vec::with_capacity(2);
+  for (variant_name, variant_data) in variants {
+    let mut child_data = test_data.clone();
+    let child_obj = child_data
+      .as_object_mut()
+      .unwrap()
+      .get_mut("variants")
+      .unwrap()
+      .as_object_mut()
+      .unwrap();
+    child_obj.clear();
+    child_obj.insert(variant_name.clone(), variant_data.clone());
+    children.push(CollectedCategoryOrTest::Test(CollectedTest {
+      name: format!("{}::{}", test_name, variant_name),
+      path: test_path.to_path_buf(),
+      data: child_data,
+    }));
+  }
+  Ok(CollectedCategoryOrTest::Category(CollectedTestCategory {
+    children,
+    name: test_name.to_string(),
+    path: test_path.to_path_buf(),
+  }))
+}
+
 /// Maps a __test__.jsonc file to a category of tests if it contains a "test" object.
 fn map_test_within_file(
   test: CollectedTest,
@@ -245,14 +318,17 @@ fn map_test_within_file(
       .with_context(|| format!("Failed deserializing {}", test_path))
       .map_err(CollectTestsError::Other)?;
     Ok(CollectedCategoryOrTest::Category(CollectedTestCategory {
-      children: data
-        .into_collected_tests(&test)
-        .into_iter()
-        .map(CollectedCategoryOrTest::Test)
-        .collect(),
+      children: data.into_collected_tests(&test),
       name: test.name,
       path: test.path,
     }))
+  } else if let Some(variants) = metadata_value
+    .as_object()
+    .and_then(|o| o.get("variants"))
+    .and_then(|v| v.as_object())
+    && !variants.is_empty()
+  {
+    map_variants(&metadata_value, &test.path, &test.name, variants.iter())
   } else {
     Ok(CollectedCategoryOrTest::Test(CollectedTest {
       name: test.name,
@@ -324,6 +400,7 @@ fn run_test_inner(
 }
 
 fn deserialize_value(metadata_value: serde_json::Value) -> MultiStepMetaData {
+  let metadata_string = metadata_value.to_string();
   // checking for "steps" leads to a more targeted error message
   // instead of when deserializing an untagged enum
   if metadata_value
@@ -336,7 +413,7 @@ fn deserialize_value(metadata_value: serde_json::Value) -> MultiStepMetaData {
     serde_json::from_value::<SingleTestMetaData>(metadata_value)
       .map(|s| s.into_multi())
   }
-  .context("Failed to parse test spec")
+  .with_context(|| format!("Failed to parse test spec: {}", metadata_string))
   .unwrap()
 }
 
@@ -426,18 +503,123 @@ fn run_flaky(action: impl Fn()) {
   action();
 }
 
+// in the future we could consider using https://docs.rs/aho_corasick to do multiple replacements at once
+// in practice, though, i suspect the numbers here will be small enough that the naive approach is fast enough
+fn variant_substitutions(
+  variants: &BTreeMap<String, JsonMap>,
+  multi_step_variants: &BTreeMap<String, JsonMap>,
+) -> Vec<(String, String)> {
+  if variants.is_empty() && multi_step_variants.is_empty() {
+    return Vec::new();
+  }
+  let mut variant = variants.values().next().cloned().unwrap_or_default();
+  let multi_step_variant = multi_step_variants
+    .values()
+    .next()
+    .cloned()
+    .unwrap_or_default();
+
+  for (name, value) in multi_step_variant {
+    if !variant.contains_key(&name) {
+      variant.insert(name, value.clone());
+    }
+  }
+
+  let mut pairs = variant
+    .into_iter()
+    .filter_map(|(name, value)| {
+      value
+        .as_str()
+        .map(|value| (format!("${{{}}}", name), value.to_string()))
+    })
+    .collect::<Vec<_>>();
+  pairs.sort_by(|a, b| a.0.cmp(&b.0).reverse());
+  pairs
+}
+
+fn substitute_variants_into_envs(
+  pairs: &Vec<(String, String)>,
+  envs: &mut HashMap<String, String>,
+) {
+  enum Update {
+    Remove(String),
+    Replace(String, String),
+  }
+  let mut updates = Vec::new();
+  for (key, value) in pairs {
+    for (k, v) in envs.iter() {
+      let replaced = v.replace(key.as_str(), value);
+      if replaced.is_empty() && &replaced != v {
+        updates.push(Update::Remove(k.clone()));
+        continue;
+      }
+      updates.push(Update::Replace(k.clone(), replaced));
+    }
+  }
+  for update in updates {
+    match update {
+      Update::Remove(key) => {
+        envs.remove(&key);
+      }
+      Update::Replace(key, value) => {
+        envs.insert(key, value);
+      }
+    }
+  }
+}
+
 fn run_step(
   step: &StepMetaData,
   metadata: &MultiStepMetaData,
   cwd: &PathRef,
   context: &test_util::TestContext,
 ) {
-  let command = context
-    .new_command()
-    .envs(metadata.envs.iter().chain(step.envs.iter()));
+  let substs = variant_substitutions(&step.variants, &metadata.variants);
+
+  let command = if substs.is_empty() {
+    let envs = metadata.envs.iter().chain(step.envs.iter());
+    context.new_command().envs(envs)
+  } else {
+    let mut envs = metadata
+      .envs
+      .iter()
+      .chain(step.envs.iter())
+      .map(|(key, value)| (key.clone(), value.clone()))
+      .collect::<HashMap<_, _>>();
+    substitute_variants_into_envs(&substs, &mut envs);
+    context.new_command().envs(envs)
+  };
+
   let command = match &step.args {
-    VecOrString::Vec(args) => command.args_vec(args),
-    VecOrString::String(text) => command.args(text),
+    VecOrString::Vec(args) => {
+      if substs.is_empty() {
+        command.args_vec(args)
+      } else {
+        let mut args_replaced = args.clone();
+        for arg in args {
+          for (from, to) in &substs {
+            let arg_replaced = arg.replace(from, to);
+            if arg_replaced.is_empty() && &arg_replaced != arg {
+              continue;
+            }
+            args_replaced.push(arg_replaced);
+          }
+        }
+
+        command.args_vec(args)
+      }
+    }
+    VecOrString::String(text) => {
+      if substs.is_empty() {
+        command.args(text)
+      } else {
+        let mut text = text.clone();
+        for (from, to) in &substs {
+          text = text.replace(from, to);
+        }
+        command.args(text)
+      }
+    }
   };
   let command = match step.cwd.as_ref().or(metadata.cwd.as_ref()) {
     Some(cwd) => command.current_dir(cwd),
@@ -465,15 +647,27 @@ fn run_step(
     None => command,
   };
   let output = command.run();
-  if step.output.ends_with(".out") {
-    let test_output_path = cwd.join(&step.output);
+
+  let step_output = {
+    if substs.is_empty() {
+      step.output.clone()
+    } else {
+      let mut output = step.output.clone();
+      for (from, to) in substs {
+        output = output.replace(&from, &to);
+      }
+      output
+    }
+  };
+  if step_output.ends_with(".out") {
+    let test_output_path = cwd.join(&step_output);
     output.assert_matches_file(test_output_path);
   } else {
     assert!(
-      step.output.len() <= 160,
+      step_output.len() <= 160,
       "The \"output\" property in your __test__.jsonc file is too long. Please extract this to an `.out` file to improve readability."
     );
-    output.assert_matches_text(&step.output);
+    output.assert_matches_text(&step_output);
   }
   output.assert_exit_code(step.exit_code);
 }

filename: tests/specs/schema.json
patch: @@ -59,6 +59,16 @@
         },
         "ignore": {
           "type": "boolean"
+        },
+        "variants": {
+          "type": "object",
+          "patternProperties": {
+            ".*": {
+              "type": "object",
+              "additionalProperties": { "type": "string" }
+            }
+          },
+          "additionalProperties": false
         }
       }
     },
@@ -107,6 +117,16 @@
           },
           "ignore": {
             "type": "boolean"
+          },
+          "variants": {
+            "type": "object",
+            "patternProperties": {
+              ".*": {
+                "type": "object",
+                "additionalProperties": { "type": "string" }
+              }
+            },
+            "additionalProperties": false
           }
         }
       }, {
@@ -165,6 +185,16 @@
         },
         "ignore": {
           "type": "boolean"
+        },
+        "variants": {
+          "type": "object",
+          "patternProperties": {
+            ".*": {
+              "type": "object",
+              "additionalProperties": { "type": "string" }
+            }
+          },
+          "additionalProperties": false
         }
       }
     }