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: skip tests having oracle calls #6666

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions .github/workflows/test-js-packages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -591,9 +591,9 @@ jobs:

- name: Run nargo test
working-directory: ./test-repo/${{ matrix.project.path }}
run: nargo test --silence-warnings
run: nargo test --silence-warnings --skip-oracle
env:
NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS: true
NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS: false

# 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
34 changes: 34 additions & 0 deletions acvm-repo/acir/src/circuit/brillig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,40 @@ pub struct BrilligBytecode<F> {
pub bytecode: Vec<BrilligOpcode<F>>,
}

/// A ForeignCall bytecode can be one of these cases:
pub enum OracleResult {
/// Bytecode which mocks oracle calls
Mocked,
/// Bytecode which calls external oracles
Unhandled,
/// Bytecode which calls internal oracles
Handled,
}
impl<F> BrilligBytecode<F> {
/// Returns
/// - Mocked: if at least one foreign call is 'mocked'
/// - Handled: if all foreign calls are 'handled'
/// - Unhandled: if at least one foreign call is 'unhandled' but none is 'mocked'
/// The foreign calls status is given by the provided filter function
pub fn get_oracle_status<Fun>(&self, filter: Fun) -> OracleResult
where
Fun: Fn(&str) -> OracleResult,
{
let mut result = OracleResult::Handled;
for op in self.bytecode.iter() {
if let BrilligOpcode::ForeignCall { function, .. } = op {
match filter(function) {
OracleResult::Mocked => return OracleResult::Mocked, // We assume that all unhandled oracle calls will be mocked. This is not necessarily the case.
OracleResult::Unhandled => {
result = OracleResult::Unhandled;
}
OracleResult::Handled => (),
}
}
}
result
}
}
/// Id for the function being called.
#[derive(
Clone, Debug, PartialEq, Eq, Serialize, Deserialize, Hash, Copy, Default, PartialOrd, Ord,
Expand Down
2 changes: 2 additions & 0 deletions docs/docs/how_to/how-to-oracles.md
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ nargo test --oracle-resolver http://localhost:5555

This tells `nargo` to use your RPC Server URL whenever it finds an oracle decorator.

You can also skip the tests using oracles by using the flag `--skip-oracle` in the `nargo test` command.

## Step 4 - Usage with NoirJS

In a JS environment, an RPC server is not strictly necessary, as you may want to resolve your oracles without needing any JSON call at all. NoirJS simply expects that you pass a callback function when you generate proofs, and that callback function can be anything.
Expand Down
1 change: 1 addition & 0 deletions tooling/lsp/src/requests/test_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ fn on_test_run_request_inner(
None,
Some(workspace.root_dir.clone()),
Some(package.name.to_string()),
false,
&CompileOptions::default(),
);
let result = match test_result {
Expand Down
20 changes: 19 additions & 1 deletion tooling/nargo/src/foreign_calls/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
use std::path::PathBuf;

use acvm::{acir::brillig::ForeignCallResult, pwg::ForeignCallWaitInfo, AcirField};
use acvm::{
acir::{brillig::ForeignCallResult, circuit::brillig::OracleResult},
pwg::ForeignCallWaitInfo,
AcirField,
};
use mocker::MockForeignCallExecutor;
use noirc_printable_type::ForeignCallError;
use print::PrintForeignCallExecutor;
Expand Down Expand Up @@ -62,6 +66,20 @@ impl ForeignCall {
_ => None,
}
}

/// Returns the foreign call status based on its name:
/// create_mock => 'mocked'
/// valid ForeignCall name => 'handled'
/// otherwise => 'unhandled'
pub(crate) fn check_oracle_status(name: &str) -> OracleResult {
if name == ForeignCall::CreateMock.name() {
OracleResult::Mocked
Comment on lines +75 to +76
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't all the other mock functions considered to be Mocked status?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to identify mocked oracles by looking at the create_mock() function, but it is not easy. Instead I used an heuristic that consider that all oracles are mocked if there is one create_mock.

} else if ForeignCall::lookup(name).is_some() {
OracleResult::Handled
} else {
OracleResult::Unhandled
}
}
}

#[derive(Debug, Default)]
Expand Down
38 changes: 37 additions & 1 deletion tooling/nargo/src/ops/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::path::PathBuf;
use acvm::{
acir::{
brillig::ForeignCallResult,
circuit::brillig::OracleResult,
native_types::{WitnessMap, WitnessStack},
},
pwg::ForeignCallWaitInfo,
Expand Down Expand Up @@ -36,7 +37,13 @@ pub enum TestStatus {

impl TestStatus {
pub fn failed(&self) -> bool {
!matches!(self, TestStatus::Pass | TestStatus::Skipped)
matches!(self, TestStatus::Fail { .. } | TestStatus::CompileError(_))
}
pub fn passed(&self) -> bool {
matches!(self, TestStatus::Pass)
}
pub fn skipped(&self) -> bool {
matches!(self, TestStatus::Skipped)
}
}

Expand All @@ -49,6 +56,7 @@ pub fn run_test<B: BlackBoxFunctionSolver<FieldElement>>(
foreign_call_resolver_url: Option<&str>,
root_path: Option<PathBuf>,
package_name: Option<String>,
skip_oracle: bool,
config: &CompileOptions,
) -> TestStatus {
let test_function_has_no_arguments = context
Expand All @@ -60,6 +68,34 @@ pub fn run_test<B: BlackBoxFunctionSolver<FieldElement>>(

match compile_no_check(context, config, test_function.get_id(), None, false) {
Ok(compiled_program) => {
if skip_oracle {
let mut has_oracle = false;
let mut unhandled = 0;
let mut mocked = 0;
for brillig_function in &compiled_program.program.unconstrained_functions {
match brillig_function.get_oracle_status(ForeignCall::check_oracle_status) {
OracleResult::Mocked => {
mocked += 1;
if !test_function.should_fail() {
has_oracle = false;
break;
}
}
OracleResult::Unhandled => {
has_oracle = true;
unhandled += 1;
}
OracleResult::Handled => (),
}
}
if mocked < unhandled {
return TestStatus::Skipped;
}
if has_oracle {
return TestStatus::Skipped;
}
}

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.
Expand Down
59 changes: 37 additions & 22 deletions tooling/nargo_cli/src/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ pub(crate) struct TestCommand {
/// JSON RPC url to solve oracle calls
#[clap(long)]
oracle_resolver: Option<String>,

/// Flag to skip tests using oracles.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Flag to skip tests using oracles.
/// Flag to skip tests using unhandled oracles without mocks.

The way it's phrased sounds like any test with an oracle will be skipped.

#[arg(long, hide = true)]
skip_oracle: bool,
}

pub(crate) fn run(args: TestCommand, config: NargoConfig) -> Result<(), CliError> {
Expand Down Expand Up @@ -76,7 +80,6 @@ pub(crate) fn run(args: TestCommand, config: NargoConfig) -> Result<(), CliError
}
None => FunctionNameMatch::Anything,
};

// Configure a thread pool with a larger stack size to prevent overflowing stack in large programs.
// Default is 2MB.
let pool = rayon::ThreadPoolBuilder::new().stack_size(4 * 1024 * 1024).build().unwrap();
Expand All @@ -94,6 +97,7 @@ pub(crate) fn run(args: TestCommand, config: NargoConfig) -> Result<(), CliError
args.oracle_resolver.as_deref(),
Some(workspace.root_dir.clone()),
Some(package.name.to_string()),
args.skip_oracle,
&args.compile_options,
)
})
Expand Down Expand Up @@ -133,6 +137,7 @@ fn run_tests<S: BlackBoxFunctionSolver<FieldElement> + Default>(
foreign_call_resolver_url: Option<&str>,
root_path: Option<PathBuf>,
package_name: Option<String>,
skip_oracle: bool,
compile_options: &CompileOptions,
) -> Result<Vec<(String, TestStatus)>, CliError> {
let test_functions =
Expand All @@ -155,6 +160,7 @@ fn run_tests<S: BlackBoxFunctionSolver<FieldElement> + Default>(
foreign_call_resolver_url,
root_path.clone(),
package_name.clone(),
skip_oracle,
compile_options,
);

Expand All @@ -176,6 +182,7 @@ fn run_test<S: BlackBoxFunctionSolver<FieldElement> + Default>(
foreign_call_resolver_url: Option<&str>,
root_path: Option<PathBuf>,
package_name: Option<String>,
skip_oracle: bool,
compile_options: &CompileOptions,
) -> TestStatus {
// This is really hacky but we can't share `Context` or `S` across threads.
Expand All @@ -199,6 +206,7 @@ fn run_test<S: BlackBoxFunctionSolver<FieldElement> + Default>(
foreign_call_resolver_url,
root_path,
package_name,
skip_oracle,
compile_options,
)
}
Expand Down Expand Up @@ -275,32 +283,39 @@ fn display_test_report(

write!(writer, "[{}] ", package.name).expect("Failed to write to stderr");

let count_all = test_report.len();
let count_failed = test_report.iter().filter(|(_, status)| status.failed()).count();
let plural = if count_all == 1 { "" } else { "s" };
if count_failed == 0 {
let count_passed = test_report.iter().filter(|(_, status)| status.passed()).count();
let count_skipped = test_report.iter().filter(|(_, status)| status.skipped()).count();
let plural_failed = if count_failed == 1 { "" } else { "s" };
let plural_passed = if count_passed == 1 { "" } else { "s" };
let plural_skipped = if count_skipped == 1 { "" } else { "s" };
let mut previous = false;
if count_passed > 0 {
aakoshh marked this conversation as resolved.
Show resolved Hide resolved
writer.set_color(ColorSpec::new().set_fg(Some(Color::Green))).expect("Failed to set color");
write!(writer, "{count_all} test{plural} passed").expect("Failed to write to stderr");
writer.reset().expect("Failed to reset writer");
writeln!(writer).expect("Failed to write to stderr");
} else {
let count_passed = count_all - count_failed;
let plural_failed = if count_failed == 1 { "" } else { "s" };
let plural_passed = if count_passed == 1 { "" } else { "s" };

if count_passed != 0 {
writer
.set_color(ColorSpec::new().set_fg(Some(Color::Green)))
.expect("Failed to set color");
write!(writer, "{count_passed} test{plural_passed} passed, ",)
.expect("Failed to write to stderr");
write!(writer, "{count_passed} test{plural_passed} passed",)
.expect("Failed to write to stderr");
previous = true;
}
if count_skipped > 0 {
writer
.set_color(ColorSpec::new().set_fg(Some(Color::Yellow)))
.expect("Failed to set color");
if previous {
write!(writer, ", ").expect("Failed to write to stderr");
}
guipublic marked this conversation as resolved.
Show resolved Hide resolved

write!(writer, "{count_skipped} test{plural_skipped} skipped")
.expect("Failed to write to stderr");
previous = true;
}
if count_failed > 0 {
writer.set_color(ColorSpec::new().set_fg(Some(Color::Red))).expect("Failed to set color");
writeln!(writer, "{count_failed} test{plural_failed} failed")
if previous {
write!(writer, ", ").expect("Failed to write to stderr");
}
write!(writer, "{count_failed} test{plural_failed} failed")
.expect("Failed to write to stderr");
writer.reset().expect("Failed to reset writer");
}

writeln!(writer).expect("Failed to write to stderr");
writer.reset().expect("Failed to reset writer");
Ok(())
}
1 change: 1 addition & 0 deletions tooling/nargo_cli/tests/stdlib-tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ fn run_stdlib_tests(force_brillig: bool, inliner_aggressiveness: i64) {
None,
Some(dummy_package.root_dir.clone()),
Some(dummy_package.name.to_string()),
false,
&CompileOptions { force_brillig, inliner_aggressiveness, ..Default::default() },
);
(test_name, status)
Expand Down
Loading