feat(levm): ensure errors are right (adaptation) #1459
Conversation
| let error_reason = format!("Expected exception: {expected_exception}"); | ||
| println!("Expected exception: {expected_exception}"); | ||
| let error_reason = format!("Expected exception: {expected_exception:?}"); | ||
| println!("Expected exception: {expected_exception:?}"); |
There was a problem hiding this comment.
Quick question, couldn't the println print the "error_reason" variable instead of the re-writing the string?
There was a problem hiding this comment.
Removed the print in Remove exception print, already prints in report
| // Instead of cloning could use references | ||
| if !exception_is_expected(expected_exceptions.clone(), err.clone()) { | ||
| let error_reason = match expected_exceptions.get(1) { | ||
| Some(second_exception) => { |
There was a problem hiding this comment.
Leave a little comment here explaining that in tests there are at most 2 exceptions, that's why even though it is a vec we only care about the first 2 exceptions. And that changing it is not worth it yet.
It doesn't have to be long, something concise just to inform the reader
There was a problem hiding this comment.
| @@ -180,8 +238,22 @@ pub fn ensure_post_state( | |||
| Ok(execution_report) => { | |||
| match test.post.vector_post_value(vector).expect_exception { | |||
| // Execution result was successful but an exception was expected. | |||
| Some(expected_exception) => { | |||
| let error_reason = format!("Expected exception: {expected_exception}"); | |||
| Some(expected_exceptions) => { | |||
There was a problem hiding this comment.
| @@ -392,6 +395,9 @@ pub fn _run_ef_test_revm(test: &EFTest) -> Result<EFTestReport, EFTestRunnerErro | |||
| Err(EFTestRunnerError::Internal(reason)) => { | |||
| return Err(EFTestRunnerError::Internal(reason)); | |||
| } | |||
| Err(EFTestRunnerError::ExpectedExceptionDoesNotMatchReceived(_)) => { | |||
| // Here should be the logic of execution and expecting an error | |||
There was a problem hiding this comment.
Clarify this please. It means that exception doesn't match the received but this is not implemented for REVM, right? Because we only do that comparison with LEVM
There was a problem hiding this comment.
| @@ -53,6 +53,9 @@ pub fn re_run_failed_ef_test( | |||
| } | |||
| } | |||
| }, | |||
| EFTestRunnerError::ExpectedExceptionDoesNotMatchReceived(_) => { | |||
| // Here should be the logic of re-execution and compare revm result with the expected error | |||
There was a problem hiding this comment.
Clarify this too. Can this error happen with revm? What do we want to do?
There was a problem hiding this comment.
Co-authored-by: Jeremías Salomón <48994069+JereSalo@users.noreply.github.com>
Motivation
This PR is a revival of an older PR that implemented this feature, and merged the branch in another which PR was closed. This is an adaptation of it to the current codebase.
Description
See #1225 for more details.