From eea90ca348f511ec3ce715155f785135fcd26d6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Bylica?= Date: Mon, 7 Sep 2026 11:22:43 +0200 Subject: [PATCH] test: Make a fixture file the only kind of test Naming a file collected one test per fixture in it, which naming a directory could not afford: enumerating fixtures means loading every file, and a release is gigabytes. The two forms counted, filtered and skipped differently, and only one of them scaled. A file is now one test either way, and collection loads nothing, so it can no longer fail. Three consequences a reader should not have to infer from the diff: --collect-only on a named file lists the file rather than what is inside it, a -k which selects nothing from a named file now passes, as it already did for a directory, and --ignore applies to a named file as well, relative to that file. Claude-Session: https://claude.ai/code/session_016UHPAGwcwXMjqhTLpT31K7 --- test/blockchaintest/blockchaintest.cpp | 84 +++++------------ .../test/blockchaintest/CMakeLists.txt | 26 ++++-- .../evmone-cli/test/statetest/CMakeLists.txt | 48 +++++++--- test/statetest/statetest.cpp | 91 ++++++------------- 4 files changed, 108 insertions(+), 141 deletions(-) diff --git a/test/blockchaintest/blockchaintest.cpp b/test/blockchaintest/blockchaintest.cpp index 2b4bf69070..3e736f5305 100644 --- a/test/blockchaintest/blockchaintest.cpp +++ b/test/blockchaintest/blockchaintest.cpp @@ -15,62 +15,28 @@ using evmone::test::TestCase; namespace { -/// Adds to @p cases every test under @p root: one per file for a directory, one per test case in -/// the file when the file itself is named. Returns whether every test was collected. -bool collect_tests(std::vector& cases, const fs::path& root, +/// Adds to @p cases one test per fixture file under @p root, which is that file itself when it +/// is not a directory. +void collect_tests(std::vector& cases, const fs::path& root, std::span ignored, evmc::VM& vm) { - if (is_directory(root)) + // A file named directly is its own collection; the ignored paths are relative to the + // directory holding it, as they are to a directory named directly. + const auto is_dir = is_directory(root); + auto files = is_dir ? evmone::test::collect_test_files(root) : std::vector{root}; + evmone::test::ignore_test_files(files, is_dir ? root : root.parent_path(), ignored); + + cases.reserve(cases.size() + files.size()); + for (const auto& path : files) { - auto files = evmone::test::collect_test_files(root); - evmone::test::ignore_test_files(files, root, ignored); - cases.reserve(cases.size() + files.size()); - for (const auto& path : files) - { - // Loaded when the test runs: loading a whole tree up front costs far more. A - // load which throws over an unsupported fixture reaches the driver, which skips. - cases.push_back({path.string(), [path, &vm](evmone::test::TestReport& report) { - std::ifstream f{path}; - for (const auto& test : evmone::test::load_blockchain_tests(f)) - evmone::test::run_blockchain_test(test, vm, report); - }}); - } + // Loaded when the test runs: loading a whole tree up front costs far more. A + // load which throws over an unsupported fixture reaches the driver, which skips. + cases.push_back({path.string(), [path, &vm](evmone::test::TestReport& report) { + std::ifstream f{path}; + for (const auto& test : evmone::test::load_blockchain_tests(f)) + evmone::test::run_blockchain_test(test, vm, report); + }}); } - else // Treat as a file. - { - // Naming a file loads it now, to name the test cases in it. One which cannot be - // loaded becomes a single test the driver skips or fails. - std::vector tests; - try - { - std::ifstream f{root}; - tests = evmone::test::load_blockchain_tests(f); - } - catch (const evmone::test::UnsupportedTestFeature&) - { - // An unsupported fixture is a skip, not a broken collection. - cases.push_back({root.string(), - [error = std::current_exception()](auto&) { std::rethrow_exception(error); }}); - return true; - } - catch (const std::exception& ex) - { - // Also reported here: --collect-only never runs the test. - std::cerr << root.string() << ": " << ex.what() << '\n'; - cases.push_back({root.string(), - [error = std::current_exception()](auto&) { std::rethrow_exception(error); }}); - return false; - } - - for (const auto& test : tests) - { - cases.push_back( - {root.string() + "::" + test.name, [test, &vm](evmone::test::TestReport& report) { - evmone::test::run_blockchain_test(test, vm, report); - }}); - } - } - return true; } } // namespace @@ -85,15 +51,14 @@ int main(int argc, char* argv[]) std::vector paths; app.add_option("path", paths, - "Path to test file or directory. For a directory, all .json " - "files (except index.json) are considered test files, and each file is treated as a " - "separate test. For a file, all tests in the file are treated as separate tests.") + "Path to a test file or a directory of them. Under a directory every .json " + "file except index.json is one test; a file named directly is one test.") ->required() ->check(CLI::ExistingPath); std::vector ignored; app.add_option("--ignore", ignored, - "Path, relative to a test directory, not to collect tests from. May be given more " + "Path, relative to a given path, not to collect tests from. May be given more " "than once. Whole path components are matched, so --ignore bc4895 keeps " "bc4895-withdrawals.") // Without this the option is variadic and swallows the positional paths after it. @@ -114,15 +79,12 @@ int main(int argc, char* argv[]) vm.set_option("trace", "1"); std::vector cases; - bool all_collected = true; for (const auto& p : paths) - all_collected &= collect_tests(cases, p, ignored, vm); + collect_tests(cases, p, ignored, vm); const evmone::test::RunOptions options{ .collect_only = collect_only, .progress = !trace_flag}; - const auto exit_code = evmone::test::run_tests(cases, std::cout, options); - // A file which could not be loaded fails the listing too, not only a run of it. - return all_collected ? exit_code : evmone::test::TESTS_FAILED; + return evmone::test::run_tests(cases, std::cout, options); } catch (const std::exception& ex) { diff --git a/test/integration/evmone-cli/test/blockchaintest/CMakeLists.txt b/test/integration/evmone-cli/test/blockchaintest/CMakeLists.txt index ab8011b1fa..f378215cf4 100644 --- a/test/integration/evmone-cli/test/blockchaintest/CMakeLists.txt +++ b/test/integration/evmone-cli/test/blockchaintest/CMakeLists.txt @@ -13,8 +13,8 @@ add_test( ) set_tests_properties( ${PREFIX}/json_test PROPERTIES - # Make sure both tests in the file are executed (both should fail). - PASS_REGULAR_EXPRESSION "2 failed, 0 passed" + # Both fixtures of the file run and both fail; the file itself counts once. + PASS_REGULAR_EXPRESSION "-call\\]:.*-callcode\\]:.*1 failed, 0 passed" ) # Exercise block-level gas accounting (EIP-7778), with tracing on so that flag is covered too. @@ -94,22 +94,36 @@ set_tests_properties( FAIL_REGULAR_EXPRESSION "unsupported_rlp" ) -# Given the file directly, each test case in it is listed on its own. +# A file named directly is listed as itself, once, whatever it holds. add_test( NAME ${PREFIX}/collect_only_file COMMAND evmone-blockchaintest ${TESTS1}/test.json --collect-only ) set_tests_properties( ${PREFIX}/collect_only_file PROPERTIES - PASS_REGULAR_EXPRESSION "test\\.json::[^\n]*-call\\]\n[^\n]*test\\.json::[^\n]*-callcode\\]" + PASS_REGULAR_EXPRESSION "^[^\n]*test\\.json\n$" ) -# A file which cannot be loaded is a failure of the listing too, not a name in it. +# Collection reads no file, so one which cannot be parsed is listed like any other. It fails +# when it runs, which is the only time anything reads it. add_test( NAME ${PREFIX}/collect_only_unloadable COMMAND evmone-blockchaintest ${TESTS1}/not_json.txt --collect-only ) -set_tests_properties(${PREFIX}/collect_only_unloadable PROPERTIES WILL_FAIL TRUE) +set_tests_properties( + ${PREFIX}/collect_only_unloadable PROPERTIES + PASS_REGULAR_EXPRESSION "^[^\n]*not_json\\.txt\n$" +) + +# Pointing at a not-json file produces a failure during test execution. +add_test( + NAME ${PREFIX}/run_unloadable + COMMAND evmone-blockchaintest ${TESTS1}/not_json.txt +) +set_tests_properties( + ${PREFIX}/run_unloadable PROPERTIES + PASS_REGULAR_EXPRESSION "1 failed, 0 passed" +) get_directory_property(ALL_TESTS TESTS) set_tests_properties(${ALL_TESTS} PROPERTIES ENVIRONMENT LLVM_PROFILE_FILE=${CMAKE_BINARY_DIR}/integration-%p.profraw) diff --git a/test/integration/evmone-cli/test/statetest/CMakeLists.txt b/test/integration/evmone-cli/test/statetest/CMakeLists.txt index b5bd5fec53..6fd23e62a0 100644 --- a/test/integration/evmone-cli/test/statetest/CMakeLists.txt +++ b/test/integration/evmone-cli/test/statetest/CMakeLists.txt @@ -30,25 +30,25 @@ set_tests_properties( PASS_REGULAR_EXPRESSION "tests1[^\n]*T\\.json\n[^\n]*tests1[^\n]*test1\\.json\n[^\n]*tests1[^\n]*test2_multi\\.json" ) -# Given the file directly, each test case in it is listed on its own. +# A file named directly is listed as itself, whatever it holds. add_test( NAME ${PREFIX}/single_file_list COMMAND evmone-statetest ${TESTS1}/SuiteA/test2_multi.json --collect-only ) set_tests_properties( ${PREFIX}/single_file_list PROPERTIES - PASS_REGULAR_EXPRESSION "test2_multi\\.json::test_case_1\n[^\n]*test2_multi\\.json::test_case_2" + PASS_REGULAR_EXPRESSION "^[^\n]*test2_multi\\.json\n$" ) # Several roots are collected in the order given, not regrouped by suite as gtest listed them. -# T.json holds no test cases, so naming it directly contributes no line. +# T.json holds no test case, but naming it is naming a test, so it is listed like any other. add_test( NAME ${PREFIX}/multiple_args_list COMMAND evmone-statetest ${TESTS1} ${TESTS2} ${TESTS1}/B/T.json ${TESTS1}/SuiteA --collect-only ) set_tests_properties( ${PREFIX}/multiple_args_list PROPERTIES - PASS_REGULAR_EXPRESSION "tests1[^\n]*T\\.json\n[^\n]*tests1[^\n]*test1\\.json\n[^\n]*tests1[^\n]*test2_multi\\.json\n[^\n]*tests2[^\n]*test1\\.json\n[^\n]*tests1[^\n]*test1\\.json\n[^\n]*tests1[^\n]*test2_multi\\.json" + PASS_REGULAR_EXPRESSION "tests1[^\n]*T\\.json\n[^\n]*tests1[^\n]*test1\\.json\n[^\n]*tests1[^\n]*test2_multi\\.json\n[^\n]*tests2[^\n]*test1\\.json\n[^\n]*tests1[^\n]*T\\.json\n[^\n]*tests1[^\n]*test1\\.json\n[^\n]*tests1[^\n]*test2_multi\\.json" ) add_test( @@ -66,8 +66,8 @@ add_test( ) set_tests_properties( ${PREFIX}/multi_test PROPERTIES - # Make sure both tests in the file are executed (both should fail). - PASS_REGULAR_EXPRESSION "test_case_1.*test_case_2" + # Both cases of the file run and both fail; the file itself counts once. + PASS_REGULAR_EXPRESSION "test_case_1.*test_case_2.*1 failed, 0 passed" ) add_test( @@ -96,9 +96,8 @@ set_tests_properties( FAIL_REGULAR_EXPRESSION "failing_test_case" ) -# Over a directory the filter is applied per case inside the file's test, not at registration as -# it is above. The summary line is what proves a case ran: forbidding the other name alone would -# hold just as well if the filter dropped every case. +# The filter is applied per case inside the file's test. The summary line is what proves a case +# ran: forbidding the other name alone would hold just as well if the filter dropped every case. add_test( NAME ${PREFIX}/filter_directory COMMAND evmone-statetest ${TESTS_FILTER} -k passing_test_case --trace-summary @@ -121,6 +120,17 @@ set_tests_properties( FAIL_REGULAR_EXPRESSION "T\\.json" ) +# A file named directly is ignored the same way, by a path relative to that file. +add_test( + NAME ${PREFIX}/ignore_file + COMMAND evmone-statetest --ignore test1.json + ${TESTS1}/SuiteA/test1.json ${TESTS1}/SuiteA/test2_multi.json --collect-only +) +set_tests_properties( + ${PREFIX}/ignore_file PROPERTIES + PASS_REGULAR_EXPRESSION "^[^\n]*test2_multi\\.json\n$" +) + # Selecting nothing fails rather than passing vacuously. WILL_FAIL only asserts a nonzero exit; # the driver unit tests pin the code itself. add_test( @@ -129,12 +139,26 @@ add_test( ) set_tests_properties(${PREFIX}/nothing_collected PROPERTIES WILL_FAIL TRUE) -# A file which cannot be loaded is a failure of the listing too, not a name in it. +# Collection reads no file, so one which cannot be parsed is listed like any other. It fails +# when it runs, which is the only time anything reads it. add_test( NAME ${PREFIX}/collect_only_unloadable - COMMAND evmone-statetest ${CMAKE_CURRENT_SOURCE_DIR}/tests1/SuiteA/notes.txt --collect-only + COMMAND evmone-statetest ${TESTS1}/SuiteA/notes.txt --collect-only +) +set_tests_properties( + ${PREFIX}/collect_only_unloadable PROPERTIES + PASS_REGULAR_EXPRESSION "^[^\n]*notes\\.txt\n$" +) + +# Pointing at a not-json file produces a failure during test execution. +add_test( + NAME ${PREFIX}/run_unloadable + COMMAND evmone-statetest ${TESTS1}/SuiteA/notes.txt +) +set_tests_properties( + ${PREFIX}/run_unloadable PROPERTIES + PASS_REGULAR_EXPRESSION "1 failed, 0 passed" ) -set_tests_properties(${PREFIX}/collect_only_unloadable PROPERTIES WILL_FAIL TRUE) get_directory_property(ALL_TESTS TESTS) set_tests_properties(${ALL_TESTS} PROPERTIES ENVIRONMENT LLVM_PROFILE_FILE=${CMAKE_BINARY_DIR}/integration-%p.profraw) diff --git a/test/statetest/statetest.cpp b/test/statetest/statetest.cpp index f2f3c24ea4..ea14b4ba99 100644 --- a/test/statetest/statetest.cpp +++ b/test/statetest/statetest.cpp @@ -15,67 +15,38 @@ using evmone::test::TestCase; namespace { -/// Adds to @p cases every test under @p root: one per file for a directory, one per test case in -/// the file when the file itself is named. Returns whether every test was collected. -bool collect_tests(std::vector& cases, const fs::path& root, +/// Adds to @p cases one test per fixture file under @p root, which is that file itself when it +/// is not a directory. +void collect_tests(std::vector& cases, const fs::path& root, const std::optional& filter, std::span ignored, evmc::VM& vm, bool trace) { - // Which cases -k keeps. Over a directory it selects within the file's test, because - // naming the cases up front would mean loading the whole tree. + // Which cases -k keeps. It selects within the file's test, because naming the cases up front + // would mean loading the whole tree. const auto selected = [&filter](const evmone::test::StateTransitionTest& test) { return !filter.has_value() || test.name.find(*filter) != std::string::npos; }; - if (is_directory(root)) - { - auto files = evmone::test::collect_test_files(root); - evmone::test::ignore_test_files(files, root, ignored); - cases.reserve(cases.size() + files.size()); - for (const auto& path : files) - { - // Loaded when the test runs: loading a whole tree up front costs far more. - cases.push_back( - {path.string(), [path, selected, &vm, trace](evmone::test::TestReport& report) { - std::ifstream f{path}; - for (const auto& test : evmone::test::load_state_tests(f)) - { - if (selected(test)) - evmone::test::run_state_test(test, vm, trace, report); - } - }}); - } - } - else // Treat as a file. - { - // Naming a file loads it now, to name the test cases in it. One which cannot be - // loaded becomes a single test reporting why. - std::vector tests; - try - { - std::ifstream f{root}; - tests = evmone::test::load_state_tests(f); - } - catch (const std::exception& ex) - { - // Also reported here: --collect-only never runs the test. - std::cerr << root.string() << ": " << ex.what() << '\n'; - cases.push_back({root.string(), - [error = std::current_exception()](auto&) { std::rethrow_exception(error); }}); - return false; - } + // A file named directly is its own collection; the ignored paths are relative to the + // directory holding it, as they are to a directory named directly. + const auto is_dir = is_directory(root); + auto files = is_dir ? evmone::test::collect_test_files(root) : std::vector{root}; + evmone::test::ignore_test_files(files, is_dir ? root : root.parent_path(), ignored); - for (const auto& test : tests) - { - if (!selected(test)) - continue; - cases.push_back({root.string() + "::" + test.name, - [test, &vm, trace](evmone::test::TestReport& report) { - evmone::test::run_state_test(test, vm, trace, report); - }}); - } + cases.reserve(cases.size() + files.size()); + for (const auto& path : files) + { + // Loaded when the test runs: loading a whole tree up front costs far more. + cases.push_back( + {path.string(), [path, selected, &vm, trace](evmone::test::TestReport& report) { + std::ifstream f{path}; + for (const auto& test : evmone::test::load_state_tests(f)) + { + if (selected(test)) + evmone::test::run_state_test(test, vm, trace, report); + } + }}); } - return true; } } // namespace @@ -90,19 +61,18 @@ int main(int argc, char* argv[]) std::vector paths; app.add_option("path", paths, - "Path to test file or directory. For a directory, all .json " - "files (except index.json) are considered test files, and each file is treated as a " - "separate test. For a file, all tests in the file are treated as separate tests.") + "Path to a test file or a directory of them. Under a directory every .json " + "file except index.json is one test; a file named directly is one test.") ->required() ->check(CLI::ExistingPath); std::optional filter; app.add_option("-k", filter, - "Test name filter. Run only tests with names containing the specified string."); + "Test case name filter. Run only the cases whose name contains the given string."); std::vector ignored; app.add_option("--ignore", ignored, - "Path, relative to a test directory, not to collect tests from. May be given more " + "Path, relative to a given path, not to collect tests from. May be given more " "than once. Whole path components are matched, so --ignore bc4895 keeps " "bc4895-withdrawals.") // Without this the option is variadic and swallows the positional paths after it. @@ -129,15 +99,12 @@ int main(int argc, char* argv[]) } std::vector cases; - bool all_collected = true; for (const auto& p : paths) - all_collected &= collect_tests(cases, p, filter, ignored, vm, trace || trace_summary); + collect_tests(cases, p, filter, ignored, vm, trace || trace_summary); const evmone::test::RunOptions options{ .collect_only = collect_only, .progress = !(trace || trace_summary)}; - const auto exit_code = evmone::test::run_tests(cases, std::cout, options); - // A file which could not be loaded fails the listing too, not only a run of it. - return all_collected ? exit_code : evmone::test::TESTS_FAILED; + return evmone::test::run_tests(cases, std::cout, options); } catch (const std::exception& ex) {