feat: Add test(should_fail) attribute for tests that are meant to f…

…ail (#2418)

Co-authored-by: Blaine Bublitz <[email protected]>
Co-authored-by: kevaundray <[email protected]>

crates/lsp/src/lib.rs

@@ -199,8 +199,8 @@ fn on_code_lens_request(
         let tests = context
             .get_all_test_functions_in_crate_matching(&crate_id, FunctionNameMatch::Anything);
 
-        for (func_name, func_id) in tests {
-            let location = context.function_meta(&func_id).name.location;
+        for (func_name, test_function) in tests {
+            let location = context.function_meta(&test_function.get_id()).name.location;
             let file_id = location.file;
 
             // Ignore diagnostics for any file that wasn't the file we saved

crates/nargo_cli/src/cli/test_cmd.rs

@@ -7,8 +7,7 @@ use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelec
 use noirc_driver::{compile_no_check, CompileOptions};
 use noirc_frontend::{
     graph::CrateName,
-    hir::{Context, FunctionNameMatch},
-    node_interner::FuncId,
+    hir::{def_map::TestFunction, Context, FunctionNameMatch},
 };
 use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
 
@@ -123,30 +122,70 @@ fn run_tests<B: Backend>(
 fn run_test<B: Backend>(
     backend: &B,
     test_name: &str,
-    main: FuncId,
+    test_function: TestFunction,
     context: &Context,
     show_output: bool,
     config: &CompileOptions,
 ) -> Result<(), CliError<B>> {
-    let mut program = compile_no_check(context, config, main).map_err(|err| {
+    let report_error = |err| {
         noirc_errors::reporter::report_all(&context.file_manager, &[err], config.deny_warnings);
-        CliError::Generic(format!("Test '{test_name}' failed to compile"))
-    })?;
-
-    // Note: We could perform this test using the unoptimized ACIR as generated by `compile_no_check`.
-    program.circuit = optimize_circuit(backend, program.circuit).unwrap().0;
-
-    // Run the backend to ensure the PWG evaluates functions like std::hash::pedersen,
-    // otherwise constraints involving these expressions will not error.
-    match execute_circuit(backend, program.circuit, WitnessMap::new(), show_output) {
-        Ok(_) => Ok(()),
-        Err(error) => {
-            let writer = StandardStream::stderr(ColorChoice::Always);
-            let mut writer = writer.lock();
-            writer.set_color(ColorSpec::new().set_fg(Some(Color::Red))).ok();
-            writeln!(writer, "failed").ok();
-            writer.reset().ok();
-            Err(error.into())
+        Err(CliError::Generic(format!("Test '{test_name}' failed to compile")))
+    };
+    let write_error = |err| {
+        let writer = StandardStream::stderr(ColorChoice::Always);
+        let mut writer = writer.lock();
+        writer.set_color(ColorSpec::new().set_fg(Some(Color::Red))).ok();
+        writeln!(writer, "failed").ok();
+        writer.reset().ok();
+        Err(err)
+    };
+
+    let program = compile_no_check(context, config, test_function.get_id());
+    match program {
+        Ok(mut program) => {
+            // Note: We don't need to use the optimized ACIR here
+            program.circuit = optimize_circuit(backend, program.circuit).unwrap().0;
+
+            // Run the backend to ensure the PWG evaluates functions like std::hash::pedersen,
+            // otherwise constraints involving these expressions will not error.
+            let circuit_execution =
+                execute_circuit(backend, program.circuit, WitnessMap::new(), show_output);
+
+            if test_function.should_fail() {
+                match circuit_execution {
+                    Ok(_) => {
+                        write_error(CliError::Generic(format!("Test '{test_name}' should fail")))
+                    }
+                    Err(_) => Ok(()),
+                }
+            } else {
+                match circuit_execution {
+                    Ok(_) => Ok(()),
+                    Err(error) => write_error(error.into()),
+                }
+            }
+        }
+        // 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 Ok.
+        Err(err) => {
+            // The test has failed compilation, but it should never fail. Report error.
+            if !test_function.should_fail() {
+                return report_error(err);
+            }
+
+            // The test has failed compilation, check if it is because the program is never satisfiable.
+            // If it is never satisfiable, then this is the expected behavior.
+            let program_is_never_satisfiable = err.diagnostic.message.contains("Failed constraint");
+            if program_is_never_satisfiable {
+                return Ok(());
+            }
+
+            // The test has failed compilation, but its a compilation error. Report error
+            report_error(err)
         }
     }
 }

crates/noirc_frontend/src/ast/function.rs

@@ -83,7 +83,7 @@ impl From<FunctionDefinition> for NoirFunction {
         let kind = match fd.attribute {
             Some(Attribute::Builtin(_)) => FunctionKind::Builtin,
             Some(Attribute::Foreign(_)) => FunctionKind::LowLevel,
-            Some(Attribute::Test) => FunctionKind::Normal,
+            Some(Attribute::Test { .. }) => FunctionKind::Normal,
             Some(Attribute::Oracle(_)) => FunctionKind::Oracle,
             Some(Attribute::Deprecated(_)) | None => FunctionKind::Normal,
             Some(Attribute::Custom(_)) => FunctionKind::Normal,

crates/noirc_frontend/src/hir/def_map/mod.rs

@@ -3,7 +3,7 @@ use crate::hir::def_collector::dc_crate::DefCollector;
 use crate::hir::Context;
 use crate::node_interner::{FuncId, NodeInterner};
 use crate::parser::{parse_program, ParsedModule};
-use crate::token::Attribute;
+use crate::token::{Attribute, TestScope};
 use arena::{Arena, Index};
 use fm::{FileId, FileManager};
 use noirc_errors::{FileDiagnostic, Location};
@@ -129,12 +129,18 @@ impl CrateDefMap {
     pub fn get_all_test_functions<'a>(
         &'a self,
         interner: &'a NodeInterner,
-    ) -> impl Iterator<Item = FuncId> + 'a {
+    ) -> impl Iterator<Item = TestFunction> + 'a {
         self.modules.iter().flat_map(|(_, module)| {
-            module
-                .value_definitions()
-                .filter_map(|id| id.as_function())
-                .filter(|id| interner.function_meta(id).attributes == Some(Attribute::Test))
+            module.value_definitions().filter_map(|id| {
+                if let Some(func_id) = id.as_function() {
+                    match interner.function_meta(&func_id).attributes {
+                        Some(Attribute::Test(scope)) => Some(TestFunction::new(func_id, scope)),
+                        _ => None,
+                    }
+                } else {
+                    None
+                }
+            })
         })
     }
 
@@ -221,3 +227,28 @@ impl std::ops::IndexMut<LocalModuleId> for CrateDefMap {
         &mut self.modules[local_module_id.0]
     }
 }
+
+pub struct TestFunction {
+    id: FuncId,
+    scope: TestScope,
+}
+
+impl TestFunction {
+    fn new(id: FuncId, scope: TestScope) -> Self {
+        TestFunction { id, scope }
+    }
+
+    /// Returns the function id of the test function
+    pub fn get_id(&self) -> FuncId {
+        self.id
+    }
+
+    /// Returns true if the test function has been specified to fail
+    /// This is done by annotating the function with `#[test(should_fail)]`
+    pub fn should_fail(&self) -> bool {
+        match self.scope {
+            TestScope::ShouldFail => true,
+            TestScope::None => false,
+        }
+    }
+}

crates/noirc_frontend/src/hir/mod.rs

@@ -11,6 +11,8 @@ use def_map::{Contract, CrateDefMap};
 use fm::FileManager;
 use std::collections::HashMap;
 
+use self::def_map::TestFunction;
+
 /// Helper object which groups together several useful context objects used
 /// during name resolution. Once name resolution is finished, only the
 /// def_interner is required for type inference and monomorphization.
@@ -111,22 +113,22 @@ impl Context {
         &self,
         crate_id: &CrateId,
         pattern: FunctionNameMatch,
-    ) -> Vec<(String, FuncId)> {
+    ) -> Vec<(String, TestFunction)> {
         let interner = &self.def_interner;
         let def_map = self.def_map(crate_id).expect("The local crate should be analyzed already");
 
         def_map
             .get_all_test_functions(interner)
-            .filter_map(|id| {
-                let fully_qualified_name = self.fully_qualified_function_name(crate_id, &id);
+            .filter_map(|test_function| {
+                let fully_qualified_name =
+                    self.fully_qualified_function_name(crate_id, &test_function.get_id());
                 match &pattern {
-                    FunctionNameMatch::Anything => Some((fully_qualified_name, id)),
-                    FunctionNameMatch::Exact(pattern) => {
-                        (&fully_qualified_name == pattern).then_some((fully_qualified_name, id))
-                    }
-                    FunctionNameMatch::Contains(pattern) => {
-                        fully_qualified_name.contains(pattern).then_some((fully_qualified_name, id))
-                    }
+                    FunctionNameMatch::Anything => Some((fully_qualified_name, test_function)),
+                    FunctionNameMatch::Exact(pattern) => (&fully_qualified_name == pattern)
+                        .then_some((fully_qualified_name, test_function)),
+                    FunctionNameMatch::Contains(pattern) => fully_qualified_name
+                        .contains(pattern)
+                        .then_some((fully_qualified_name, test_function)),
                 }
             })
             .collect()

crates/noirc_frontend/src/hir/resolution/resolver.rs

@@ -700,7 +700,8 @@ impl<'a> Resolver<'a> {
             self.push_err(ResolverError::DistinctNotAllowed { ident: func.name_ident().clone() });
         }
 
-        if attributes == Some(Attribute::Test) && !parameters.is_empty() {
+        if matches!(attributes, Some(Attribute::Test { .. })) && !parameters.is_empty()
+        {
             self.push_err(ResolverError::TestFunctionHasParameters {
                 span: func.name_ident().span(),
             });