Skip to content

Commit

Permalink
feat: allow ignoring test failures from foreign calls (#6660)
Browse files Browse the repository at this point in the history
  • Loading branch information
TomAFrench authored Dec 2, 2024
1 parent f0c5fe9 commit e3a0914
Show file tree
Hide file tree
Showing 8 changed files with 180 additions and 25 deletions.
30 changes: 23 additions & 7 deletions .github/workflows/test-js-packages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -519,12 +519,25 @@ jobs:
fail-fast: false
matrix:
project:
# Disabled as these are currently failing with many visibility errors
- { repo: AztecProtocol/aztec-nr, path: ./ }
- { repo: noir-lang/ec, path: ./ }
- { repo: noir-lang/eddsa, path: ./ }
- { repo: noir-lang/mimc, path: ./ }
- { repo: noir-lang/noir_sort, path: ./ }
- { repo: noir-lang/noir-edwards, path: ./ }
- { repo: noir-lang/noir-bignum, path: ./ }
- { repo: noir-lang/noir_bigcurve, path: ./ }
- { repo: noir-lang/noir_base64, path: ./ }
- { repo: noir-lang/noir_string_search, path: ./ }
- { repo: noir-lang/sparse_array, path: ./ }
- { repo: noir-lang/noir_rsa, path: ./lib }
- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/aztec-nr }
- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-contracts }
# Disabled as aztec-packages requires a setup-step in order to generate a `Nargo.toml`
#- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits }
- { repo: noir-lang/noir-edwards, path: ./, ref: 3188ea74fe3b059219a2ea87899589c266256d74 }
- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/parity-lib }
- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/private-kernel-lib }
- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/reset-kernel-lib }
- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/rollup-lib }
- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/types }

name: Check external repo - ${{ matrix.project.repo }}
steps:
- name: Checkout
Expand Down Expand Up @@ -554,9 +567,12 @@ jobs:
# Github actions seems to not expand "**" in globs by default.
shopt -s globstar
sed -i '/^compiler_version/d' ./**/Nargo.toml
- name: Run nargo check
- name: Run nargo test
working-directory: ./test-repo/${{ matrix.project.path }}
run: nargo check
run: nargo test --silence-warnings
env:
NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS: true

# This is a job which depends on all test jobs and reports the overall status.
# This allows us to add/remove test jobs without having to update the required workflows.
Expand Down
5 changes: 5 additions & 0 deletions tooling/lsp/src/requests/test_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ fn on_test_run_request_inner(
result: "fail".to_string(),
message: Some(message),
},
TestStatus::Skipped => NargoTestRunResult {
id: params.id.clone(),
result: "skipped".to_string(),
message: None,
},
TestStatus::CompileError(diag) => NargoTestRunResult {
id: params.id.clone(),
result: "error".to_string(),
Expand Down
6 changes: 3 additions & 3 deletions tooling/nargo/src/foreign_calls/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ use rand::Rng;
use rpc::RPCForeignCallExecutor;
use serde::{Deserialize, Serialize};

mod mocker;
mod print;
mod rpc;
pub(crate) mod mocker;
pub(crate) mod print;
pub(crate) mod rpc;

pub trait ForeignCallExecutor<F> {
fn execute(
Expand Down
2 changes: 1 addition & 1 deletion tooling/nargo/src/foreign_calls/print.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use noirc_printable_type::{ForeignCallError, PrintableValueDisplay};
use super::{ForeignCall, ForeignCallExecutor};

#[derive(Debug, Default)]
pub(super) struct PrintForeignCallExecutor;
pub(crate) struct PrintForeignCallExecutor;

impl<F: AcirField> ForeignCallExecutor<F> for PrintForeignCallExecutor {
fn execute(
Expand Down
4 changes: 2 additions & 2 deletions tooling/nargo/src/foreign_calls/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use serde::{Deserialize, Serialize};
use super::ForeignCallExecutor;

#[derive(Debug)]
pub(super) struct RPCForeignCallExecutor {
pub(crate) struct RPCForeignCallExecutor {
/// A randomly generated id for this `DefaultForeignCallExecutor`.
///
/// This is used so that a single `external_resolver` can distinguish between requests from multiple
Expand Down Expand Up @@ -44,7 +44,7 @@ struct ResolveForeignCallRequest<F> {
}

impl RPCForeignCallExecutor {
pub(super) fn new(
pub(crate) fn new(
resolver_url: &str,
id: u64,
root_path: Option<PathBuf>,
Expand Down
146 changes: 134 additions & 12 deletions tooling/nargo/src/ops/test.rs
Original file line number Diff line number Diff line change
@@ -1,29 +1,42 @@
use std::path::PathBuf;

use acvm::{
acir::native_types::{WitnessMap, WitnessStack},
BlackBoxFunctionSolver, FieldElement,
acir::{
brillig::ForeignCallResult,
native_types::{WitnessMap, WitnessStack},
},
pwg::ForeignCallWaitInfo,
AcirField, BlackBoxFunctionSolver, FieldElement,
};
use noirc_abi::Abi;
use noirc_driver::{compile_no_check, CompileError, CompileOptions};
use noirc_errors::{debug_info::DebugInfo, FileDiagnostic};
use noirc_frontend::hir::{def_map::TestFunction, Context};
use noirc_printable_type::ForeignCallError;
use rand::Rng;
use serde::{Deserialize, Serialize};

use crate::{
errors::try_to_diagnose_runtime_error, foreign_calls::DefaultForeignCallExecutor, NargoError,
errors::try_to_diagnose_runtime_error,
foreign_calls::{
mocker::MockForeignCallExecutor, print::PrintForeignCallExecutor,
rpc::RPCForeignCallExecutor, DefaultForeignCallExecutor, ForeignCall, ForeignCallExecutor,
},
NargoError,
};

use super::execute_program;

pub enum TestStatus {
Pass,
Fail { message: String, error_diagnostic: Option<FileDiagnostic> },
Skipped,
CompileError(FileDiagnostic),
}

impl TestStatus {
pub fn failed(&self) -> bool {
!matches!(self, TestStatus::Pass)
!matches!(self, TestStatus::Pass | TestStatus::Skipped)
}
}

Expand All @@ -50,23 +63,42 @@ pub fn run_test<B: BlackBoxFunctionSolver<FieldElement>>(
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 DefaultForeignCallExecutor::new(
show_output,
foreign_call_resolver_url,
root_path,
package_name,
),
&mut foreign_call_executor,
);
test_status_program_compile_pass(

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")]
{
Expand Down Expand Up @@ -217,3 +249,93 @@ fn check_expected_failure_message(
error_diagnostic,
}
}

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

encountered_unknown_foreign_call: bool,
}

impl<F: Default> TestForeignCallExecutor<F> {
fn new(
show_output: bool,
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 external_resolver = resolver_url.map(|resolver_url| {
RPCForeignCallExecutor::new(resolver_url, id, root_path, package_name)
});
TestForeignCallExecutor {
printer,
mocker: MockForeignCallExecutor::default(),
external: external_resolver,
encountered_unknown_foreign_call: false,
}
}
}

impl<F: AcirField + Serialize + for<'a> Deserialize<'a>> ForeignCallExecutor<F>
for TestForeignCallExecutor<F>
{
fn execute(
&mut self,
foreign_call: &ForeignCallWaitInfo<F>,
) -> Result<ForeignCallResult<F>, ForeignCallError> {
// If the circuit has reached a new foreign call opcode then it can't have failed from any previous unknown foreign calls.
self.encountered_unknown_foreign_call = false;

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::CreateMock
| ForeignCall::SetMockParams
| ForeignCall::GetMockLastParams
| ForeignCall::SetMockReturns
| ForeignCall::SetMockTimes
| ForeignCall::ClearMock,
) => self.mocker.execute(foreign_call),

None => {
// First check if there's any defined mock responses for this foreign call.
match self.mocker.execute(foreign_call) {
Err(ForeignCallError::NoHandler(_)) => (),
response_or_error => return response_or_error,
};

if let Some(external_resolver) = &mut self.external {
// If the user has registered an external resolver then we forward any remaining oracle calls there.
match external_resolver.execute(foreign_call) {
Err(ForeignCallError::NoHandler(_)) => (),
response_or_error => return response_or_error,
};
}

self.encountered_unknown_foreign_call = true;

// If all executors have no handler for the given foreign call then we cannot
// return a correct response to the ACVM. The best we can do is to return an empty response,
// this allows us to ignore any foreign calls which exist solely to pass information from inside
// the circuit to the environment (e.g. custom logging) as the execution will still be able to progress.
//
// We optimistically return an empty response for all oracle calls as the ACVM will error
// should a response have been required.
Ok(ForeignCallResult::default())
}
}
}
}
6 changes: 6 additions & 0 deletions tooling/nargo_cli/src/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,12 @@ fn display_test_report(
);
}
}
TestStatus::Skipped { .. } => {
writer
.set_color(ColorSpec::new().set_fg(Some(Color::Yellow)))
.expect("Failed to set color");
writeln!(writer, "skipped").expect("Failed to write to stderr");
}
TestStatus::CompileError(err) => {
noirc_errors::reporter::report_all(
file_manager.as_file_map(),
Expand Down
6 changes: 6 additions & 0 deletions tooling/nargo_cli/tests/stdlib-tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,12 @@ fn display_test_report(
);
}
}
TestStatus::Skipped { .. } => {
writer
.set_color(ColorSpec::new().set_fg(Some(Color::Yellow)))
.expect("Failed to set color");
writeln!(writer, "skipped").expect("Failed to write to stderr");
}
TestStatus::CompileError(err) => {
noirc_errors::reporter::report_all(
file_manager.as_file_map(),
Expand Down

0 comments on commit e3a0914

Please sign in to comment.