From 165eeddcca6a723b6e392220a18e5f5e41e7ed24 Mon Sep 17 00:00:00 2001 From: cydparser Date: Thu, 8 Aug 2024 18:26:23 -0700 Subject: [PATCH] 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 | 15 ++++ Cargo.lock | 2 +- cli/tests/snapshot/main.rs | 5 +- core/Cargo.toml | 3 +- core/benches/arrays.rs | 7 +- core/benches/functions.rs | 7 +- core/benches/mantis.rs | 7 +- core/benches/numeric.rs | 7 +- core/benches/records.rs | 7 +- core/benches/serialization.rs | 7 +- core/benches/stdlib.rs | 4 +- core/benches/typecheck-nixpkgs-lib.rs | 7 +- core/src/serialize.rs | 2 + lsp/lsp-harness/src/lib.rs | 95 +++++++++++++++++--- lsp/nls/Cargo.toml | 2 +- lsp/nls/benches/main.rs | 3 +- lsp/nls/tests/main.rs | 39 ++++---- utils/Cargo.toml | 6 ++ utils/src/bench.rs | 23 +++-- 19 files changed, 179 insertions(+), 69 deletions(-) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index ba181c6e7d..0b962b6f79 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.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-* diff --git a/Cargo.lock b/Cargo.lock index 6a2d92545e..6b838fbb1c 100644 --- a/Cargo.lock +++ b/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", ] diff --git a/cli/tests/snapshot/main.rs b/cli/tests/snapshot/main.rs index 3c17bbcbad..00b5e9629a 100644 --- a/cli/tests/snapshot/main.rs +++ b/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); diff --git a/core/Cargo.toml b/core/Cargo.toml index de6963701c..4c2fa3df0a 100644 --- a/core/Cargo.toml +++ b/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 diff --git a/core/benches/arrays.rs b/core/benches/arrays.rs index 70d16e8e17..9c8d36bf83 100644 --- a/core/benches/arrays.rs +++ b/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", diff --git a/core/benches/functions.rs b/core/benches/functions.rs index 28cd5f19a7..3635bb25bc 100644 --- a/core/benches/functions.rs +++ b/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", diff --git a/core/benches/mantis.rs b/core/benches/mantis.rs index 1fff0ef26f..636c5b6f91 100644 --- a/core/benches/mantis.rs +++ b/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", diff --git a/core/benches/numeric.rs b/core/benches/numeric.rs index d413c95e0c..a72c690025 100644 --- a/core/benches/numeric.rs +++ b/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", diff --git a/core/benches/records.rs b/core/benches/records.rs index d3157d9ca2..a2edaeabb7 100644 --- a/core/benches/records.rs +++ b/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", diff --git a/core/benches/serialization.rs b/core/benches/serialization.rs index d823b9fade..2c6f26c474 100644 --- a/core/benches/serialization.rs +++ b/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", diff --git a/core/benches/stdlib.rs b/core/benches/stdlib.rs index e442199f18..c15d4b8f50 100644 --- a/core/benches/stdlib.rs +++ b/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); diff --git a/core/benches/typecheck-nixpkgs-lib.rs b/core/benches/typecheck-nixpkgs-lib.rs index ea28210952..d7181eb402 100644 --- a/core/benches/typecheck-nixpkgs-lib.rs +++ b/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", diff --git a/core/src/serialize.rs b/core/src/serialize.rs index ee1478582d..16eafe0272 100644 --- a/core/src/serialize.rs +++ b/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); diff --git a/lsp/lsp-harness/src/lib.rs b/lsp/lsp-harness/src/lib.rs index 5c99a57fff..3e70f3519a 100644 --- a/lsp/lsp-harness/src/lib.rs +++ b/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>, } +/// 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 { + 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(¶ms.text_document_position_params.text_document.uri); + } + Request::References(params) => { + params.text_document_position.text_document.uri = + file_url(¶ms.text_document_position.text_document.uri); + } + Request::Completion(params) => { + params.text_document_position.text_document.uri = + file_url(¶ms.text_document_position.text_document.uri); + } + Request::Formatting(params) => { + params.text_document.uri = file_url(¶ms.text_document.uri); + } + Request::Hover(params) => { + params.text_document_position_params.text_document.uri = + file_url(¶ms.text_document_position_params.text_document.uri); + } + Request::Rename(params) => { + params.text_document_position.text_document.uri = + file_url(¶ms.text_document_position.text_document.uri); + } + Request::Symbols(params) => { + params.text_document.uri = file_url(¶ms.text_document.uri); + } + } +} + impl TestFixture { - pub fn parse(s: &str) -> Option { + pub fn parse(s: &str) -> Result { 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(), diff --git a/lsp/nls/Cargo.toml b/lsp/nls/Cargo.toml index 8ddee0d9ce..62a7bafdbb 100644 --- a/lsp/nls/Cargo.toml +++ b/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 diff --git a/lsp/nls/benches/main.rs b/lsp/nls/benches/main.rs index 87bc67aeb2..21bb147d0d 100644 --- a/lsp/nls/benches/main.rs +++ b/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); diff --git a/lsp/nls/tests/main.rs b/lsp/nls/tests/main.rs index 173610e4ee..1f4ded2d32 100644 --- a/lsp/nls/tests/main.rs +++ b/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 }", ); diff --git a/utils/Cargo.toml b/utils/Cargo.toml index 299881dd98..a9e30bc44c 100644 --- a/utils/Cargo.toml +++ b/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 } diff --git a/utils/src/bench.rs b/utils/src/bench.rs index b275949148..6179e07266 100644 --- a/utils/src/bench.rs +++ b/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>) -> Box }) } +/// 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 { {