Extend execution-error assertion macros with patter and any arms#2897
Extend execution-error assertion macros with patter and any arms#2897marijamijailovic wants to merge 2 commits into
Conversation
|
@marijamijailovic Would you mind fixing the broken lint workflow? Seems like something is ill-formatted. |
marijamijailovic
force-pushed
the
pr/execution-error-assert-macros
branch
from
969c4de to
0702a68
Compare
|
|
Changelog check is failing now – should we add the |
mmagician
added
the
no changelog
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good! I left a few small comments.
| Err($crate::ExecError(actual)) => { | ||
| if !$crate::expected_error::ExpectedExecutionError::matches(&$expected, &actual) { | ||
| ::core::panic!( | ||
| "Execution error did not match expected `{}`: {}", |
There was a problem hiding this comment.
| "Execution error did not match expected `{}`: {}", | |
| "Execution error did not match:\nexpected: {}\nactual: {}", |
Nit: Newlines for better readability. Would be nice to update this across all panic messages in the macros.
| 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"), |
There was a problem hiding this comment.
| Ok(_) => ::core::panic!("Execution was unexpectedly successful"), | |
| Ok(_) => ::core::panic!("Execution was unexpectedly successful\nexpected error: {}", err), |
I'd include the error in the unexpectedly successful case, which helps find the issue. Applies to other unexpectedly successful output as well.
| ($execution_result:expr, any $(,)?) => { | ||
| match $execution_result { | ||
| Err(_) => {}, | ||
| Ok(_) => ::core::panic!("Execution was unexpectedly successful"), | ||
| } | ||
| }; |
There was a problem hiding this comment.
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 any error.
If users really, really want to do this, it is very simple with assert!(res.is_err()).
| let core_lib = CoreLibrary::default(); | ||
| let assembler = Assembler::new(Arc::new(DefaultSourceManager::default())) | ||
| .with_dynamic_library(&core_lib) | ||
| .expect("CoreLibrary should load"); | ||
| let program = assembler.assemble_program(src).expect("MASM should assemble"); | ||
|
|
||
| let mut host = DefaultHost::default(); | ||
| host.load_library(core_lib.mast_forest()) | ||
| .expect("CoreLibrary mast forest should load"); | ||
|
|
||
| let stack = StackInputs::new(&[]).expect("empty stack inputs are valid"); | ||
| FastProcessor::new(stack) | ||
| .with_options(options) | ||
| .execute(&program, &mut host) | ||
| .await | ||
| .map_err(ExecError::new) |
There was a problem hiding this comment.
I would replace this with CodeExecutor, if possible. It would be one less place to maintain a FastProcessor invocation.
| impl ExpectedExecutionError for MasmError { | ||
| fn matches(&self, actual: &ExecutionError) -> bool { |
There was a problem hiding this comment.
I think we could move this to MasmError itself in miden-protocol, so we don't need to add the public trait.
4 participants
Closes #2807
Adds two new arms to
assert_execution_error!andassert_transaction_executor_error!:matches <pat> [if <guard>]- pattern-match the innerExecutionErrordirectly, with an optional guard. Useful for variants other thanFailedAssertion, or to assert on a specificerr_code.any- accept anyErr.The existing
$expectedarm still works as before; tests passingMasmErrorconstants don't change.New
tests/assertion_macros.rs:#[should_panic]checks for the failure paths.FastProcessorto trigger and assert realExecutionErrorvariants end-to-end (DivideByZero,CycleLimitExceeded,MemoryError::UnalignedWordAccess,AdviceError::StackReadFailed,EventError, etc).