-
Notifications
You must be signed in to change notification settings - Fork 139
Extend execution-error assertion macros with patter and any arms #2897
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
base: next
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| //! Matcher trait used by the `$expected` arm of `assert_execution_error!` | ||
| //! and `assert_transaction_executor_error!`. The built-in impl for | ||
| //! [`MasmError`] preserves the legacy behavior (compare against | ||
| //! `OperationError::FailedAssertion`). | ||
|
|
||
| use core::fmt::Display; | ||
|
|
||
| use miden_processor::ExecutionError; | ||
| use miden_processor::operation::OperationError; | ||
| use miden_protocol::errors::MasmError; | ||
|
|
||
| /// Matcher for an expected [`ExecutionError`] shape. | ||
| /// `Display` is required so the macro can format a panic message on mismatch. | ||
| pub trait ExpectedExecutionError: Display { | ||
| fn matches(&self, actual: &ExecutionError) -> bool; | ||
| } | ||
|
|
||
| /// Matches `FailedAssertion` with the same `err_code`. If the actual error | ||
| /// carries an `err_msg`, it must equal the constant's message; an absent | ||
| /// `err_msg` is accepted. | ||
| impl ExpectedExecutionError for MasmError { | ||
| fn matches(&self, actual: &ExecutionError) -> bool { | ||
| let ExecutionError::OperationError { | ||
| err: OperationError::FailedAssertion { err_code, err_msg }, | ||
| .. | ||
| } = actual | ||
| else { | ||
| return false; | ||
| }; | ||
|
|
||
| if *err_code != self.code() { | ||
| return false; | ||
| } | ||
|
|
||
| match err_msg { | ||
| Some(msg) => msg.as_ref() == self.message(), | ||
| None => true, | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,8 @@ pub mod asserts; | |
|
|
||
| pub mod executor; | ||
|
|
||
| pub mod expected_error; | ||
|
|
||
| mod mock_host; | ||
|
|
||
| pub mod utils; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,52 +18,90 @@ use rand::rngs::SmallRng; | |||||
| // HELPER MACROS | ||||||
| // ================================================================================================ | ||||||
|
|
||||||
| /// Asserts that a `Result<_, ExecError>` failed as expected. | ||||||
| /// | ||||||
| /// Three forms: | ||||||
| /// - `..., matches <pat> [if <guard>]` — matches the inner `ExecutionError` against `<pat>`. Use | ||||||
| /// this for variants other than `FailedAssertion`, or to assert on a specific `err_code`. | ||||||
| /// - `..., any` — succeeds on any `Err`. | ||||||
| /// - `..., $expected` — delegates to `ExpectedExecutionError::matches` (e.g. a `MasmError` error | ||||||
| /// code). | ||||||
| #[macro_export] | ||||||
| macro_rules! assert_execution_error { | ||||||
| ($execution_result:expr, $expected_err:expr) => { | ||||||
| ($execution_result:expr, matches $pat:pat $(if $guard:expr)? $(,)?) => { | ||||||
| match $execution_result { | ||||||
| Err($crate::ExecError(miden_processor::ExecutionError::OperationError { label: _, source_file: _, err: miden_processor::operation::OperationError::FailedAssertion { err_code, err_msg } })) => { | ||||||
| if let Some(ref msg) = err_msg { | ||||||
| assert_eq!(msg.as_ref(), $expected_err.message(), "error messages did not match"); | ||||||
| } | ||||||
| Err($crate::ExecError($pat)) $(if $guard)? => {}, | ||||||
| Ok(_) => ::core::panic!("Execution was unexpectedly successful"), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I'd include the error in the unexpectedly successful case, which helps find the issue. Applies to other unexpectedly successful output as well. |
||||||
| Err(err) => ::core::panic!( | ||||||
| "Execution error did not match `{}`: {}", | ||||||
| ::core::stringify!($pat), | ||||||
| err, | ||||||
| ), | ||||||
| } | ||||||
| }; | ||||||
|
|
||||||
| ($execution_result:expr, any $(,)?) => { | ||||||
| match $execution_result { | ||||||
| Err(_) => {}, | ||||||
| Ok(_) => ::core::panic!("Execution was unexpectedly successful"), | ||||||
| } | ||||||
| }; | ||||||
|
|
||||||
| assert_eq!( | ||||||
| err_code, $expected_err.code(), | ||||||
| "Execution failed on assertion with an unexpected error (Actual code: {}, msg: {}, Expected code: {}).", | ||||||
| err_code, err_msg.as_ref().map(|string| string.as_ref()).unwrap_or("<no message>"), $expected_err, | ||||||
| ); | ||||||
| ($execution_result:expr, $expected:expr $(,)?) => { | ||||||
| match $execution_result { | ||||||
| Err($crate::ExecError(actual)) => { | ||||||
| if !$crate::expected_error::ExpectedExecutionError::matches(&$expected, &actual) { | ||||||
| ::core::panic!( | ||||||
| "Execution error did not match expected `{}`: {}", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Nit: Newlines for better readability. Would be nice to update this across all panic messages in the macros. |
||||||
| $expected, | ||||||
| actual, | ||||||
| ); | ||||||
| } | ||||||
| }, | ||||||
| Ok(_) => panic!("Execution was unexpectedly successful"), | ||||||
| Err(err) => panic!("Execution error was not as expected: {err}"), | ||||||
| Ok(_) => ::core::panic!("Execution was unexpectedly successful"), | ||||||
| } | ||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| /// Same as [`assert_execution_error!`], but for `TransactionExecutorError`. | ||||||
| /// The `matches` and `$expected` arms unwrap | ||||||
| /// `TransactionProgramExecutionFailed(_)` and match against the inner | ||||||
| /// `ExecutionError`. | ||||||
| #[macro_export] | ||||||
| macro_rules! assert_transaction_executor_error { | ||||||
| ($execution_result:expr, $expected_err:expr) => { | ||||||
| ($execution_result:expr, matches $pat:pat $(if $guard:expr)? $(,)?) => { | ||||||
| match $execution_result { | ||||||
| Err(miden_tx::TransactionExecutorError::TransactionProgramExecutionFailed( | ||||||
| miden_processor::ExecutionError::OperationError { | ||||||
| label: _, | ||||||
| source_file: _, | ||||||
| err: miden_processor::operation::OperationError::FailedAssertion { | ||||||
| err_code, | ||||||
| err_msg, | ||||||
| }, | ||||||
| }, | ||||||
| )) => { | ||||||
| if let Some(ref msg) = err_msg { | ||||||
| assert_eq!(msg.as_ref(), $expected_err.message(), "error messages did not match"); | ||||||
| } | ||||||
| Err(miden_tx::TransactionExecutorError::TransactionProgramExecutionFailed($pat)) | ||||||
| $(if $guard)? => {}, | ||||||
| Ok(_) => ::core::panic!("Execution was unexpectedly successful"), | ||||||
| Err(err) => ::core::panic!( | ||||||
| "Execution error did not match `{}`: {}", | ||||||
| ::core::stringify!($pat), | ||||||
| err, | ||||||
| ), | ||||||
| } | ||||||
| }; | ||||||
|
|
||||||
| assert_eq!( | ||||||
| err_code, $expected_err.code(), | ||||||
| "Execution failed on assertion with an unexpected error (Actual code: {}, msg: {}, Expected: {}).", | ||||||
| err_code, err_msg.as_ref().map(|string| string.as_ref()).unwrap_or("<no message>"), $expected_err); | ||||||
| ($execution_result:expr, any $(,)?) => { | ||||||
| match $execution_result { | ||||||
| Err(_) => {}, | ||||||
| Ok(_) => ::core::panic!("Execution was unexpectedly successful"), | ||||||
| } | ||||||
| }; | ||||||
|
Comment on lines
+85
to
+90
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Asserting that some error occurred is basically always too coarse in my opinion. The error can silently change from A to B and the test would happily pass. A good test, imo, is precise about its assertions. So anything that nudges users to write precise assertions is great, and so I would suggest removing this functionality to match If users really, really want to do this, it is very simple with |
||||||
|
|
||||||
| ($execution_result:expr, $expected:expr $(,)?) => { | ||||||
| match $execution_result { | ||||||
| Err(miden_tx::TransactionExecutorError::TransactionProgramExecutionFailed(actual)) => { | ||||||
| if !$crate::expected_error::ExpectedExecutionError::matches(&$expected, &actual) { | ||||||
| ::core::panic!( | ||||||
| "Execution error did not match expected `{}`: {}", | ||||||
| $expected, | ||||||
| actual, | ||||||
| ); | ||||||
| } | ||||||
| }, | ||||||
| Ok(_) => panic!("Execution was unexpectedly successful"), | ||||||
| Err(err) => panic!("Execution error was not as expected: {err}"), | ||||||
| Ok(_) => ::core::panic!("Execution was unexpectedly successful"), | ||||||
| Err(err) => ::core::panic!("Execution error was not as expected: {err}"), | ||||||
| } | ||||||
| }; | ||||||
| } | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could move this to
MasmErroritself inmiden-protocol, so we don't need to add the public trait.