Skip to content

Commit

Permalink
feat: several nargo test improvements (#6728)
Browse files Browse the repository at this point in the history
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
  • Loading branch information
asterite and TomAFrench authored Dec 12, 2024
1 parent f6957fa commit 1b0dd41
Show file tree
Hide file tree
Showing 17 changed files with 522 additions and 281 deletions.
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

0 comments on commit 1b0dd41

Please sign in to comment.