Fix windows tests (#2025)

* Fix benchmarks on Windows

* Fix tests for Windows

+ Improve failure messages

* Add CI job for Windows

* Disable unused warning for ParseFormatError

* Add `pprof` feature to utils

.github/workflows/continuous-integration.yml

@@ -83,3 +83,18 @@ jobs:
 
     - name: Typecheck benchmarks
       run: find core/benches -type f -name "*.ncl" -print0 | xargs -0 -I file nix run . -- typecheck file
+
+  build-and-test-windows:
+    name: "build-and-test (windows-latest, stable)"
+    runs-on: windows-latest
+    continue-on-error: true
+    steps:
+      - uses: actions/checkout@v4
+
+      - uses: actions-rust-lang/setup-rust-toolchain@v1
+
+      - name: Build
+        run: cargo build --all-targets --package nickel-lang-*
+
+      - name: Test
+        run: cargo test --package nickel-lang-*

Cargo.lock

@@ -1774,7 +1774,6 @@ dependencies = [
  "nickel-lang-utils",
  "once_cell",
  "pkg-config",
- "pprof",
  "pretty",
  "pretty_assertions",
  "regex",
@@ -1848,6 +1847,7 @@ dependencies = [
  "codespan",
  "criterion",
  "nickel-lang-core",
+ "pprof",
  "serde",
  "toml",
 ]

cli/tests/snapshot/main.rs

@@ -16,8 +16,9 @@ macro_rules! assert_snapshot_filtered {
             // Since error output includes fully-qualified paths to the source file
             // we need to replace those with something static to avoid snapshots
             // differing across machines.
-            (r"(?:/.+/tests/snapshot/inputs)", "[INPUTS_PATH]"),
-            (r"(?:/.+/tests/snapshot/imports)", "[IMPORTS_PATH]")
+            // Replace filepath backslashes with forward slashes for Windows.
+            (r"(?:(?:.:)?(?:/|\\).+(?:/|\\)tests(?:/|\\)snapshot(?:/|\\)inputs(?:/|\\))([0-9A-Za-z_-]+)(?:/|\\)(.+)", "[INPUTS_PATH]/${1}/${2}"),
+            (r"(?:(?:.:)?(?:/|\\).+(?:/|\\)tests(?:/|\\)snapshot(?:/|\\)imports(?:/|\\))(.+)", "[IMPORTS_PATH]/${1}")
         ]},
         {
             insta::assert_snapshot!($name, $snapshot);

core/Cargo.toml

@@ -86,8 +86,7 @@ strsim = "0.10.0"
 pretty_assertions.workspace = true
 assert_matches.workspace = true
 criterion.workspace = true
-pprof = { workspace = true, features = ["criterion", "flamegraph"] }
-nickel-lang-utils.workspace = true
+nickel-lang-utils = { workspace = true, features = ["pprof"] }
 similar.workspace = true
 test-generator.workspace = true
 

core/benches/arrays.rs

@@ -1,12 +1,11 @@
 use std::rc::Rc;
 
-use criterion::{criterion_main, Criterion};
+use criterion::criterion_main;
 use nickel_lang_core::term::{
     array::{Array, ArrayAttrs},
     Number, RichTerm, Term,
 };
-use nickel_lang_utils::{bench::EvalMode, ncl_bench_group};
-use pprof::criterion::{Output, PProfProfiler};
+use nickel_lang_utils::{bench::criterion_config, bench::EvalMode, ncl_bench_group};
 use pretty::{BoxAllocator, DocBuilder, Pretty};
 
 /// Generates a pseaudo-random Nickel array as a string.
@@ -35,7 +34,7 @@ fn ncl_random_array(len: usize) -> String {
 
 ncl_bench_group! {
 name = benches;
-config = Criterion::default().with_profiler(PProfProfiler::new(100, Output::Flamegraph(None)));
+config = criterion_config();
 {
         name = "foldr strings 50",
         path = "arrays/fold",

core/benches/functions.rs

@@ -1,10 +1,9 @@
-use criterion::{criterion_main, Criterion};
-use nickel_lang_utils::ncl_bench_group;
-use pprof::criterion::{Output, PProfProfiler};
+use criterion::criterion_main;
+use nickel_lang_utils::{bench::criterion_config, ncl_bench_group};
 
 ncl_bench_group! {
     name = benches;
-    config = Criterion::default().with_profiler(PProfProfiler::new(100, Output::Flamegraph(None)));
+    config = criterion_config();
     {
         name = "church 3",
         path = "functions/church",

core/benches/mantis.rs

@@ -1,10 +1,9 @@
-use criterion::{criterion_main, Criterion};
-use nickel_lang_utils::{bench::EvalMode, ncl_bench_group};
-use pprof::criterion::{Output, PProfProfiler};
+use criterion::criterion_main;
+use nickel_lang_utils::{bench::criterion_config, bench::EvalMode, ncl_bench_group};
 
 ncl_bench_group! {
     name = benches;
-    config = Criterion::default().with_profiler(PProfProfiler::new(100, Output::Flamegraph(None)));
+    config = criterion_config();
     {
         name = "mantis",
         path = "mantis/run",

core/benches/numeric.rs

@@ -1,10 +1,9 @@
-use criterion::{criterion_main, Criterion};
-use nickel_lang_utils::ncl_bench_group;
-use pprof::criterion::{Output, PProfProfiler};
+use criterion::criterion_main;
+use nickel_lang_utils::{bench::criterion_config, ncl_bench_group};
 
 ncl_bench_group! {
     name = benches;
-    config = Criterion::default().with_profiler(PProfProfiler::new(100, Output::Flamegraph(None)));
+    config = criterion_config();
     {
         name = "fibonacci 10",
         path = "numeric/fibonacci",

core/benches/records.rs

@@ -1,10 +1,9 @@
-use criterion::{criterion_main, Criterion};
-use nickel_lang_utils::{bench::EvalMode, ncl_bench_group};
-use pprof::criterion::{Output, PProfProfiler};
+use criterion::criterion_main;
+use nickel_lang_utils::{bench::criterion_config, bench::EvalMode, ncl_bench_group};
 
 ncl_bench_group! {
     name = benches;
-    config = Criterion::default().with_profiler(PProfProfiler::new(100, Output::Flamegraph(None)));
+    config = criterion_config();
     {
             name = "countLetters",
             path = "records/countLetters",

core/benches/serialization.rs

@@ -1,10 +1,9 @@
-use criterion::{criterion_main, Criterion};
-use nickel_lang_utils::ncl_bench_group;
-use pprof::criterion::{Output, PProfProfiler};
+use criterion::criterion_main;
+use nickel_lang_utils::{bench::criterion_config, ncl_bench_group};
 
 ncl_bench_group! {
     name = benches;
-    config = Criterion::default().with_profiler(PProfProfiler::new(100, Output::Flamegraph(None)));
+    config = criterion_config();
     {
         name = "round_trip",
         path = "serialization/main",

core/benches/stdlib.rs

@@ -1,7 +1,7 @@
 use criterion::{criterion_group, criterion_main, Criterion};
-use pprof::criterion::{Output, PProfProfiler};
 
 use nickel_lang_core::cache::{Cache, ErrorTolerance};
+use nickel_lang_utils::bench::criterion_config;
 
 pub fn typecheck_stdlib(c: &mut Criterion) {
     let mut cache = Cache::new(ErrorTolerance::Strict);
@@ -18,7 +18,7 @@ pub fn typecheck_stdlib(c: &mut Criterion) {
 
 criterion_group!(
 name = benches;
-config = Criterion::default().with_profiler(PProfProfiler::new(100, Output::Flamegraph(None)));
+config = criterion_config();
 targets = typecheck_stdlib
 );
 criterion_main!(benches);

core/benches/typecheck-nixpkgs-lib.rs

@@ -1,10 +1,9 @@
-use criterion::{criterion_main, Criterion};
-use nickel_lang_utils::{bench::EvalMode, ncl_bench_group};
-use pprof::criterion::{Output, PProfProfiler};
+use criterion::criterion_main;
+use nickel_lang_utils::{bench::criterion_config, bench::EvalMode, ncl_bench_group};
 
 ncl_bench_group! {
     name = benches;
-    config = Criterion::default().with_profiler(PProfProfiler::new(100, Output::Flamegraph(None)));
+    config = criterion_config();
     {
         name = "nixpkgs lists",
         path = "nixpkgs/lists",

core/src/serialize.rs

@@ -47,6 +47,8 @@ impl fmt::Display for ExportFormat {
     }
 }
 
+// TODO: This type is publicly exposed, but never constructed.
+#[allow(dead_code)]
 #[derive(Clone, Eq, PartialEq, Debug)]
 pub struct ParseFormatError(String);
 

lsp/lsp-harness/src/lib.rs

@@ -5,7 +5,6 @@ use std::collections::{hash_map::Entry, HashMap};
 
 use assert_cmd::prelude::CommandCargoExt;
 pub use jsonrpc::Server;
-use log::error;
 use lsp_types::{
     notification::{Notification, PublishDiagnostics},
     request::{
@@ -57,23 +56,94 @@ pub struct Requests {
     diagnostic: Option<Vec<Url>>,
 }
 
+/// Produce an absolute filepath `Url` that is safe to use in tests.
+///
+/// The `C:\` prefix on Windows is needed both to avoid `Url::from_file_path` failing due to a
+/// missing drive letter and to avoid invalid filepath errors.
+pub fn file_url_from_path(path: &str) -> Result<Url, String> {
+    assert!(path.starts_with('/'));
+
+    let path = if cfg!(unix) {
+        path.to_owned()
+    } else {
+        format!("C:\\{}", &path[1..])
+    };
+
+    Url::from_file_path(&path).map_err(|()| format!("Unable to convert filepath {path:?} into Url"))
+}
+
+#[cfg(windows)]
+fn modify_requests_uris(mut reqs: Requests) -> Requests {
+    match reqs.request.iter_mut().next() {
+        None => {}
+        Some(rs) => {
+            for req in rs.iter_mut() {
+                modify_request_uri(req);
+            }
+        }
+    }
+    match reqs.diagnostic.iter_mut().next() {
+        None => {}
+        Some(urls) => {
+            for url in urls.iter_mut() {
+                *url = file_url_from_path(url.path()).unwrap();
+            }
+        }
+    };
+    reqs
+}
+
+#[cfg(windows)]
+fn modify_request_uri(req: &mut Request) {
+    fn file_url(url: &Url) -> Url {
+        file_url_from_path(url.path()).unwrap()
+    }
+
+    match req {
+        Request::GotoDefinition(params) => {
+            params.text_document_position_params.text_document.uri =
+                file_url(&params.text_document_position_params.text_document.uri);
+        }
+        Request::References(params) => {
+            params.text_document_position.text_document.uri =
+                file_url(&params.text_document_position.text_document.uri);
+        }
+        Request::Completion(params) => {
+            params.text_document_position.text_document.uri =
+                file_url(&params.text_document_position.text_document.uri);
+        }
+        Request::Formatting(params) => {
+            params.text_document.uri = file_url(&params.text_document.uri);
+        }
+        Request::Hover(params) => {
+            params.text_document_position_params.text_document.uri =
+                file_url(&params.text_document_position_params.text_document.uri);
+        }
+        Request::Rename(params) => {
+            params.text_document_position.text_document.uri =
+                file_url(&params.text_document_position.text_document.uri);
+        }
+        Request::Symbols(params) => {
+            params.text_document.uri = file_url(&params.text_document.uri);
+        }
+    }
+}
+
 impl TestFixture {
-    pub fn parse(s: &str) -> Option<Self> {
+    pub fn parse(s: &str) -> Result<Self, String> {
         let mut header_lines = Vec::new();
         let mut content = String::new();
         let mut files = Vec::new();
         let mut push_file = |header: &[&str], content: &mut String| {
-            if header.len() > 1 {
-                error!("files can only have 1 header line");
-                return None;
-            }
-
-            let uri = Url::from_file_path(header.first()?).ok()?;
+            let uri = match header {
+                &[path] => file_url_from_path(path),
+                _ => Err(format!("Files can only have 1 header line: {header:?}")),
+            }?;
             files.push(TestFile {
                 uri,
                 contents: std::mem::take(content),
             });
-            Some(())
+            Ok::<(), String>(())
         };
 
         for line in s.lines() {
@@ -94,7 +164,7 @@ impl TestFixture {
             // The text fixture ended with a nickel file; there are no lsp
             // requests specified.
             push_file(&header_lines, &mut content)?;
-            Some(TestFixture {
+            Ok(TestFixture {
                 files,
                 reqs: Vec::new(),
                 expected_diags: Vec::new(),
@@ -105,7 +175,10 @@ impl TestFixture {
             // we expect to receive.
             let remaining = header_lines.join("\n");
             let reqs: Requests = toml::from_str(&remaining).unwrap();
-            Some(TestFixture {
+            #[cfg(windows)]
+            let reqs = modify_requests_uris(reqs);
+
+            Ok(TestFixture {
                 files,
                 reqs: reqs.request.unwrap_or_default(),
                 expected_diags: reqs.diagnostic.unwrap_or_default(),

lsp/nls/Cargo.toml

@@ -58,7 +58,7 @@ assert_cmd.workspace = true
 assert_matches.workspace = true
 criterion.workspace = true
 glob = "0.3.1"
-insta.workspace = true
+insta = { workspace = true, features = ["filters"] }
 lsp-harness.workspace = true
 nickel-lang-utils.workspace = true
 pretty_assertions.workspace = true

lsp/nls/benches/main.rs

@@ -53,7 +53,8 @@ fn test_requests(c: &mut Criterion) {
 fn benchmark_one_test(c: &mut Criterion, path: &str) {
     let full_path = project_root().join(path);
     let contents = std::fs::read_to_string(&full_path).unwrap();
-    let fixture = TestFixture::parse(&contents).unwrap();
+    let fixture =
+        TestFixture::parse(&contents).unwrap_or_else(|s| panic!("Failed parsing {path:?}: {s}"));
     let mut harness = TestHarness::new();
 
     harness.prepare_files(&fixture);

lsp/nls/tests/main.rs

@@ -3,7 +3,7 @@ use pretty_assertions::assert_eq;
 use serde_json::json;
 use test_generator::test_resources;
 
-use lsp_harness::{TestFixture, TestHarness};
+use lsp_harness::{file_url_from_path, TestFixture, TestHarness};
 
 #[test_resources("lsp/nls/tests/inputs/*.ncl")]
 fn check_snapshots(path: &str) {
@@ -23,7 +23,12 @@ fn check_snapshots(path: &str) {
     harness.drain_diagnostics(fixture.expected_diags.iter().cloned());
     let output = String::from_utf8(harness.out).unwrap();
 
+    insta::with_settings!(
+        {filters => vec![("file:///C:", "file://")]},
+        {
     insta::assert_snapshot!(path, output);
+       }
+    );
 }
 
 #[test]
@@ -31,8 +36,8 @@ fn refresh_missing_imports() {
     let _ = env_logger::try_init();
     let mut harness = TestHarness::new();
 
-    let url = |s: &str| lsp_types::Url::from_file_path(s).unwrap();
-    harness.send_file(url("/test.ncl"), "import \"dep.ncl\"");
+    let test_uri = file_url_from_path("/test.ncl").unwrap();
+    harness.send_file(test_uri.clone(), "import \"dep.ncl\"");
     let diags = harness.wait_for_diagnostics().diagnostics;
     assert_eq!(2, diags.len());
     assert!(diags[0].message.contains("import of dep.ncl failed"));
@@ -43,7 +48,8 @@ fn refresh_missing_imports() {
     assert!(diags[0].message.contains("import of dep.ncl failed"));
 
     // Now provide the import.
-    harness.send_file(url("/dep.ncl"), "42");
+    let dep_uri = file_url_from_path("/dep.ncl").unwrap();
+    harness.send_file(dep_uri.clone(), "42");
 
     // Check that we get back clean diagnostics for both files.
     // (LSP doesn't define the order, but we happen to know it)
@@ -52,7 +58,7 @@ fn refresh_missing_imports() {
     loop {
         let diags = harness.wait_for_diagnostics();
         assert!(diags.diagnostics.is_empty());
-        if diags.uri.path() == "/dep.ncl" {
+        if diags.uri == dep_uri {
             break;
         }
     }
@@ -60,7 +66,7 @@ fn refresh_missing_imports() {
     loop {
         let diags = harness.wait_for_diagnostics();
         assert!(diags.diagnostics.is_empty());
-        if diags.uri.path() == "/test.ncl" {
+        if diags.uri == test_uri {
             break;
         }
     }
@@ -73,17 +79,18 @@ fn reload_broken_imports() {
     let _ = env_logger::try_init();
     let mut harness = TestHarness::new();
 
-    let url = |s: &str| lsp_types::Url::from_file_path(s).unwrap();
-    harness.send_file(url("/dep.ncl"), "{ x }");
+    let dep_uri = file_url_from_path("/dep.ncl").unwrap();
+    harness.send_file(dep_uri.clone(), "{ x }");
 
-    harness.send_file(url("/test.ncl"), "import \"dep.ncl\"");
+    let test_uri = file_url_from_path("/test.ncl").unwrap();
+    harness.send_file(test_uri.clone(), "import \"dep.ncl\"");
     let diags = harness.wait_for_diagnostics();
 
-    assert_eq!("/dep.ncl", diags.uri.path());
+    assert_eq!(diags.uri, dep_uri);
     assert!(diags.diagnostics.is_empty());
 
     let diags = harness.wait_for_diagnostics();
-    assert_eq!("/test.ncl", diags.uri.path());
+    assert_eq!(diags.uri, test_uri);
     assert!(diags.diagnostics.is_empty());
 
     // We expect two more diagnostics coming from background eval.
@@ -91,7 +98,7 @@ fn reload_broken_imports() {
     let _diags = harness.wait_for_diagnostics();
 
     // Introduce an error in the import.
-    harness.send_file(url("/dep.ncl"), "{ `x = 1 }");
+    harness.send_file(dep_uri.clone(), "{ `x = 1 }");
 
     // Check that we get back clean diagnostics for both files.
     // (LSP doesn't define the order, but we happen to know it)
@@ -99,7 +106,7 @@ fn reload_broken_imports() {
     // file (once from synchronous typechecking, once from eval in the background).
     loop {
         let diags = harness.wait_for_diagnostics();
-        if diags.uri.path() == "/dep.ncl" {
+        if diags.uri == dep_uri {
             assert_eq!(diags.diagnostics[0].message, "unexpected token");
             break;
         }
@@ -107,7 +114,7 @@ fn reload_broken_imports() {
 
     loop {
         let diags = harness.wait_for_diagnostics();
-        if diags.uri.path() == "/test.ncl" {
+        if diags.uri == test_uri {
             assert_eq!(diags.diagnostics[0].message, "unexpected token");
             break;
         }
@@ -125,9 +132,9 @@ fn apply_client_options() {
         }
     });
     let mut harness = TestHarness::new_with_options(Some(lsp_options));
-    let url = |s: &str| lsp_types::Url::from_file_path(s).unwrap();
+    let test_uri = file_url_from_path("/test.ncl").unwrap();
     harness.send_file(
-        url("/test.ncl"),
+        test_uri,
         "{ C = fun n => if n == 0 then String else C (n - 1), res = 2 | C 5 }",
     );
 

utils/Cargo.toml

@@ -13,9 +13,15 @@ repository.workspace = true
 bench = false
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
+[features]
+pprof = ["dep:pprof"]
+
 [dependencies]
 nickel-lang-core.workspace = true
 criterion.workspace = true
 codespan.workspace = true
 serde = { workspace = true, features = ["derive"] }
 toml = { workspace = true, features = ["parse"] }
+
+[target.'cfg(target_family = "unix")'.dependencies]
+pprof = { workspace = true, features = ["criterion", "flamegraph"], optional = true }

utils/src/bench.rs

@@ -71,7 +71,7 @@ impl<'b> Bench<'b> {
 
         let field_path = self.subtest.map(|s| format!(".{s}")).unwrap_or_default();
         let content = format!(
-            "(import \"{}\"){}.run {}",
+            "(import {:?}){}.run {}",
             path.to_string_lossy(),
             field_path,
             self.args
@@ -86,13 +86,15 @@ impl<'b> Bench<'b> {
         } else {
             content
         };
-        parse(&content).unwrap()
+        parse(&content).unwrap_or_else(|err| panic!("Failed parsing {path:?}: {err:?}"))
     }
 
     pub fn path(&self) -> PathBuf {
-        let mut path = PathBuf::from(self.base_dir);
-        path.push(format!("benches/{}.ncl", self.subpath));
-        path
+        PathBuf::from_iter([
+            self.base_dir,
+            "benches",
+            format!("{}.ncl", self.subpath).as_str(),
+        ])
     }
 }
 
@@ -139,6 +141,17 @@ pub fn bench_terms<'r>(rts: Vec<Bench<'r>>) -> Box<dyn Fn(&mut Criterion) + 'r>
     })
 }
 
+/// Create a `Criterion` config. Uses `PProfProfiler` when `pprof` is enabled on Unix systems.
+pub fn criterion_config() -> Criterion {
+    let config = Criterion::default();
+    #[cfg(all(target_family = "unix", feature = "pprof"))]
+    let config = config.with_profiler(pprof::criterion::PProfProfiler::new(
+        100,
+        pprof::criterion::Output::Flamegraph(None),
+    ));
+    config
+}
+
 #[macro_export]
 macro_rules! ncl_bench {
     {