From f71bb351d29f58935e6002615cd94d6e6259bf26 Mon Sep 17 00:00:00 2001
From: Ryan Lahfa <tvl@lahfa.xyz>
Date: Sun, 14 Jan 2024 01:41:16 +0100
Subject: feat(tvix/glue): introduce test suite for context strings

This is an additional test suite on the top of the Nix ones
for context strings matters.

It already smoked out multiple mistakes and potential bugs and non-deterministic result from the evaluator.

It uses a similar technology as the one in the tvix-eval albeit we instantiate a fully fledged evaluator
with in-memory store.

We copy the files instead of symlinking them because crates are built in
isolation, so symlinks cannot work.

Change-Id: I63ae225ce4f83c6e2c8ccd60d779c2f8eb9d08fb
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10619
Autosubmit: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
---
 tvix/glue/Cargo.toml                               |   8 ++
 tvix/glue/src/lib.rs                               |   3 +
 tvix/glue/src/tests/mod.rs                         | 140 +++++++++++++++++++++
 .../glue/src/tests/nix_tests/eval-okay-context.exp |   1 +
 .../glue/src/tests/nix_tests/eval-okay-context.nix |   6 +
 .../eval-okay-context-introspection.exp            |   1 +
 .../eval-okay-context-introspection.nix            |  42 +++++++
 .../tvix_tests/eval-okay-context-introspection.exp |   1 +
 .../tvix_tests/eval-okay-context-introspection.nix |  72 +++++++++++
 .../tvix_tests/eval-okay-context-propagation.exp   |   1 +
 .../tvix_tests/eval-okay-context-propagation.nix   |  88 +++++++++++++
 11 files changed, 363 insertions(+)
 create mode 100644 tvix/glue/src/tests/mod.rs
 create mode 100644 tvix/glue/src/tests/nix_tests/eval-okay-context.exp
 create mode 100644 tvix/glue/src/tests/nix_tests/eval-okay-context.nix
 create mode 100644 tvix/glue/src/tests/nix_tests/notyetpassing/eval-okay-context-introspection.exp
 create mode 100644 tvix/glue/src/tests/nix_tests/notyetpassing/eval-okay-context-introspection.nix
 create mode 100644 tvix/glue/src/tests/tvix_tests/eval-okay-context-introspection.exp
 create mode 100644 tvix/glue/src/tests/tvix_tests/eval-okay-context-introspection.nix
 create mode 100644 tvix/glue/src/tests/tvix_tests/eval-okay-context-propagation.exp
 create mode 100644 tvix/glue/src/tests/tvix_tests/eval-okay-context-propagation.nix

(limited to 'tvix/glue')

diff --git a/tvix/glue/Cargo.toml b/tvix/glue/Cargo.toml
index c58c66dd60a1..451b28f1f0a0 100644
--- a/tvix/glue/Cargo.toml
+++ b/tvix/glue/Cargo.toml
@@ -25,9 +25,17 @@ git = "https://github.com/tvlfyi/wu-manber.git"
 [dev-dependencies]
 criterion = { version = "0.5", features = ["html_reports"] }
 lazy_static = "1.4.0"
+pretty_assertions = "1.4.0"
+rstest = "0.18.2"
 tempfile = "3.8.1"
 test-case = "3.3.1"
 
+[features]
+default = ["nix_tests"]
+# Enables running the Nix language test suite from the original C++
+# Nix implementation (at version 2.3) against Tvix.
+nix_tests = []
+
 [[bench]]
 name = "eval"
 harness = false
diff --git a/tvix/glue/src/lib.rs b/tvix/glue/src/lib.rs
index 2b36a0b890bb..31bcbede37d4 100644
--- a/tvix/glue/src/lib.rs
+++ b/tvix/glue/src/lib.rs
@@ -5,6 +5,9 @@ pub mod tvix_build;
 pub mod tvix_io;
 pub mod tvix_store_io;
 
+#[cfg(test)]
+mod tests;
+
 /// Tell the Evaluator to resolve `<nix>` to the path `/__corepkgs__`,
 /// which has special handling in [tvix_io::TvixIO].
 /// This is used in nixpkgs to import `fetchurl.nix` from `<nix>`.
diff --git a/tvix/glue/src/tests/mod.rs b/tvix/glue/src/tests/mod.rs
new file mode 100644
index 000000000000..9ae17c219ac9
--- /dev/null
+++ b/tvix/glue/src/tests/mod.rs
@@ -0,0 +1,140 @@
+use std::{cell::RefCell, rc::Rc, sync::Arc};
+
+use pretty_assertions::assert_eq;
+use std::path::PathBuf;
+use tvix_castore::{
+    blobservice::{BlobService, MemoryBlobService},
+    directoryservice::{DirectoryService, MemoryDirectoryService},
+};
+use tvix_eval::Value;
+use tvix_store::pathinfoservice::{MemoryPathInfoService, PathInfoService};
+
+use rstest::rstest;
+
+use crate::{
+    builtins::add_derivation_builtins, known_paths::KnownPaths, tvix_store_io::TvixStoreIO,
+};
+
+fn eval_test(code_path: PathBuf, expect_success: bool) {
+    assert_eq!(
+        code_path.extension().unwrap(),
+        "nix",
+        "test files always end in .nix"
+    );
+    let exp_path = code_path.with_extension("exp");
+    let exp_xml_path = code_path.with_extension("exp.xml");
+
+    let code = std::fs::read_to_string(&code_path).expect("should be able to read test code");
+
+    if exp_xml_path.exists() {
+        // We can't test them at the moment because we don't have XML output yet.
+        // Checking for success / failure only is a bit disingenious.
+        return;
+    }
+
+    let mut eval = tvix_eval::Evaluation::new_impure();
+
+    let blob_service = Arc::new(MemoryBlobService::default()) as Arc<dyn BlobService>;
+    let directory_service =
+        Arc::new(MemoryDirectoryService::default()) as Arc<dyn DirectoryService>;
+    let path_info_service = Box::new(MemoryPathInfoService::new(
+        blob_service.clone(),
+        directory_service.clone(),
+    )) as Box<dyn PathInfoService>;
+    let runtime = tokio::runtime::Runtime::new().unwrap();
+
+    eval.io_handle = Box::new(TvixStoreIO::new(
+        blob_service,
+        directory_service,
+        path_info_service,
+        runtime.handle().clone(),
+    ));
+
+    let known_paths: Rc<RefCell<KnownPaths>> = Default::default();
+
+    eval.strict = true;
+    add_derivation_builtins(&mut eval, known_paths.clone());
+
+    let result = eval.evaluate(code, Some(code_path.clone()));
+    let failed = match result.value {
+        Some(Value::Catchable(_)) => true,
+        _ => !result.errors.is_empty(),
+    };
+    if expect_success && failed {
+        panic!(
+            "{}: evaluation of eval-okay test should succeed, but failed with {:?}",
+            code_path.display(),
+            result.errors,
+        );
+    }
+
+    if !expect_success && failed {
+        return;
+    }
+
+    let value = result.value.unwrap();
+    let result_str = value.to_string();
+
+    if let Ok(exp) = std::fs::read_to_string(exp_path) {
+        if expect_success {
+            assert_eq!(
+                result_str,
+                exp.trim(),
+                "{}: result value representation (left) must match expectation (right)",
+                code_path.display()
+            );
+        } else {
+            assert_ne!(
+                result_str,
+                exp.trim(),
+                "{}: test passed unexpectedly!  consider moving it out of notyetpassing",
+                code_path.display()
+            );
+        }
+    } else if expect_success {
+        panic!(
+            "{}: should be able to read test expectation",
+            code_path.display()
+        );
+    } else {
+        panic!(
+            "{}: test should have failed, but succeeded with output {}",
+            code_path.display(),
+            result_str
+        );
+    }
+}
+
+// eval-okay-* tests contain a snippet of Nix code, and an expectation
+// of the produced string output of the evaluator.
+//
+// These evaluations are always supposed to succeed, i.e. all snippets
+// are guaranteed to be valid Nix code.
+#[rstest]
+fn eval_okay(#[files("src/tests/tvix_tests/eval-okay-*.nix")] code_path: PathBuf) {
+    eval_test(code_path, true)
+}
+
+// eval-okay-* tests from the original Nix test suite.
+#[cfg(feature = "nix_tests")]
+#[rstest]
+fn nix_eval_okay(#[files("src/tests/nix_tests/eval-okay-*.nix")] code_path: PathBuf) {
+    eval_test(code_path, true)
+}
+
+// eval-okay-* tests from the original Nix test suite which do not yet pass for tvix
+//
+// Eventually there will be none of these left, and this function
+// will disappear :) Until then, to run these tests, use `cargo test
+// --features expected_failures`.
+//
+// Please don't submit failing tests unless they're in
+// notyetpassing; this makes the test suite much more useful for
+// regression testing, since there should always be zero non-ignored
+// failing tests.
+#[rstest]
+fn nix_eval_okay_currently_failing(
+    #[files("src/tests/nix_tests/notyetpassing/eval-okay-*.nix")] code_path: PathBuf,
+) {
+    eval_test(code_path, false)
+}
diff --git a/tvix/glue/src/tests/nix_tests/eval-okay-context.exp b/tvix/glue/src/tests/nix_tests/eval-okay-context.exp
new file mode 100644
index 000000000000..2f535bdbc454
--- /dev/null
+++ b/tvix/glue/src/tests/nix_tests/eval-okay-context.exp
@@ -0,0 +1 @@
+"foo eval-okay-context.nix bar"
diff --git a/tvix/glue/src/tests/nix_tests/eval-okay-context.nix b/tvix/glue/src/tests/nix_tests/eval-okay-context.nix
new file mode 100644
index 000000000000..c8732118628a
--- /dev/null
+++ b/tvix/glue/src/tests/nix_tests/eval-okay-context.nix
@@ -0,0 +1,6 @@
+let s = "foo ${builtins.substring 33 100 (baseNameOf "${./eval-okay-context.nix}")} bar";
+in
+if s != "foo eval-okay-context.nix bar"
+then abort "context not discarded"
+else builtins.unsafeDiscardStringContext s
+
diff --git a/tvix/glue/src/tests/nix_tests/notyetpassing/eval-okay-context-introspection.exp b/tvix/glue/src/tests/nix_tests/notyetpassing/eval-okay-context-introspection.exp
new file mode 100644
index 000000000000..03b400cc8862
--- /dev/null
+++ b/tvix/glue/src/tests/nix_tests/notyetpassing/eval-okay-context-introspection.exp
@@ -0,0 +1 @@
+[ true true true true true true ]
diff --git a/tvix/glue/src/tests/nix_tests/notyetpassing/eval-okay-context-introspection.nix b/tvix/glue/src/tests/nix_tests/notyetpassing/eval-okay-context-introspection.nix
new file mode 100644
index 000000000000..354376b89535
--- /dev/null
+++ b/tvix/glue/src/tests/nix_tests/notyetpassing/eval-okay-context-introspection.nix
@@ -0,0 +1,42 @@
+let
+  drv = derivation {
+    name = "fail";
+    builder = "/bin/false";
+    system = "x86_64-linux";
+    outputs = [ "out" "foo" ];
+  };
+
+  path = "${./eval-okay-context-introspection.nix}";
+
+  desired-context = {
+    "${builtins.unsafeDiscardStringContext path}" = {
+      path = true;
+    };
+    "${builtins.unsafeDiscardStringContext drv.drvPath}" = {
+      outputs = [ "foo" "out" ];
+      allOutputs = true;
+    };
+  };
+
+  combo-path = "${path}${drv.outPath}${drv.foo.outPath}${drv.drvPath}";
+  legit-context = builtins.getContext combo-path;
+
+  reconstructed-path = builtins.appendContext
+    (builtins.unsafeDiscardStringContext combo-path)
+    desired-context;
+
+  # Eta rule for strings with context.
+  etaRule = str:
+    str == builtins.appendContext
+      (builtins.unsafeDiscardStringContext str)
+      (builtins.getContext str);
+
+in
+[
+  (legit-context == desired-context)
+  (reconstructed-path == combo-path)
+  (etaRule "foo")
+  (etaRule drv.drvPath)
+  (etaRule drv.foo.outPath)
+  (etaRule (builtins.unsafeDiscardOutputDependency drv.drvPath))
+]
diff --git a/tvix/glue/src/tests/tvix_tests/eval-okay-context-introspection.exp b/tvix/glue/src/tests/tvix_tests/eval-okay-context-introspection.exp
new file mode 100644
index 000000000000..3f8905e9bd0d
--- /dev/null
+++ b/tvix/glue/src/tests/tvix_tests/eval-okay-context-introspection.exp
@@ -0,0 +1 @@
+[ true true true true true true true true true true true ]
diff --git a/tvix/glue/src/tests/tvix_tests/eval-okay-context-introspection.nix b/tvix/glue/src/tests/tvix_tests/eval-okay-context-introspection.nix
new file mode 100644
index 000000000000..ed78112ab7eb
--- /dev/null
+++ b/tvix/glue/src/tests/tvix_tests/eval-okay-context-introspection.nix
@@ -0,0 +1,72 @@
+# Contrary to the Nix tests, this one does not make any use of `builtins.appendContext`
+# It's a weaker yet interesting test by abusing knowledge on how does our builtins
+# performs propagation.
+let
+  drv = derivation {
+    name = "fail";
+    builder = "/bin/false";
+    system = "x86_64-linux";
+    outputs = [ "out" "foo" ];
+  };
+
+  # `substr` propagates context, we truncate to an empty string and concatenate to the target
+  # to infect it with the context of `copied`.
+  appendContextFrom = copied: target: (builtins.substring 0 0 "${copied}") + "${target}";
+
+  # `split` discards (!!) contexts, we first split by `/` (there's at least one such `/` by
+  # virtue of `target` being a store path, i.e. starting with `$store_root/$derivation_name`)
+  # then, we reassemble the list into a proper string.
+  discardContext = target: builtins.concatStringsSep "" (builtins.split "(.*)" "${target}");
+
+  hasContextInAttrKeys = attrs: builtins.all builtins.hasContext (builtins.attrNames attrs);
+
+  path = "${./eval-okay-context-introspection.nix}";
+
+  # This is a context-less attribute set, which should be exactly the same
+  # as `builtins.getContext combo-path`.
+  desired-context = {
+    "${builtins.unsafeDiscardStringContext path}" = {
+      path = true;
+    };
+    "${builtins.unsafeDiscardStringContext drv.drvPath}" = {
+      outputs = [ "foo" "out" ];
+      allOutputs = true;
+    };
+  };
+
+  combo-path = "${path}${drv.outPath}${drv.foo.outPath}${drv.drvPath}";
+  legit-context = builtins.getContext combo-path;
+
+  reconstructed-path = appendContextFrom combo-path
+    (builtins.unsafeDiscardStringContext combo-path);
+
+  # Eta rule for strings with context.
+  etaRule = str:
+    str == appendContextFrom
+      str
+      (builtins.unsafeDiscardStringContext str);
+
+  etaRule' = str:
+    str == appendContextFrom
+      str
+      (discardContext str);
+
+in
+[
+  (!hasContextInAttrKeys desired-context)
+  (legit-context."${builtins.unsafeDiscardStringContext path}".path)
+  (legit-context."${builtins.unsafeDiscardStringContext drv.drvPath}".outputs == [ "foo" "out" ])
+  # `allOutputs` is present only on DrvClosure-style context string, i.e. the
+  # context string of a drvPath itself, not an outPath.
+  (!builtins.hasAttr "allOutputs" (builtins.getContext drv.outPath)."${builtins.unsafeDiscardStringContext drv.drvPath}")
+  (builtins.hasAttr "allOutputs" legit-context."${builtins.unsafeDiscardStringContext drv.drvPath}")
+  (builtins.hasAttr "allOutputs" (builtins.getContext drv.drvPath)."${builtins.unsafeDiscardStringContext drv.drvPath}")
+  (legit-context == desired-context) # FIXME(raitobezarius): this should not use `builtins.seq`, this is a consequence of excessive laziness of Tvix, I believe.
+  (reconstructed-path == combo-path)
+  # Those are too slow?
+  # (etaRule' "foo")
+  # (etaRule' combo-path)
+  (etaRule "foo")
+  (etaRule drv.drvPath)
+  (etaRule drv.foo.outPath)
+]
diff --git a/tvix/glue/src/tests/tvix_tests/eval-okay-context-propagation.exp b/tvix/glue/src/tests/tvix_tests/eval-okay-context-propagation.exp
new file mode 100644
index 000000000000..71c6ded40612
--- /dev/null
+++ b/tvix/glue/src/tests/tvix_tests/eval-okay-context-propagation.exp
@@ -0,0 +1 @@
+[ true true true true true true true true true true true true true true true true true true true true true true true true true true ]
diff --git a/tvix/glue/src/tests/tvix_tests/eval-okay-context-propagation.nix b/tvix/glue/src/tests/tvix_tests/eval-okay-context-propagation.nix
new file mode 100644
index 000000000000..67f0ac46729b
--- /dev/null
+++ b/tvix/glue/src/tests/tvix_tests/eval-okay-context-propagation.nix
@@ -0,0 +1,88 @@
+# We test various propagation of contexts under other builtins here.
+let
+  drv = derivation {
+    name = "fail";
+    builder = "/bin/false";
+    system = "x86_64-linux";
+    outputs = [ "out" "foo" ];
+  };
+  other-drv = derivation {
+    name = "other-fail";
+    builder = "/bin/false";
+    system = "x86_64-linux";
+    outputs = [ "out" "bar" ];
+  };
+
+  # `substr` propagates context, we truncate to an empty string and concatenate to the target
+  # to infect it with the context of `copied`.
+  appendContextFrom = copied: target: (builtins.substring 0 0 copied) + target;
+
+  path = "${./eval-okay-context-introspection.nix}";
+
+  combo-path = "${path}${drv.outPath}${drv.foo.outPath}${drv.drvPath}";
+
+  mergeContext = a: b:
+    builtins.getContext a // builtins.getContext b;
+
+  preserveContext = origin: result:
+    builtins.getContext "${result}" == builtins.getContext "${origin}";
+
+  preserveContexts = origins: result:
+    let union = builtins.foldl' (x: y: x // y) { } (builtins.map (d: builtins.getContext "${d}") origins);
+    in
+    union == builtins.getContext "${result}";
+in
+[
+  # `toFile` should produce context.
+  (builtins.hasContext "${(builtins.toFile "myself" "${./eval-okay-context-introspection.nix}")}")
+  # `derivation` should produce context.
+  (builtins.hasContext "${drv}")
+  # Low-level test to ensure that interpolation is working as expected.
+  (builtins.length (builtins.attrNames (builtins.getContext "${drv}${other-drv}")) == 2)
+  (builtins.getContext "${drv}${other-drv}" == mergeContext drv other-drv)
+  # Those three next tests are extremely related.
+  # To test interpolation, we need concatenation to be working and vice versa.
+  # In addition, we need `builtins.substring` empty string propagation to attach context
+  # in absence of `builtins.appendContext`.
+  # The previous test should ensure that we don't test vacuous truths.
+  # Substring preserves contexts.
+  (preserveContext combo-path (builtins.substring 0 0 combo-path)) # <- FIXME: broken
+  # Interpolation preserves contexts.
+  (preserveContext "${drv}${other-drv}" (appendContextFrom drv other-drv))
+  # Concatenation preserves contexts.
+  (preserveContext "${drv}${other-drv}" (drv + other-drv))
+  # Special case when Nix does not assert that the length argument is non-negative
+  # when the starting index is ≥ than the string's length.
+  # FIXME: those three are broken too, NON DETERMINISTIC!!!
+  (preserveContext combo-path (builtins.substring 5 (-5) (builtins.substring 0 0 combo-path)))
+  (preserveContext combo-path (toString combo-path))
+  # No replacement should yield at least the same context.
+  (preserveContext combo-path (builtins.replaceStrings [ ] [ ] combo-path))
+  # This is an idempotent replacement, it should yield therefore to full preservation of the context.
+  (preserveContext "${drv}${drv}" (builtins.replaceStrings [ "${drv}" ] [ "${drv}" ] "${drv}"))
+  # There's no context here, so no context should appear from `drv`.
+  (preserveContext "abc" (builtins.replaceStrings [ "${drv}" ] [ "${drv}" ] "abc"))
+  # Context should appear by a successful replacement.
+  (preserveContext "${drv}" (builtins.replaceStrings [ "a" ] [ "${drv}" ] "a"))
+  # We test multiple successful replacements.
+  (preserveContexts [ drv other-drv ] (builtins.replaceStrings [ "a" "b" ] [ "${drv}" "${other-drv}" ] "ab"))
+  # We test *empty* string replacements.
+  (preserveContext "${drv}" (builtins.replaceStrings [ "" ] [ "${drv}" ] "abc"))
+  (preserveContext "${drv}" (builtins.replaceStrings [ "" ] [ "${drv}" ] ""))
+  # There should be no context in a parsed derivation name.
+  (!builtins.any builtins.hasContext (builtins.attrValues (builtins.parseDrvName "${drv.name}")))
+  # Nix does not propagate contexts for `match`.
+  (!builtins.any builtins.hasContext (builtins.match "(.*)" "${drv}"))
+  # `dirOf` preserves contexts of non-paths.
+  (preserveContext "${drv}" (builtins.dirOf "${drv}"))
+  (preserveContext "abc" (builtins.dirOf "abc"))
+  # `baseNameOf propagates context of argument
+  (preserveContext "${drv}" (builtins.baseNameOf drv))
+  (preserveContext "abc" (builtins.baseNameOf "abc"))
+  # `concatStringsSep` preserves contexts of both arguments.
+  (preserveContexts [ drv other-drv ] (builtins.concatStringsSep "${other-drv}" (map toString [ drv drv drv drv drv ])))
+  (preserveContext drv (builtins.concatStringsSep "|" (map toString [ drv drv drv drv drv ])))
+  (preserveContext other-drv (builtins.concatStringsSep "${other-drv}" [ "abc" "def" ]))
+  # `attrNames` will never ever produce context.
+  (preserveContext "abc" (toString (builtins.attrNames { a = { }; b = { }; c = { }; })))
+]
-- 
cgit 1.4.1