From 072c84083c58a95b5b9245136017379501a9ceb2 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Thu, 10 Aug 2023 13:06:30 +0200 Subject: [PATCH] chore: improve ef-tests readability (#4136) --- testing/ef-tests/src/assert.rs | 10 +- testing/ef-tests/src/cases/blockchain_test.rs | 120 ++++++++---------- testing/ef-tests/src/models.rs | 2 +- testing/ef-tests/src/result.rs | 11 +- 4 files changed, 67 insertions(+), 76 deletions(-) diff --git a/testing/ef-tests/src/assert.rs b/testing/ef-tests/src/assert.rs index 8db027f6c..9f1f83e00 100644 --- a/testing/ef-tests/src/assert.rs +++ b/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(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:?}`"))) } - - Ok(()) } diff --git a/testing/ef-tests/src/cases/blockchain_test.rs b/testing/ef-tests/src/cases/blockchain_test.rs index e2f7fc8ac..7cd863646 100644 --- a/testing/ef-tests/src/cases/blockchain_test.rs +++ b/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 { 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 { - // 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 - } - // 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 - } - // 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 - } + 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 + | "ValueOverflow.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 - } + // txbyte is of type 02 and we dont parse tx bytes for this test to fail. + | "typeTwoBerlin.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 - } + // 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. + | "CreateTransactionHighNonce.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 - } + // Test check if gas price overflows, we handle this correctly but does not match tests specific + // exception. + | "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. + | "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. + | "loopExp.json" + | "Call50000_sha256.json" + | "static_Call50000_sha256.json" + | "loopMul.json" + | "CALLBlake2f_MaxRounds.json" + | "shiftCombinations.json" + + // TODO: re-enable when blobtx are supported + | "blobtxExample.json" + ) // 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 - } - - // TODO: re-enable when blobtx are supported - if path.file_name() == Some(OsStr::new("blobtxExample.json")) { - return true - } - - false + || 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) } diff --git a/testing/ef-tests/src/models.rs b/testing/ef-tests/src/models.rs index 44e9c9b3a..7276174b4 100644 --- a/testing/ef-tests/src/models.rs +++ b/testing/ef-tests/src/models.rs @@ -216,7 +216,7 @@ impl Account { Tx: DbTx<'a>, { let account = tx.get::(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")?; diff --git a/testing/ef-tests/src/result.rs b/testing/ef-tests/src/result.rs index 5aaf58a9e..0aabf560a 100644 --- a/testing/ef-tests/src/result.rs +++ b/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); } }