chore: improve ef-tests readability (#4136)

testing/ef-tests/src/assert.rs

@@ -6,11 +6,11 @@ use std::fmt::Debug;
 /// A helper like `assert_eq!` that instead returns `Err(Error::Assertion)` on failure.
 pub fn assert_equal<T>(left: T, right: T, msg: &str) -> Result<(), Error>
 where
-    T: Eq + Debug,
+    T: PartialEq + Debug,
 {
-    if left != right {
-        return Err(Error::Assertion(format!("{msg}. Left {:?}, right {:?}", left, right)))
-    }
-
+    if left == right {
         Ok(())
+    } else {
+        Err(Error::Assertion(format!("{msg}\n  left `{left:?}`,\n right `{right:?}`")))
+    }
 }

testing/ef-tests/src/cases/blockchain_test.rs

@@ -9,7 +9,7 @@ use reth_primitives::{BlockBody, SealedBlock};
 use reth_provider::{BlockWriter, ProviderFactory};
 use reth_rlp::Decodable;
 use reth_stages::{stages::ExecutionStage, ExecInput, Stage};
-use std::{collections::BTreeMap, ffi::OsStr, fs, path::Path, sync::Arc};
+use std::{collections::BTreeMap, fs, path::Path, sync::Arc};
 
 /// A handler for the blockchain test suite.
 #[derive(Debug)]
@@ -42,14 +42,12 @@ pub struct BlockchainTestCase {
 impl Case for BlockchainTestCase {
     fn load(path: &Path) -> Result<Self, Error> {
         Ok(BlockchainTestCase {
-            tests: fs::read_to_string(path)
-                .map_err(|e| Error::Io { path: path.into(), error: e.to_string() })
-                .and_then(|s| {
-                    serde_json::from_str(&s).map_err(|e| Error::CouldNotDeserialize {
-                        path: path.into(),
-                        error: e.to_string(),
-                    })
-                })?,
+            tests: {
+                let s = fs::read_to_string(path)
+                    .map_err(|error| Error::Io { path: path.into(), error })?;
+                serde_json::from_str(&s)
+                    .map_err(|error| Error::CouldNotDeserialize { path: path.into(), error })?
+            },
             skip: should_skip(path),
         })
     }
@@ -130,66 +128,58 @@ impl Case for BlockchainTestCase {
     }
 }
 
-/// Tests are test edge cases that are not possible to happen on mainnet, so we are skipping them.
+/// Returns whether the test at the given path should be skipped.
+///
+/// Some tests are edge cases that cannot happen on mainnet, while others are skipped for
+/// convenience (e.g. they take a long time to run) or are temporarily disabled.
+///
+/// The reason should be documented in a comment above the file name(s).
 pub fn should_skip(path: &Path) -> bool {
+    let path_str = path.to_str().expect("Path is not valid UTF-8");
+    let name = path.file_name().unwrap().to_str().unwrap();
+    matches!(
+        name,
         // funky test with `bigint 0x00` value in json :) not possible to happen on mainnet and require
         // custom json parser. https://github.com/ethereum/tests/issues/971
-    if path.file_name() == Some(OsStr::new("ValueOverflow.json")) {
-        return true
-    }
+        | "ValueOverflow.json"
+
         // txbyte is of type 02 and we dont parse tx bytes for this test to fail.
-    if path.file_name() == Some(OsStr::new("typeTwoBerlin.json")) {
-        return true
-    }
+        | "typeTwoBerlin.json"
+
         // Test checks if nonce overflows. We are handling this correctly but we are not parsing
         // exception in testsuite There are more nonce overflow tests that are in internal
         // call/create, and those tests are passing and are enabled.
-    if path.file_name() == Some(OsStr::new("CreateTransactionHighNonce.json")) {
-        return true
-    }
+        | "CreateTransactionHighNonce.json"
 
         // Test check if gas price overflows, we handle this correctly but does not match tests specific
         // exception.
-    if path.file_name() == Some(OsStr::new("HighGasPrice.json")) {
-        return true
-    }
+        | "HighGasPrice.json"
 
         // Skip test where basefee/accesslist/difficulty is present but it shouldn't be supported in
         // London/Berlin/TheMerge. https://github.com/ethereum/tests/blob/5b7e1ab3ffaf026d99d20b17bb30f533a2c80c8b/GeneralStateTests/stExample/eip1559.json#L130
         // It is expected to not execute these tests.
-    if path.file_name() == Some(OsStr::new("accessListExample.json")) ||
-        path.file_name() == Some(OsStr::new("basefeeExample.json")) ||
-        path.file_name() == Some(OsStr::new("eip1559.json")) ||
-        path.file_name() == Some(OsStr::new("mergeTest.json"))
-    {
-        return true
-    }
+        | "accessListExample.json"
+        | "basefeeExample.json"
+        | "eip1559.json"
+        | "mergeTest.json"
 
         // These tests are passing, but they take a lot of time to execute so we are going to skip them.
-    if path.file_name() == Some(OsStr::new("loopExp.json")) ||
-        path.file_name() == Some(OsStr::new("Call50000_sha256.json")) ||
-        path.file_name() == Some(OsStr::new("static_Call50000_sha256.json")) ||
-        path.file_name() == Some(OsStr::new("loopMul.json")) ||
-        path.file_name() == Some(OsStr::new("CALLBlake2f_MaxRounds.json")) ||
-        path.file_name() == Some(OsStr::new("shiftCombinations.json"))
-    {
-        return true
-    }
-
-    // Ignore outdated EOF tests that haven't been updated for Cancun yet.
-    let eof_path = Path::new("EIPTests").join("stEOF");
-    if path.to_string_lossy().contains(&*eof_path.to_string_lossy()) {
-        return true
-    }
-
-    if path.file_name() == Some(OsStr::new("ValueOverflow.json")) {
-        return true
-    }
+        | "loopExp.json"
+        | "Call50000_sha256.json"
+        | "static_Call50000_sha256.json"
+        | "loopMul.json"
+        | "CALLBlake2f_MaxRounds.json"
+        | "shiftCombinations.json"
 
         // TODO: re-enable when blobtx are supported
-    if path.file_name() == Some(OsStr::new("blobtxExample.json")) {
-        return true
-    }
-
-    false
+        | "blobtxExample.json"
+    )
+    // Ignore outdated EOF tests that haven't been updated for Cancun yet.
+    || path_contains(path_str, &["EIPTests", "stEOF"])
+}
+
+/// `str::contains` but for a path. Takes into account the OS path separator (`/` or `\`).
+fn path_contains(path_str: &str, rhs: &[&str]) -> bool {
+    let rhs = rhs.join(std::path::MAIN_SEPARATOR_STR);
+    path_str.contains(&rhs)
 }

testing/ef-tests/src/models.rs

@@ -216,7 +216,7 @@ impl Account {
         Tx: DbTx<'a>,
     {
         let account = tx.get::<tables::PlainAccountState>(address)?.ok_or_else(|| {
-            Error::Assertion(format!("Account is missing ({address}) expected: {:?}", self))
+            Error::Assertion(format!("Expected account ({address:?}) is missing from DB: {self:?}"))
         })?;
 
         assert_equal(self.balance.into(), account.balance, "Balance does not match")?;

testing/ef-tests/src/result.rs

@@ -10,7 +10,7 @@ use thiserror::Error;
 /// # Note
 ///
 /// `Error::Skipped` should not be treated as a test failure.
-#[derive(Error, Debug, Clone)]
+#[derive(Debug, Error)]
 #[non_exhaustive]
 pub enum Error {
     /// The test was skipped
@@ -22,7 +22,8 @@ pub enum Error {
         /// The path to the file or directory
         path: PathBuf,
         /// The specific error
-        error: String,
+        #[source]
+        error: std::io::Error,
     },
     /// A deserialization error occurred
     #[error("An error occurred deserializing the test at {path}: {error}")]
@@ -30,7 +31,8 @@ pub enum Error {
         /// The path to the file we wanted to deserialize
         path: PathBuf,
         /// The specific error
-        error: String,
+        #[source]
+        error: serde_json::Error,
     },
     /// A database error occurred.
     #[error(transparent)]
@@ -116,8 +118,7 @@ pub(crate) fn print_results(
     }
 
     for case in failed {
-        let error = case.result.clone().unwrap_err();
-
+        let error = case.result.as_ref().unwrap_err();
         println!("[!] Case {} failed (description: {}): {}", case.path.display(), case.desc, error);
     }
 }