From 2a0edaab6fac8bcfdd5ec186aa93d54d846664af Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 1 Nov 2024 16:16:06 +0000 Subject: [PATCH 1/5] Run test matrix on test_programs --- tooling/nargo_cli/build.rs | 49 +++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 94f74a06149..3056b525486 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -92,17 +92,22 @@ fn generate_test_case( test_file: &mut File, test_name: &str, test_dir: &std::path::Display, + test_command: &str, test_content: &str, ) { write!( test_file, r#" -#[test] -fn test_{test_name}() {{ +#[test_case::test_matrix( + [i64::MIN, 0, i64::MAX] +)] +fn test_{test_name}(inliner_aggressiveness: i64) {{ let test_program_dir = PathBuf::from("{test_dir}"); let mut nargo = Command::cargo_bin("nargo").unwrap(); nargo.arg("--program-dir").arg(test_program_dir); + nargo.arg("{test_command}").arg("--force"); + nargo.arg("--inliner-aggressiveness").arg(inliner_aggressiveness.to_string()); {test_content} }} "# @@ -128,9 +133,8 @@ fn generate_execution_success_tests(test_file: &mut File, test_data_dir: &Path) test_file, &test_name, &test_dir, + "execute", r#" - nargo.arg("execute").arg("--force"); - nargo.assert().success();"#, ); @@ -139,10 +143,10 @@ fn generate_execution_success_tests(test_file: &mut File, test_data_dir: &Path) test_file, &format!("{test_name}_brillig"), &test_dir, + "execute", r#" - nargo.arg("execute").arg("--force").arg("--force-brillig"); - - nargo.assert().success();"#, + nargo.arg("--force-brillig"); + nargo.assert().success();"#, ); } } @@ -167,9 +171,8 @@ fn generate_execution_failure_tests(test_file: &mut File, test_data_dir: &Path) test_file, &test_name, &test_dir, + "execute", r#" - nargo.arg("execute").arg("--force"); - nargo.assert().failure().stderr(predicate::str::contains("The application panicked (crashed).").not());"#, ); } @@ -194,10 +197,9 @@ fn generate_noir_test_success_tests(test_file: &mut File, test_data_dir: &Path) test_file, &test_name, &test_dir, + "test", r#" - nargo.arg("test"); - - nargo.assert().success();"#, + nargo.assert().success();"#, ); } writeln!(test_file, "}}").unwrap(); @@ -220,10 +222,9 @@ fn generate_noir_test_failure_tests(test_file: &mut File, test_data_dir: &Path) test_file, &test_name, &test_dir, + "test", r#" - nargo.arg("test"); - - nargo.assert().failure();"#, + nargo.assert().failure();"#, ); } writeln!(test_file, "}}").unwrap(); @@ -270,10 +271,10 @@ fn generate_compile_success_empty_tests(test_file: &mut File, test_data_dir: &Pa test_file, &test_name, &test_dir, + "info", &format!( r#" - nargo.arg("info").arg("--json").arg("--force"); - + nargo.arg("--json"); {assert_zero_opcodes}"#, ), ); @@ -299,9 +300,9 @@ fn generate_compile_success_contract_tests(test_file: &mut File, test_data_dir: test_file, &test_name, &test_dir, + "compile", r#" - nargo.arg("compile").arg("--force"); - nargo.assert().success().stderr(predicate::str::contains("warning:").not());"#, + nargo.assert().success().stderr(predicate::str::contains("warning:").not());"#, ); } writeln!(test_file, "}}").unwrap(); @@ -326,9 +327,9 @@ fn generate_compile_success_no_bug_tests(test_file: &mut File, test_data_dir: &P test_file, &test_name, &test_dir, + "compile", r#" - nargo.arg("compile").arg("--force"); - nargo.assert().success().stderr(predicate::str::contains("bug:").not());"#, + nargo.assert().success().stderr(predicate::str::contains("bug:").not());"#, ); } writeln!(test_file, "}}").unwrap(); @@ -352,9 +353,9 @@ fn generate_compile_failure_tests(test_file: &mut File, test_data_dir: &Path) { test_file, &test_name, &test_dir, - r#"nargo.arg("compile").arg("--force"); - - nargo.assert().failure().stderr(predicate::str::contains("The application panicked (crashed).").not());"#, + "compile", + r#" + nargo.assert().failure().stderr(predicate::str::contains("The application panicked (crashed).").not());"#, ); } writeln!(test_file, "}}").unwrap(); From 85a6b2bc8d6f7a5223636eee0e9b2e3e8eb0d274 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 4 Nov 2024 12:37:29 +0000 Subject: [PATCH 2/5] Add MatrixConfig to cover brillig --- tooling/nargo_cli/build.rs | 92 +++++++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 31 deletions(-) diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 3056b525486..9c9e8d5b623 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -88,26 +88,49 @@ fn read_test_cases( }) } -fn generate_test_case( +struct MatrixConfig { + vary_brillig: bool, + vary_inliner: bool, +} + +impl Default for MatrixConfig { + fn default() -> Self { + Self { vary_brillig: false, vary_inliner: true } + } +} + +/// Generate all test cases for a given test directory. +/// These will be executed serially, but independently from other test directories. +/// Be careful not to run multiple tests on the same directory concurrently because +/// they can override each others' compilation artifact files. +fn generate_test_cases( test_file: &mut File, test_name: &str, test_dir: &std::path::Display, test_command: &str, test_content: &str, + matrix_config: &MatrixConfig, ) { + let brillig_cases = if matrix_config.vary_brillig { "[false, true]" } else { "[false]" }; + let inliner_cases = if matrix_config.vary_inliner { "[i64::MIN, 0, i64::MAX]" } else { "[0]" }; write!( test_file, r#" #[test_case::test_matrix( - [i64::MIN, 0, i64::MAX] + {brillig_cases}, + {inliner_cases} )] -fn test_{test_name}(inliner_aggressiveness: i64) {{ +fn test_{test_name}(force_brillig: bool, inliner_aggressiveness: i64) {{ let test_program_dir = PathBuf::from("{test_dir}"); let mut nargo = Command::cargo_bin("nargo").unwrap(); nargo.arg("--program-dir").arg(test_program_dir); nargo.arg("{test_command}").arg("--force"); nargo.arg("--inliner-aggressiveness").arg(inliner_aggressiveness.to_string()); + if force_brillig {{ + nargo.arg("--force-brillig"); + }} + {test_content} }} "# @@ -129,26 +152,19 @@ fn generate_execution_success_tests(test_file: &mut File, test_data_dir: &Path) for (test_name, test_dir) in test_cases { let test_dir = test_dir.display(); - generate_test_case( + generate_test_cases( test_file, &test_name, &test_dir, "execute", r#" - nargo.assert().success();"#, + nargo.assert().success(); + "#, + &MatrixConfig { + vary_brillig: !IGNORED_BRILLIG_TESTS.contains(&test_name.as_str()), + vary_inliner: true, + }, ); - - if !IGNORED_BRILLIG_TESTS.contains(&test_name.as_str()) { - generate_test_case( - test_file, - &format!("{test_name}_brillig"), - &test_dir, - "execute", - r#" - nargo.arg("--force-brillig"); - nargo.assert().success();"#, - ); - } } writeln!(test_file, "}}").unwrap(); } @@ -167,13 +183,15 @@ fn generate_execution_failure_tests(test_file: &mut File, test_data_dir: &Path) for (test_name, test_dir) in test_cases { let test_dir = test_dir.display(); - generate_test_case( + generate_test_cases( test_file, &test_name, &test_dir, "execute", r#" - nargo.assert().failure().stderr(predicate::str::contains("The application panicked (crashed).").not());"#, + nargo.assert().failure().stderr(predicate::str::contains("The application panicked (crashed).").not()); + "#, + &MatrixConfig::default(), ); } writeln!(test_file, "}}").unwrap(); @@ -193,13 +211,15 @@ fn generate_noir_test_success_tests(test_file: &mut File, test_data_dir: &Path) for (test_name, test_dir) in test_cases { let test_dir = test_dir.display(); - generate_test_case( + generate_test_cases( test_file, &test_name, &test_dir, "test", r#" - nargo.assert().success();"#, + nargo.assert().success(); + "#, + &MatrixConfig::default(), ); } writeln!(test_file, "}}").unwrap(); @@ -218,13 +238,15 @@ fn generate_noir_test_failure_tests(test_file: &mut File, test_data_dir: &Path) .unwrap(); for (test_name, test_dir) in test_cases { let test_dir = test_dir.display(); - generate_test_case( + generate_test_cases( test_file, &test_name, &test_dir, "test", r#" - nargo.assert().failure();"#, + nargo.assert().failure(); + "#, + &MatrixConfig::default(), ); } writeln!(test_file, "}}").unwrap(); @@ -267,7 +289,7 @@ fn generate_compile_success_empty_tests(test_file: &mut File, test_data_dir: &Pa assert_eq!(num_opcodes.as_u64().expect("number of opcodes should fit in a u64"), 0); "#; - generate_test_case( + generate_test_cases( test_file, &test_name, &test_dir, @@ -275,8 +297,10 @@ fn generate_compile_success_empty_tests(test_file: &mut File, test_data_dir: &Pa &format!( r#" nargo.arg("--json"); - {assert_zero_opcodes}"#, + {assert_zero_opcodes} + "#, ), + &MatrixConfig::default(), ); } writeln!(test_file, "}}").unwrap(); @@ -296,13 +320,15 @@ fn generate_compile_success_contract_tests(test_file: &mut File, test_data_dir: for (test_name, test_dir) in test_cases { let test_dir = test_dir.display(); - generate_test_case( + generate_test_cases( test_file, &test_name, &test_dir, "compile", r#" - nargo.assert().success().stderr(predicate::str::contains("warning:").not());"#, + nargo.assert().success().stderr(predicate::str::contains("warning:").not()); + "#, + &MatrixConfig::default(), ); } writeln!(test_file, "}}").unwrap(); @@ -323,13 +349,15 @@ fn generate_compile_success_no_bug_tests(test_file: &mut File, test_data_dir: &P for (test_name, test_dir) in test_cases { let test_dir = test_dir.display(); - generate_test_case( + generate_test_cases( test_file, &test_name, &test_dir, "compile", r#" - nargo.assert().success().stderr(predicate::str::contains("bug:").not());"#, + nargo.assert().success().stderr(predicate::str::contains("bug:").not()); + "#, + &MatrixConfig::default(), ); } writeln!(test_file, "}}").unwrap(); @@ -349,13 +377,15 @@ fn generate_compile_failure_tests(test_file: &mut File, test_data_dir: &Path) { for (test_name, test_dir) in test_cases { let test_dir = test_dir.display(); - generate_test_case( + generate_test_cases( test_file, &test_name, &test_dir, "compile", r#" - nargo.assert().failure().stderr(predicate::str::contains("The application panicked (crashed).").not());"#, + nargo.assert().failure().stderr(predicate::str::contains("The application panicked (crashed).").not()); + "#, + &MatrixConfig::default(), ); } writeln!(test_file, "}}").unwrap(); From 52d25ac6d355983168ab711b28a0d8047775e647 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 4 Nov 2024 13:15:29 +0000 Subject: [PATCH 3/5] Use a Mutex to make sure only one test compiles artifacts in a given directory at any time --- Cargo.lock | 1 + Cargo.toml | 1 + acvm-repo/bn254_blackbox_solver/Cargo.toml | 4 ++-- tooling/nargo_cli/Cargo.toml | 1 + tooling/nargo_cli/build.rs | 16 +++++++++++++++- 5 files changed, 20 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3659a264737..aca7f199bb5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2582,6 +2582,7 @@ dependencies = [ "fm", "iai", "iter-extended", + "lazy_static", "light-poseidon", "nargo", "nargo_fmt", diff --git a/Cargo.toml b/Cargo.toml index d819c37daeb..df5cc1f3e4b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -142,6 +142,7 @@ build-data = "0.1.3" bincode = "1.3.3" hex = "0.4.2" const_format = "0.2.30" +lazy_static = "1.4" num-bigint = "0.4" num-traits = "0.2" similar-asserts = "1.5.0" diff --git a/acvm-repo/bn254_blackbox_solver/Cargo.toml b/acvm-repo/bn254_blackbox_solver/Cargo.toml index 97f6b76a9a3..062131bb672 100644 --- a/acvm-repo/bn254_blackbox_solver/Cargo.toml +++ b/acvm-repo/bn254_blackbox_solver/Cargo.toml @@ -19,10 +19,10 @@ workspace = true acir.workspace = true acvm_blackbox_solver.workspace = true hex.workspace = true -lazy_static = "1.4" +lazy_static.workspace = true ark-bn254.workspace = true -grumpkin.workspace = true +grumpkin.workspace = true ark-ec.workspace = true ark-ff.workspace = true num-bigint.workspace = true diff --git a/tooling/nargo_cli/Cargo.toml b/tooling/nargo_cli/Cargo.toml index 4e45749ddaf..317706bb237 100644 --- a/tooling/nargo_cli/Cargo.toml +++ b/tooling/nargo_cli/Cargo.toml @@ -88,6 +88,7 @@ sha3.workspace = true iai = "0.1.1" test-binary = "3.0.2" test-case.workspace = true +lazy_static.workspace = true light-poseidon = "0.2.0" diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 9c9e8d5b623..678b8aaad60 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -95,7 +95,12 @@ struct MatrixConfig { impl Default for MatrixConfig { fn default() -> Self { - Self { vary_brillig: false, vary_inliner: true } + Self { + // Only used with execution, and only on selected tests. + vary_brillig: false, + // Only seems to have an effect on the `execute_success` cases. + vary_inliner: false, + } } } @@ -111,16 +116,25 @@ fn generate_test_cases( test_content: &str, matrix_config: &MatrixConfig, ) { + let mutex_name = format! {"TEST_MUTEX_{}", test_name.to_uppercase()}; let brillig_cases = if matrix_config.vary_brillig { "[false, true]" } else { "[false]" }; let inliner_cases = if matrix_config.vary_inliner { "[i64::MIN, 0, i64::MAX]" } else { "[0]" }; write!( test_file, r#" +lazy_static::lazy_static! {{ + /// Prevent concurrent tests in the matrix from overwriting the compilation artifacts in {test_dir} + static ref {mutex_name}: std::sync::Mutex<()> = std::sync::Mutex::new(()); +}} + #[test_case::test_matrix( {brillig_cases}, {inliner_cases} )] fn test_{test_name}(force_brillig: bool, inliner_aggressiveness: i64) {{ + // Ignore poisoning errors if some of the matrix cases failed. + let _guard = {mutex_name}.lock().unwrap_or_else(|e| e.into_inner()); + let test_program_dir = PathBuf::from("{test_dir}"); let mut nargo = Command::cargo_bin("nargo").unwrap(); From 0229ddf489fc0471bb98d15b3838dbbb4026b638 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 4 Nov 2024 13:23:43 +0000 Subject: [PATCH 4/5] Derive Default matrix config --- tooling/nargo_cli/build.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 678b8aaad60..0c612ace888 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -88,22 +88,14 @@ fn read_test_cases( }) } +#[derive(Default)] struct MatrixConfig { + // Only used with execution, and only on selected tests. vary_brillig: bool, + // Only seems to have an effect on the `execute_success` cases. vary_inliner: bool, } -impl Default for MatrixConfig { - fn default() -> Self { - Self { - // Only used with execution, and only on selected tests. - vary_brillig: false, - // Only seems to have an effect on the `execute_success` cases. - vary_inliner: false, - } - } -} - /// Generate all test cases for a given test directory. /// These will be executed serially, but independently from other test directories. /// Be careful not to run multiple tests on the same directory concurrently because From a45e93efb244a4cc2a3ff0d794795f44199125bd Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 5 Nov 2024 15:43:07 +0000 Subject: [PATCH 5/5] Disable inliner aggressiveness until fixed --- tooling/nargo_cli/build.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 0c612ace888..eee10ede8c4 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -98,8 +98,8 @@ struct MatrixConfig { /// Generate all test cases for a given test directory. /// These will be executed serially, but independently from other test directories. -/// Be careful not to run multiple tests on the same directory concurrently because -/// they can override each others' compilation artifact files. +/// Running multiple tests on the same directory concurrently risks overriding each +/// others compilation artifacts. fn generate_test_cases( test_file: &mut File, test_name: &str, @@ -110,7 +110,9 @@ fn generate_test_cases( ) { let mutex_name = format! {"TEST_MUTEX_{}", test_name.to_uppercase()}; let brillig_cases = if matrix_config.vary_brillig { "[false, true]" } else { "[false]" }; - let inliner_cases = if matrix_config.vary_inliner { "[i64::MIN, 0, i64::MAX]" } else { "[0]" }; + let _inliner_cases = if matrix_config.vary_inliner { "[i64::MIN, 0, i64::MAX]" } else { "[0]" }; + // TODO (#6429): Remove this once the failing tests are fixed. + let inliner_cases = "[i64::MAX]"; write!( test_file, r#"