Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: several nargo test improvements #6728

Merged
merged 39 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
1c10349
Run tests according to the system's parallelism (for now)
asterite Dec 6, 2024
e4300ce
Run package tests (as a group) sequentially
asterite Dec 9, 2024
dd2d2b9
Extract `run_package_tests`
asterite Dec 9, 2024
6c85398
Add `--test-threads` option
asterite Dec 9, 2024
89d936c
Move ouptut to receiver side
asterite Dec 9, 2024
60b52b8
Show overall "running" message after collecting tests
asterite Dec 9, 2024
82cc0ad
Show list of failed tests at the end
asterite Dec 9, 2024
368feaf
Merge branch 'master' into ab/nargo-test-improvements
TomAFrench Dec 9, 2024
bf71960
Update tooling/nargo_cli/src/cli/test_cmd.rs
asterite Dec 9, 2024
d16ea3b
clippy
asterite Dec 9, 2024
e04a2b8
Merge branch 'master' into ab/nargo-test-improvements
asterite Dec 9, 2024
63e8b7c
Merge branch 'master' into ab/nargo-test-improvements
asterite Dec 9, 2024
f0e98d2
Compile packages in parallel
asterite Dec 9, 2024
38b62b7
Merge branch 'master' into ab/nargo-test-improvements
asterite Dec 9, 2024
dffc883
Use `current_num_threads` instead of `max_num_threads`
asterite Dec 10, 2024
3bb2ef2
Run all tests in parallel
asterite Dec 10, 2024
8a3c0da
Introduce TestRunner to avoid passing too many arguments
asterite Dec 10, 2024
776f541
Simplify display test status/report
asterite Dec 10, 2024
27f4ceb
Add some comments
asterite Dec 10, 2024
4e1a758
Merge branch 'master' into ab/nargo-test-improvements
asterite Dec 10, 2024
9e81a31
Remove accidental test
asterite Dec 10, 2024
ab041c5
Capture print output and show it together with their tests
asterite Dec 10, 2024
1be0680
Show test duration for tests that took more than 30 seconds
asterite Dec 10, 2024
7abb6d7
Clarify who the output belongs to
asterite Dec 11, 2024
37dd81f
Use default value in option
asterite Dec 11, 2024
738534f
Remove comment
asterite Dec 11, 2024
fb4f4e8
package_test_results -> packages_tests
asterite Dec 11, 2024
d9446d5
Break if the receiver is closed
asterite Dec 11, 2024
6341604
Use BTree map so we showd package output by package name
asterite Dec 11, 2024
6559b26
No need to track all test results
asterite Dec 11, 2024
94e53d2
Use `then_some`
asterite Dec 11, 2024
9693722
Clippy
asterite Dec 11, 2024
d5f8e2c
Show trailing "---" after stdout so the entire output is easier to see
asterite Dec 11, 2024
d89eb90
Merge branch 'master' into ab/nargo-test-improvements
asterite Dec 11, 2024
6add1d8
Let `collect_packages_tests` return `Result`
asterite Dec 11, 2024
a6ca811
Print package test count right before printing package tests
asterite Dec 11, 2024
8e40161
Use `catch_unwind`
asterite Dec 11, 2024
7967f33
Handle `panic!("...")` in catch_unwind handling
asterite Dec 11, 2024
cac485a
cargo fmt
asterite Dec 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion tooling/acvm_cli/src/cli/execute_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use acir::native_types::{WitnessMap, WitnessStack};
use acir::FieldElement;
use bn254_blackbox_solver::Bn254BlackBoxSolver;
use clap::Args;
use nargo::PrintOutput;

use crate::cli::fs::inputs::{read_bytecode_from_file, read_inputs_from_file};
use crate::errors::CliError;
Expand Down Expand Up @@ -73,7 +74,7 @@ pub(crate) fn execute_program_from_witness(
&program,
inputs_map,
&Bn254BlackBoxSolver,
&mut DefaultForeignCallExecutor::new(true, None, None, None),
&mut DefaultForeignCallExecutor::new(PrintOutput::Stdout, None, None, None),
)
.map_err(CliError::CircuitExecutionError)
}
15 changes: 10 additions & 5 deletions tooling/debugger/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -967,6 +967,7 @@ mod tests {
BinaryFieldOp, HeapValueType, MemoryAddress, Opcode as BrilligOpcode, ValueOrArray,
},
};
use nargo::PrintOutput;

#[test]
fn test_resolve_foreign_calls_stepping_into_brillig() {
Expand Down Expand Up @@ -1025,8 +1026,10 @@ mod tests {

let initial_witness = BTreeMap::from([(Witness(1), fe_1)]).into();

let foreign_call_executor =
Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact));
let foreign_call_executor = Box::new(DefaultDebugForeignCallExecutor::from_artifact(
PrintOutput::Stdout,
debug_artifact,
));
let mut context = DebugContext::new(
&StubbedBlackBoxSolver,
circuits,
Expand Down Expand Up @@ -1190,8 +1193,10 @@ mod tests {

let initial_witness = BTreeMap::from([(Witness(1), fe_1), (Witness(2), fe_1)]).into();

let foreign_call_executor =
Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact));
let foreign_call_executor = Box::new(DefaultDebugForeignCallExecutor::from_artifact(
PrintOutput::Stdout,
debug_artifact,
));
let brillig_funcs = &vec![brillig_bytecode];
let mut context = DebugContext::new(
&StubbedBlackBoxSolver,
Expand Down Expand Up @@ -1285,7 +1290,7 @@ mod tests {
&circuits,
&debug_artifact,
WitnessMap::new(),
Box::new(DefaultDebugForeignCallExecutor::new(true)),
Box::new(DefaultDebugForeignCallExecutor::new(PrintOutput::Stdout)),
brillig_funcs,
);

Expand Down
6 changes: 5 additions & 1 deletion tooling/debugger/src/dap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use acvm::acir::circuit::brillig::BrilligBytecode;
use acvm::acir::circuit::Circuit;
use acvm::acir::native_types::WitnessMap;
use acvm::{BlackBoxFunctionSolver, FieldElement};
use nargo::PrintOutput;

use crate::context::DebugContext;
use crate::context::{DebugCommandResult, DebugLocation};
Expand Down Expand Up @@ -71,7 +72,10 @@ impl<'a, R: Read, W: Write, B: BlackBoxFunctionSolver<FieldElement>> DapSession<
circuits,
debug_artifact,
initial_witness,
Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact)),
Box::new(DefaultDebugForeignCallExecutor::from_artifact(
PrintOutput::Stdout,
debug_artifact,
)),
unconstrained_functions,
);
Self {
Expand Down
23 changes: 13 additions & 10 deletions tooling/debugger/src/foreign_calls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ use acvm::{
pwg::ForeignCallWaitInfo,
AcirField, FieldElement,
};
use nargo::foreign_calls::{DefaultForeignCallExecutor, ForeignCallExecutor};
use nargo::{
foreign_calls::{DefaultForeignCallExecutor, ForeignCallExecutor},
PrintOutput,
};
use noirc_artifacts::debug::{DebugArtifact, DebugVars, StackFrame};
use noirc_errors::debug_info::{DebugFnId, DebugVarId};
use noirc_printable_type::ForeignCallError;
Expand Down Expand Up @@ -41,21 +44,21 @@ pub trait DebugForeignCallExecutor: ForeignCallExecutor<FieldElement> {
fn current_stack_frame(&self) -> Option<StackFrame<FieldElement>>;
}

pub struct DefaultDebugForeignCallExecutor {
executor: DefaultForeignCallExecutor<FieldElement>,
pub struct DefaultDebugForeignCallExecutor<'a> {
executor: DefaultForeignCallExecutor<'a, FieldElement>,
pub debug_vars: DebugVars<FieldElement>,
}

impl DefaultDebugForeignCallExecutor {
pub fn new(show_output: bool) -> Self {
impl<'a> DefaultDebugForeignCallExecutor<'a> {
pub fn new(output: PrintOutput<'a>) -> Self {
Self {
executor: DefaultForeignCallExecutor::new(show_output, None, None, None),
executor: DefaultForeignCallExecutor::new(output, None, None, None),
debug_vars: DebugVars::default(),
}
}

pub fn from_artifact(show_output: bool, artifact: &DebugArtifact) -> Self {
let mut ex = Self::new(show_output);
pub fn from_artifact(output: PrintOutput<'a>, artifact: &DebugArtifact) -> Self {
let mut ex = Self::new(output);
ex.load_artifact(artifact);
ex
}
Expand All @@ -70,7 +73,7 @@ impl DefaultDebugForeignCallExecutor {
}
}

impl DebugForeignCallExecutor for DefaultDebugForeignCallExecutor {
impl DebugForeignCallExecutor for DefaultDebugForeignCallExecutor<'_> {
fn get_variables(&self) -> Vec<StackFrame<FieldElement>> {
self.debug_vars.get_variables()
}
Expand All @@ -88,7 +91,7 @@ fn debug_fn_id(value: &FieldElement) -> DebugFnId {
DebugFnId(value.to_u128() as u32)
}

impl ForeignCallExecutor<FieldElement> for DefaultDebugForeignCallExecutor {
impl ForeignCallExecutor<FieldElement> for DefaultDebugForeignCallExecutor<'_> {
fn execute(
&mut self,
foreign_call: &ForeignCallWaitInfo<FieldElement>,
Expand Down
14 changes: 9 additions & 5 deletions tooling/debugger/src/repl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use acvm::brillig_vm::brillig::Opcode as BrilligOpcode;
use acvm::brillig_vm::MemoryValue;
use acvm::AcirField;
use acvm::{BlackBoxFunctionSolver, FieldElement};
use nargo::NargoError;
use nargo::{NargoError, PrintOutput};
use noirc_driver::CompiledProgram;

use crate::foreign_calls::DefaultDebugForeignCallExecutor;
Expand Down Expand Up @@ -42,8 +42,10 @@ impl<'a, B: BlackBoxFunctionSolver<FieldElement>> ReplDebugger<'a, B> {
initial_witness: WitnessMap<FieldElement>,
unconstrained_functions: &'a [BrilligBytecode<FieldElement>],
) -> Self {
let foreign_call_executor =
Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, debug_artifact));
let foreign_call_executor = Box::new(DefaultDebugForeignCallExecutor::from_artifact(
PrintOutput::Stdout,
debug_artifact,
));
let context = DebugContext::new(
blackbox_solver,
circuits,
Expand Down Expand Up @@ -313,8 +315,10 @@ impl<'a, B: BlackBoxFunctionSolver<FieldElement>> ReplDebugger<'a, B> {

fn restart_session(&mut self) {
let breakpoints: Vec<DebugLocation> = self.context.iterate_breakpoints().copied().collect();
let foreign_call_executor =
Box::new(DefaultDebugForeignCallExecutor::from_artifact(true, self.debug_artifact));
let foreign_call_executor = Box::new(DefaultDebugForeignCallExecutor::from_artifact(
PrintOutput::Stdout,
self.debug_artifact,
));
self.context = DebugContext::new(
self.blackbox_solver,
self.circuits,
Expand Down
7 changes: 5 additions & 2 deletions tooling/lsp/src/requests/test_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ use std::future::{self, Future};

use crate::insert_all_files_for_workspace_into_file_manager;
use async_lsp::{ErrorCode, ResponseError};
use nargo::ops::{run_test, TestStatus};
use nargo::{
ops::{run_test, TestStatus},
PrintOutput,
};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{check_crate, CompileOptions, NOIR_ARTIFACT_VERSION_STRING};
use noirc_frontend::hir::FunctionNameMatch;
Expand Down Expand Up @@ -84,7 +87,7 @@ fn on_test_run_request_inner(
&state.solver,
&mut context,
&test_function,
true,
PrintOutput::Stdout,
None,
Some(workspace.root_dir.clone()),
Some(package.name.to_string()),
Expand Down
24 changes: 9 additions & 15 deletions tooling/nargo/src/foreign_calls/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::path::PathBuf;
use acvm::{acir::brillig::ForeignCallResult, pwg::ForeignCallWaitInfo, AcirField};
use mocker::MockForeignCallExecutor;
use noirc_printable_type::ForeignCallError;
use print::PrintForeignCallExecutor;
use print::{PrintForeignCallExecutor, PrintOutput};
use rand::Rng;
use rpc::RPCForeignCallExecutor;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -65,22 +65,22 @@ impl ForeignCall {
}

#[derive(Debug, Default)]
pub struct DefaultForeignCallExecutor<F> {
pub struct DefaultForeignCallExecutor<'a, F> {
/// The executor for any [`ForeignCall::Print`] calls.
printer: Option<PrintForeignCallExecutor>,
printer: PrintForeignCallExecutor<'a>,
mocker: MockForeignCallExecutor<F>,
external: Option<RPCForeignCallExecutor>,
}

impl<F: Default> DefaultForeignCallExecutor<F> {
impl<'a, F: Default> DefaultForeignCallExecutor<'a, F> {
pub fn new(
show_output: bool,
output: PrintOutput<'a>,
resolver_url: Option<&str>,
root_path: Option<PathBuf>,
package_name: Option<String>,
) -> Self {
let id = rand::thread_rng().gen();
let printer = if show_output { Some(PrintForeignCallExecutor) } else { None };
let printer = PrintForeignCallExecutor { output };
let external_resolver = resolver_url.map(|resolver_url| {
RPCForeignCallExecutor::new(resolver_url, id, root_path, package_name)
});
Expand All @@ -92,22 +92,16 @@ impl<F: Default> DefaultForeignCallExecutor<F> {
}
}

impl<F: AcirField + Serialize + for<'a> Deserialize<'a>> ForeignCallExecutor<F>
for DefaultForeignCallExecutor<F>
impl<'a, F: AcirField + Serialize + for<'b> Deserialize<'b>> ForeignCallExecutor<F>
for DefaultForeignCallExecutor<'a, F>
{
fn execute(
&mut self,
foreign_call: &ForeignCallWaitInfo<F>,
) -> Result<ForeignCallResult<F>, ForeignCallError> {
let foreign_call_name = foreign_call.function.as_str();
match ForeignCall::lookup(foreign_call_name) {
Some(ForeignCall::Print) => {
if let Some(printer) = &mut self.printer {
printer.execute(foreign_call)
} else {
Ok(ForeignCallResult::default())
}
}
Some(ForeignCall::Print) => self.printer.execute(foreign_call),
Some(
ForeignCall::CreateMock
| ForeignCall::SetMockParams
Expand Down
22 changes: 19 additions & 3 deletions tooling/nargo/src/foreign_calls/print.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,19 @@ use noirc_printable_type::{ForeignCallError, PrintableValueDisplay};
use super::{ForeignCall, ForeignCallExecutor};

#[derive(Debug, Default)]
pub(crate) struct PrintForeignCallExecutor;
pub enum PrintOutput<'a> {
#[default]
None,
Stdout,
String(&'a mut String),
}

#[derive(Debug, Default)]
pub(crate) struct PrintForeignCallExecutor<'a> {
pub(crate) output: PrintOutput<'a>,
}

impl<F: AcirField> ForeignCallExecutor<F> for PrintForeignCallExecutor {
impl<F: AcirField> ForeignCallExecutor<F> for PrintForeignCallExecutor<'_> {
fn execute(
&mut self,
foreign_call: &ForeignCallWaitInfo<F>,
Expand All @@ -26,7 +36,13 @@ impl<F: AcirField> ForeignCallExecutor<F> for PrintForeignCallExecutor {
let display_string =
format!("{display_values}{}", if skip_newline { "" } else { "\n" });

print!("{display_string}");
match &mut self.output {
PrintOutput::None => (),
PrintOutput::Stdout => print!("{display_string}"),
PrintOutput::String(string) => {
string.push_str(&display_string);
}
}

Ok(ForeignCallResult::default())
}
Expand Down
1 change: 1 addition & 0 deletions tooling/nargo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use rayon::prelude::*;
use walkdir::WalkDir;

pub use self::errors::NargoError;
pub use self::foreign_calls::print::PrintOutput;

pub fn prepare_dependencies(
context: &mut Context,
Expand Down
34 changes: 15 additions & 19 deletions tooling/nargo/src/ops/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ use serde::{Deserialize, Serialize};
use crate::{
errors::try_to_diagnose_runtime_error,
foreign_calls::{
mocker::MockForeignCallExecutor, print::PrintForeignCallExecutor,
rpc::RPCForeignCallExecutor, ForeignCall, ForeignCallExecutor,
mocker::MockForeignCallExecutor,
print::{PrintForeignCallExecutor, PrintOutput},
rpc::RPCForeignCallExecutor,
ForeignCall, ForeignCallExecutor,
},
NargoError,
};
Expand All @@ -45,7 +47,7 @@ pub fn run_test<B: BlackBoxFunctionSolver<FieldElement>>(
blackbox_solver: &B,
context: &mut Context,
test_function: &TestFunction,
show_output: bool,
output: PrintOutput<'_>,
foreign_call_resolver_url: Option<&str>,
root_path: Option<PathBuf>,
package_name: Option<String>,
Expand All @@ -64,7 +66,7 @@ pub fn run_test<B: BlackBoxFunctionSolver<FieldElement>>(
// 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,
output,
foreign_call_resolver_url,
root_path,
package_name,
Expand Down Expand Up @@ -125,7 +127,7 @@ pub fn run_test<B: BlackBoxFunctionSolver<FieldElement>>(
initial_witness,
blackbox_solver,
&mut TestForeignCallExecutor::<FieldElement>::new(
false,
PrintOutput::None,
foreign_call_resolver_url,
root_path.clone(),
package_name.clone(),
Expand Down Expand Up @@ -251,24 +253,24 @@ fn check_expected_failure_message(
}

/// A specialized foreign call executor which tracks whether it has encountered any unknown foreign calls
struct TestForeignCallExecutor<F> {
struct TestForeignCallExecutor<'a, F> {
/// The executor for any [`ForeignCall::Print`] calls.
printer: Option<PrintForeignCallExecutor>,
printer: PrintForeignCallExecutor<'a>,
mocker: MockForeignCallExecutor<F>,
external: Option<RPCForeignCallExecutor>,

encountered_unknown_foreign_call: bool,
}

impl<F: Default> TestForeignCallExecutor<F> {
impl<'a, F: Default> TestForeignCallExecutor<'a, F> {
fn new(
show_output: bool,
output: PrintOutput<'a>,
resolver_url: Option<&str>,
root_path: Option<PathBuf>,
package_name: Option<String>,
) -> Self {
let id = rand::thread_rng().gen();
let printer = if show_output { Some(PrintForeignCallExecutor) } else { None };
let printer = PrintForeignCallExecutor { output };
let external_resolver = resolver_url.map(|resolver_url| {
RPCForeignCallExecutor::new(resolver_url, id, root_path, package_name)
});
Expand All @@ -281,8 +283,8 @@ impl<F: Default> TestForeignCallExecutor<F> {
}
}

impl<F: AcirField + Serialize + for<'a> Deserialize<'a>> ForeignCallExecutor<F>
for TestForeignCallExecutor<F>
impl<'a, F: AcirField + Serialize + for<'b> Deserialize<'b>> ForeignCallExecutor<F>
for TestForeignCallExecutor<'a, F>
{
fn execute(
&mut self,
Expand All @@ -293,13 +295,7 @@ impl<F: AcirField + Serialize + for<'a> Deserialize<'a>> ForeignCallExecutor<F>

let foreign_call_name = foreign_call.function.as_str();
match ForeignCall::lookup(foreign_call_name) {
Some(ForeignCall::Print) => {
if let Some(printer) = &mut self.printer {
printer.execute(foreign_call)
} else {
Ok(ForeignCallResult::default())
}
}
Some(ForeignCall::Print) => self.printer.execute(foreign_call),

Some(
ForeignCall::CreateMock
Expand Down
Loading
Loading