From b45ee3363bb76dd23ee8ea5fcd174712d0630b66 Mon Sep 17 00:00:00 2001 From: Mario Vega Date: Wed, 31 Jul 2024 15:34:25 -0600 Subject: [PATCH] fix(plugins/filler): Index generation concurrency issue (#725) * fix(plugins/filler): Index generation concurrency issue * changelog * fix(plugins/filler): tests * fix(plugins/filler): fix parameter group --- docs/CHANGELOG.md | 1 + src/cli/pytest_commands/fill.py | 16 +++-- src/pytest_plugins/filler/filler.py | 64 +++++++++++++++++-- .../filler/tests/test_filler.py | 26 +++++--- 4 files changed, 87 insertions(+), 20 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 20138f7a5c7..b179daa5953 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -14,6 +14,7 @@ Test fixtures for use by clients are available for each release on the [Github r - ✨ `consume hive` command is now available to run all types of hive tests ([#712](https://github.com/ethereum/execution-spec-tests/pull/712)). - ✨ Generated fixtures now contain the test index `index.json` by default ([#716](https://github.com/ethereum/execution-spec-tests/pull/716)). - ✨ A metadata folder `.meta/` now stores all fixture metadata files by default ([#721](https://github.com/ethereum/execution-spec-tests/pull/721)). +- 🐞 Fixed `fill` command index generation issue due to concurrency ([#725](https://github.com/ethereum/execution-spec-tests/pull/725)). ### 🔧 EVM Tools diff --git a/src/cli/pytest_commands/fill.py b/src/cli/pytest_commands/fill.py index 343076f18b1..aa73d1d9bee 100644 --- a/src/cli/pytest_commands/fill.py +++ b/src/cli/pytest_commands/fill.py @@ -3,6 +3,7 @@ """ import sys +from tempfile import TemporaryDirectory from typing import List import click @@ -65,11 +66,12 @@ def fill( """ Entry point for the fill command. """ - result = pytest.main( - handle_fill_command_flags( - pytest_args, - help_flag, - pytest_help_flag, - ), - ) + with TemporaryDirectory() as temp_dir: + result = pytest.main( + handle_fill_command_flags( + [f"--session-temp-folder={temp_dir}", "--index", *pytest_args], + help_flag, + pytest_help_flag, + ), + ) sys.exit(result) diff --git a/src/pytest_plugins/filler/filler.py b/src/pytest_plugins/filler/filler.py index 0697fbc957e..7f93b4f5481 100644 --- a/src/pytest_plugins/filler/filler.py +++ b/src/pytest_plugins/filler/filler.py @@ -5,7 +5,7 @@ and that modifies pytest hooks in order to fill test specs for all tests and writes the generated fixtures to file. """ - +import argparse import configparser import datetime import os @@ -15,6 +15,7 @@ from typing import Generator, List, Type import pytest +from filelock import FileLock from pytest_metadata.plugin import metadata_key # type: ignore from cli.gen_index import generate_fixtures_index @@ -185,6 +186,13 @@ def pytest_addoption(parser: pytest.Parser): type=str, help="Specify a build name for the fixtures.ini file, e.g., 'stable'.", ) + test_group.addoption( + "--index", + action="store_true", + dest="generate_index", + default=False, + help="Generate an index file for all produced fixtures.", + ) debug_group = parser.getgroup("debug", "Arguments defining debug behavior") debug_group.addoption( @@ -196,6 +204,16 @@ def pytest_addoption(parser: pytest.Parser): help="Path to dump the transition tool debug output.", ) + internal_group = parser.getgroup("internal", "Internal arguments") + internal_group.addoption( + "--session-temp-folder", + action="store", + dest="session_temp_folder", + type=Path, + default=None, + help=argparse.SUPPRESS, + ) + @pytest.hookimpl(tryfirst=True) def pytest_configure(config): @@ -610,6 +628,16 @@ def get_fixture_collection_scope(fixture_name, config): return "module" +@pytest.fixture(scope="session") +def session_temp_folder(request) -> Path | None: # noqa: D103 + return request.config.option.session_temp_folder + + +@pytest.fixture(scope="session") +def generate_index(request) -> bool: # noqa: D103 + return request.config.option.generate_index + + @pytest.fixture(scope=get_fixture_collection_scope) def fixture_collector( request: pytest.FixtureRequest, @@ -618,11 +646,29 @@ def fixture_collector( filler_path: Path, base_dump_dir: Path | None, output_dir: Path, + session_temp_folder: Path | None, + generate_index: bool, ) -> Generator[FixtureCollector, None, None]: """ Returns the configured fixture collector instance used for all tests in one test module. """ + if session_temp_folder is not None: + fixture_collector_count_file_name = "fixture_collector_count" + fixture_collector_count_file = session_temp_folder / fixture_collector_count_file_name + fixture_collector_count_file_lock = ( + session_temp_folder / f"{fixture_collector_count_file_name}.lock" + ) + with FileLock(fixture_collector_count_file_lock): + if fixture_collector_count_file.exists(): + with open(fixture_collector_count_file, "r") as f: + fixture_collector_count = int(f.read()) + else: + fixture_collector_count = 0 + fixture_collector_count += 1 + with open(fixture_collector_count_file, "w") as f: + f.write(str(fixture_collector_count)) + fixture_collector = FixtureCollector( output_dir=output_dir, flat_output=request.config.getoption("flat_output"), @@ -634,9 +680,19 @@ def fixture_collector( fixture_collector.dump_fixtures() if do_fixture_verification: fixture_collector.verify_fixture_files(evm_fixture_verification) - generate_fixtures_index( - output_dir, quiet_mode=True, force_flag=True, disable_infer_format=False - ) + + fixture_collector_count = 0 + if session_temp_folder is not None: + with FileLock(fixture_collector_count_file_lock): + with open(fixture_collector_count_file, "r") as f: + fixture_collector_count = int(f.read()) + fixture_collector_count -= 1 + with open(fixture_collector_count_file, "w") as f: + f.write(str(fixture_collector_count)) + if generate_index and fixture_collector_count == 0: + generate_fixtures_index( + output_dir, quiet_mode=True, force_flag=True, disable_infer_format=False + ) @pytest.fixture(autouse=True, scope="session") diff --git a/src/pytest_plugins/filler/tests/test_filler.py b/src/pytest_plugins/filler/tests/test_filler.py index fd4f19e3db4..6a3d47f04c5 100644 --- a/src/pytest_plugins/filler/tests/test_filler.py +++ b/src/pytest_plugins/filler/tests/test_filler.py @@ -77,7 +77,7 @@ def test_shanghai_two(state_test, x): @pytest.mark.parametrize( - "args, expected_fixture_files, expected_fixture_counts", + "args, expected_fixture_files, expected_fixture_counts, expected_index", [ pytest.param( [], @@ -100,10 +100,11 @@ def test_shanghai_two(state_test, x): Path("fixtures/state_tests/shanghai/module_shanghai/shanghai_two.json"), ], [2, 2, 2, 2, 2, 2, 2, 2, 2, 6, 6, 6], + False, id="default-args", ), pytest.param( - ["--build-name", "test_build"], + ["--index", "--build-name", "test_build"], [ Path("fixtures/blockchain_tests/paris/module_paris/paris_one.json"), Path("fixtures/blockchain_tests_engine/paris/module_paris/paris_one.json"), @@ -123,10 +124,11 @@ def test_shanghai_two(state_test, x): Path("fixtures/state_tests/shanghai/module_shanghai/shanghai_two.json"), ], [2, 2, 2, 2, 2, 2, 2, 2, 2, 6, 6, 6], + True, id="build-name-in-fixtures-ini-file", ), pytest.param( - ["--flat-output"], + ["--flat-output", "--index"], [ Path("fixtures/blockchain_tests/paris_one.json"), Path("fixtures/blockchain_tests_engine/paris_one.json"), @@ -142,10 +144,11 @@ def test_shanghai_two(state_test, x): Path("fixtures/state_tests/shanghai_two.json"), ], [2, 2, 2, 2, 2, 2, 2, 2, 2, 6, 6, 6], + True, id="flat-output", ), pytest.param( - ["--flat-output", "--output", "other_fixtures"], + ["--flat-output", "--index", "--output", "other_fixtures"], [ Path("other_fixtures/blockchain_tests/paris_one.json"), Path("other_fixtures/blockchain_tests_engine/paris_one.json"), @@ -161,10 +164,11 @@ def test_shanghai_two(state_test, x): Path("other_fixtures/state_tests/shanghai_two.json"), ], [2, 2, 2, 2, 2, 2, 2, 2, 2, 6, 6, 6], + True, id="flat-output_custom-output-dir", ), pytest.param( - ["--single-fixture-per-file"], + ["--single-fixture-per-file", "--index"], [ Path( "fixtures/blockchain_tests/paris/module_paris/paris_one__fork_Paris_blockchain_test.json" @@ -276,10 +280,11 @@ def test_shanghai_two(state_test, x): ), ], [1] * 36, + True, id="single-fixture-per-file", ), pytest.param( - ["--single-fixture-per-file", "--output", "other_fixtures"], + ["--single-fixture-per-file", "--index", "--output", "other_fixtures"], [ Path( "other_fixtures/blockchain_tests/paris/module_paris/paris_one__fork_Paris_blockchain_test.json" @@ -391,10 +396,11 @@ def test_shanghai_two(state_test, x): ), ], [1] * 36, + True, id="single-fixture-per-file_custom_output_dir", ), pytest.param( - ["--flat-output", "--single-fixture-per-file"], + ["--flat-output", "--index", "--single-fixture-per-file"], [ Path("fixtures/blockchain_tests/paris_one__fork_Paris_blockchain_test.json"), Path("fixtures/state_tests/paris_one__fork_Paris_state_test.json"), @@ -470,12 +476,13 @@ def test_shanghai_two(state_test, x): ), ], [1] * 36, + True, id="flat-single-per-file_flat-output", ), ], ) def test_fixture_output_based_on_command_line_args( - testdir, args, expected_fixture_files, expected_fixture_counts + testdir, args, expected_fixture_files, expected_fixture_counts, expected_index ): """ Test: @@ -555,7 +562,8 @@ def test_fixture_output_based_on_command_line_args( config = configparser.ConfigParser() config.read(ini_file) - assert index_file is not None, f"No {expected_index_file} file was found in {meta_dir}" + if expected_index: + assert index_file is not None, f"No {expected_index_file} file was found in {meta_dir}" properties = {key: value for key, value in config.items("fixtures")} assert "timestamp" in properties