From a8bd98959cd1de3107bcfd8a135898822a5b2917 Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 9 Dec 2024 10:00:24 +0000 Subject: [PATCH 1/2] chore: separate test compilation from test running --- tooling/lsp/src/requests/test_run.rs | 29 +++- tooling/nargo/src/ops/test.rs | 224 +++++++++++++------------- tooling/nargo_cli/src/cli/test_cmd.rs | 16 +- 3 files changed, 146 insertions(+), 123 deletions(-) diff --git a/tooling/lsp/src/requests/test_run.rs b/tooling/lsp/src/requests/test_run.rs index 937fdcc0a5e..ac2901d98eb 100644 --- a/tooling/lsp/src/requests/test_run.rs +++ b/tooling/lsp/src/requests/test_run.rs @@ -4,7 +4,8 @@ use crate::insert_all_files_for_workspace_into_file_manager; use async_lsp::{ErrorCode, ResponseError}; use nargo::ops::{run_test, TestStatus}; use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection}; -use noirc_driver::{check_crate, CompileOptions, NOIR_ARTIFACT_VERSION_STRING}; +use noirc_driver::{check_crate, compile_no_check, CompileOptions, NOIR_ARTIFACT_VERSION_STRING}; +use noirc_errors::FileDiagnostic; use noirc_frontend::hir::FunctionNameMatch; use crate::{ @@ -80,15 +81,31 @@ fn on_test_run_request_inner( ) })?; + let compiled_program = match compile_no_check( + &mut context, + &CompileOptions::default(), + test_function.get_id(), + None, + false, + ) { + Ok(compiled_program) => compiled_program, + Err(err) => { + return Ok(NargoTestRunResult { + id: params.id.clone(), + result: "error".to_string(), + message: Some(FileDiagnostic::from(err).diagnostic.message), + }) + } + }; + let test_result = run_test( &state.solver, - &mut context, + compiled_program, &test_function, true, None, Some(workspace.root_dir.clone()), Some(package.name.to_string()), - &CompileOptions::default(), ); let result = match test_result { TestStatus::Pass => NargoTestRunResult { @@ -106,11 +123,7 @@ fn on_test_run_request_inner( result: "skipped".to_string(), message: None, }, - TestStatus::CompileError(diag) => NargoTestRunResult { - id: params.id.clone(), - result: "error".to_string(), - message: Some(diag.diagnostic.message), - }, + TestStatus::CompileError(_) => unreachable!("Test has successfully compiled"), }; Ok(result) } diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index e258627b522..c0df5eefbe8 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -9,9 +9,9 @@ use acvm::{ AcirField, BlackBoxFunctionSolver, FieldElement, }; use noirc_abi::Abi; -use noirc_driver::{compile_no_check, CompileError, CompileOptions}; +use noirc_driver::CompiledProgram; use noirc_errors::{debug_info::DebugInfo, FileDiagnostic}; -use noirc_frontend::hir::{def_map::TestFunction, Context}; +use noirc_frontend::hir::def_map::TestFunction; use noirc_printable_type::ForeignCallError; use rand::Rng; use serde::{Deserialize, Serialize}; @@ -40,137 +40,137 @@ impl TestStatus { } } -#[allow(clippy::too_many_arguments)] pub fn run_test>( blackbox_solver: &B, - context: &mut Context, + compiled_program: CompiledProgram, test_function: &TestFunction, show_output: bool, foreign_call_resolver_url: Option<&str>, root_path: Option, package_name: Option, - config: &CompileOptions, ) -> TestStatus { - let test_function_has_no_arguments = context - .def_interner - .function_meta(&test_function.get_id()) - .function_signature() - .0 - .is_empty(); - - match compile_no_check(context, config, test_function.get_id(), None, false) { - Ok(compiled_program) => { - if test_function_has_no_arguments { - // Run the backend to ensure the PWG evaluates functions like std::hash::pedersen, - // otherwise constraints involving these expressions will not error. - let mut foreign_call_executor = TestForeignCallExecutor::new( - show_output, - foreign_call_resolver_url, - root_path, - package_name, - ); - - let circuit_execution = execute_program( - &compiled_program.program, - WitnessMap::new(), - blackbox_solver, - &mut foreign_call_executor, - ); - - let status = test_status_program_compile_pass( - test_function, - compiled_program.abi, - compiled_program.debug, - circuit_execution, - ); - - let ignore_foreign_call_failures = - std::env::var("NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS") - .is_ok_and(|var| &var == "true"); - - if let TestStatus::Fail { .. } = status { - if ignore_foreign_call_failures - && foreign_call_executor.encountered_unknown_foreign_call - { - TestStatus::Skipped - } else { - status - } - } else { - status - } - } else { - #[cfg(target_arch = "wasm32")] - { - // We currently don't support fuzz testing on wasm32 as the u128 strategies do not exist on this platform. - TestStatus::Fail { - message: "Fuzz tests are not supported on wasm32".to_string(), - error_diagnostic: None, - } - } - - #[cfg(not(target_arch = "wasm32"))] - { - use acvm::acir::circuit::Program; - use noir_fuzzer::FuzzedExecutor; - use proptest::test_runner::TestRunner; - let runner = TestRunner::default(); - - let executor = - |program: &Program, - initial_witness: WitnessMap| - -> Result, String> { - execute_program( - program, - initial_witness, - blackbox_solver, - &mut TestForeignCallExecutor::::new( - false, - foreign_call_resolver_url, - root_path.clone(), - package_name.clone(), - ), - ) - .map_err(|err| err.to_string()) - }; - let fuzzer = FuzzedExecutor::new(compiled_program.into(), executor, runner); + let test_function_has_no_arguments = compiled_program.abi.parameters.is_empty(); + if test_function_has_no_arguments { + run_regular_test( + blackbox_solver, + test_function, + compiled_program, + show_output, + foreign_call_resolver_url, + root_path, + package_name, + ) + } else { + run_fuzz_test( + blackbox_solver, + compiled_program, + foreign_call_resolver_url, + root_path, + package_name, + ) + } +} - let result = fuzzer.fuzz(); - if result.success { - TestStatus::Pass - } else { - TestStatus::Fail { - message: result.reason.unwrap_or_default(), - error_diagnostic: None, - } - } - } - } +fn run_regular_test>( + blackbox_solver: &B, + test_function: &TestFunction, + compiled_program: CompiledProgram, + show_output: bool, + foreign_call_resolver_url: Option<&str>, + root_path: Option, + package_name: Option, +) -> TestStatus { + let mut foreign_call_executor = TestForeignCallExecutor::new( + show_output, + foreign_call_resolver_url, + root_path, + package_name, + ); + + let circuit_execution = execute_program( + &compiled_program.program, + WitnessMap::new(), + blackbox_solver, + &mut foreign_call_executor, + ); + + let status = check_test_status( + test_function, + compiled_program.abi, + compiled_program.debug, + circuit_execution, + ); + + let ignore_foreign_call_failures = + std::env::var("NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS") + .is_ok_and(|var| &var == "true"); + + if let TestStatus::Fail { .. } = status { + if ignore_foreign_call_failures && foreign_call_executor.encountered_unknown_foreign_call { + TestStatus::Skipped + } else { + status } - Err(err) => test_status_program_compile_fail(err, test_function), + } else { + status } } -/// Test function failed to compile -/// -/// Note: This could be because the compiler was able to deduce -/// that a constraint was never satisfiable. -/// An example of this is the program `assert(false)` -/// In that case, we check if the test function should fail, and if so, we return `TestStatus::Pass`. -fn test_status_program_compile_fail(err: CompileError, test_function: &TestFunction) -> TestStatus { - // The test has failed compilation, but it should never fail. Report error. - if !test_function.should_fail() { - return TestStatus::CompileError(err.into()); +fn run_fuzz_test>( + blackbox_solver: &B, + compiled_program: CompiledProgram, + foreign_call_resolver_url: Option<&str>, + root_path: Option, + package_name: Option, +) -> TestStatus { + #[cfg(target_arch = "wasm32")] + { + // We currently don't support fuzz testing on wasm32 as the u128 strategies do not exist on this platform. + TestStatus::Fail { + message: "Fuzz tests are not supported on wasm32".to_string(), + error_diagnostic: None, + } } - check_expected_failure_message(test_function, None, Some(err.into())) + #[cfg(not(target_arch = "wasm32"))] + { + use acvm::acir::circuit::Program; + use noir_fuzzer::FuzzedExecutor; + use proptest::test_runner::TestRunner; + let runner = TestRunner::default(); + + let executor = |program: &Program, + initial_witness: WitnessMap| + -> Result, String> { + execute_program( + program, + initial_witness, + blackbox_solver, + &mut TestForeignCallExecutor::new( + false, + foreign_call_resolver_url, + root_path.clone(), + package_name.clone(), + ), + ) + .map_err(|err| err.to_string()) + }; + let fuzzer = FuzzedExecutor::new(compiled_program.into(), executor, runner); + + let result = fuzzer.fuzz(); + if result.success { + TestStatus::Pass + } else { + TestStatus::Fail { message: result.reason.unwrap_or_default(), error_diagnostic: None } + } + } } /// The test function compiled successfully. /// /// We now check whether execution passed/failed and whether it should have /// passed/failed to determine the test status. -fn test_status_program_compile_pass( +fn check_test_status( test_function: &TestFunction, abi: Abi, debug: Vec, diff --git a/tooling/nargo_cli/src/cli/test_cmd.rs b/tooling/nargo_cli/src/cli/test_cmd.rs index aa0ee1bb94b..54cd3c2c9f0 100644 --- a/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/tooling/nargo_cli/src/cli/test_cmd.rs @@ -11,7 +11,7 @@ use nargo::{ parse_all, prepare_package, }; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; -use noirc_driver::{check_crate, CompileOptions, NOIR_ARTIFACT_VERSION_STRING}; +use noirc_driver::{check_crate, compile_no_check, CompileOptions, NOIR_ARTIFACT_VERSION_STRING}; use noirc_frontend::hir::{FunctionNameMatch, ParsedFiles}; use rayon::prelude::{IntoParallelIterator, ParallelBridge, ParallelIterator}; use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; @@ -191,15 +191,25 @@ fn run_test + Default>( let blackbox_solver = S::default(); + let compiled_program = match compile_no_check( + &mut context, + compile_options, + test_function.get_id(), + None, + false, + ) { + Ok(compiled_program) => compiled_program, + Err(err) => return TestStatus::CompileError(err.into()), + }; + nargo::ops::run_test( &blackbox_solver, - &mut context, + compiled_program, test_function, show_output, foreign_call_resolver_url, root_path, package_name, - compile_options, ) } From c9f310e249e210b0a0bfc82093545a7d5c28a125 Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 9 Dec 2024 10:36:52 +0000 Subject: [PATCH 2/2] . --- tooling/nargo_cli/tests/stdlib-tests.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tooling/nargo_cli/tests/stdlib-tests.rs b/tooling/nargo_cli/tests/stdlib-tests.rs index 99f0c9a2e7f..30e4ea65c24 100644 --- a/tooling/nargo_cli/tests/stdlib-tests.rs +++ b/tooling/nargo_cli/tests/stdlib-tests.rs @@ -2,7 +2,7 @@ #![allow(clippy::items_after_test_module)] use clap::Parser; use fm::FileManager; -use noirc_driver::{check_crate, file_manager_with_stdlib, CompileOptions}; +use noirc_driver::{check_crate, compile_no_check, file_manager_with_stdlib, CompileOptions}; use noirc_frontend::hir::FunctionNameMatch; use std::io::Write; use std::{collections::BTreeMap, path::PathBuf}; @@ -82,15 +82,25 @@ fn run_stdlib_tests(force_brillig: bool, inliner_aggressiveness: i64) { let test_report: Vec<(String, TestStatus)> = test_functions .into_iter() .map(|(test_name, test_function)| { + let compiled_program = match compile_no_check( + &mut context, + &CompileOptions { force_brillig, inliner_aggressiveness, ..Default::default() }, + test_function.get_id(), + None, + false, + ) { + Ok(compiled_program) => compiled_program, + Err(err) => return (test_name, TestStatus::CompileError(err.into())), + }; + let status = run_test( &bn254_blackbox_solver::Bn254BlackBoxSolver, - &mut context, + compiled_program, &test_function, true, None, Some(dummy_package.root_dir.clone()), Some(dummy_package.name.to_string()), - &CompileOptions { force_brillig, inliner_aggressiveness, ..Default::default() }, ); (test_name, status) })