fix(plugins/filler): Index generation concurrency issue (ethereum#725)

* fix(plugins/filler): Index generation concurrency issue

* changelog

* fix(plugins/filler): tests

* fix(plugins/filler): fix parameter group

Author: marioevz

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
 

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)

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")

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