feat(fill): improve performance when xdist is enabled (#1118)

Co-authored-by: danceratopz <[email protected]>

Author: marioevz

docs/CHANGELOG.md

@@ -83,6 +83,7 @@ Release tarball changes:
 - ✨ Modify `valid_at_transition_to` marker to add keyword arguments `subsequent_transitions` and `until` to fill a test using multiple transition forks ([#1081](https://github.com/ethereum/execution-spec-tests/pull/1081)).
 - 🐞 fix(consume): use `"HIVE_CHECK_LIVE_PORT"` to signal hive to wait for port 8551 (Engine API port) instead of the 8545 port when running `consume engine` ([#1095](https://github.com/ethereum/execution-spec-tests/pull/1095)).
 - ✨ `state_test`, `blockchain_test` and `blockchain_test_engine` fixtures now contain the `blobSchedule` from [EIP-7840](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-7840.md), only for tests filled for Cancun and Prague forks ([#1040](https://github.com/ethereum/execution-spec-tests/pull/1040)).
+- 🔀 Change `--dist` flag to the default value, `load`, for better parallelism handling during test filling ([#1118](https://github.com/ethereum/execution-spec-tests/pull/1118))
 
 ### 🔧 EVM Tools
 

docs/gen_test_case_reference.py

@@ -32,6 +32,7 @@
     "--gen-docs",
     f"--gen-docs-target-fork={TARGET_FORK}",
     f"--until={GENERATE_UNTIL_FORK}",
+    "--skip-index",
     "-m",
     "(not blockchain_test_engine) and (not eip_version_check)",
     "-s",

pytest.ini

@@ -18,7 +18,6 @@ addopts =
     -p pytest_plugins.help.help
     -m "not eip_version_check"
     --tb short
-    --dist loadscope
     --ignore tests/cancun/eip4844_blobs/point_evaluation_vectors/
 # these customizations require the pytest-custom-report plugin
 report_passed_verbose = FILLED

src/cli/pytest_commands/fill.py

@@ -44,9 +44,7 @@ def handle_fill_command_flags(fill_args: List[str]) -> List[str]:
 def fill(pytest_args: List[str], **kwargs) -> None:
     """Entry point for the fill command."""
     result = pytest.main(
-        handle_fill_command_flags(
-            ["--index", *pytest_args],
-        ),
+        handle_fill_command_flags(list(pytest_args)),
     )
     sys.exit(result)
 
@@ -59,9 +57,7 @@ def fill(pytest_args: List[str], **kwargs) -> None:
 @common_click_options
 def phil(pytest_args: List[str], **kwargs) -> None:
     """Friendly alias for the fill command."""
-    args = handle_fill_command_flags(
-        ["--index", *pytest_args],
-    )
+    args = handle_fill_command_flags(list(pytest_args))
     result = pytest.main(
         args
         + [

src/ethereum_test_fixtures/file.py

@@ -4,6 +4,7 @@
 from pathlib import Path
 from typing import Annotated, Any, Dict, Optional
 
+from filelock import FileLock
 from pydantic import Discriminator, Tag
 
 from ethereum_test_base_types import EthereumTestRootModel
@@ -65,10 +66,16 @@ def collect_into_file(self, file_path: Path):
         add the hash to the info field on per-fixture basis.
         """
         json_fixtures: Dict[str, Dict[str, Any]] = {}
-        for name, fixture in self.items():
-            json_fixtures[name] = fixture.json_dict_with_info()
-        with open(file_path, "w") as f:
-            json.dump(json_fixtures, f, indent=4)
+        lock_file_path = file_path.with_suffix(".lock")
+        with FileLock(lock_file_path):
+            if file_path.exists():
+                with open(file_path, "r") as f:
+                    json_fixtures = json.load(f)
+            for name, fixture in self.items():
+                json_fixtures[name] = fixture.json_dict_with_info()
+
+            with open(file_path, "w") as f:
+                json.dump(dict(sorted(json_fixtures.items())), f, indent=4)
 
     @classmethod
     def from_file(

src/pytest_plugins/filler/filler.py

@@ -15,8 +15,8 @@
 from typing import Any, Dict, Generator, List, Type
 
 import pytest
+import xdist
 from _pytest.terminal import TerminalReporter
-from filelock import FileLock
 from pytest_metadata.plugin import metadata_key  # type: ignore
 
 from cli.gen_index import generate_fixtures_index
@@ -164,11 +164,11 @@ def pytest_addoption(parser: pytest.Parser):
         help="Specify a build name for the fixtures.ini file, e.g., 'stable'.",
     )
     test_group.addoption(
-        "--index",
-        action="store_true",
+        "--skip-index",
+        action="store_false",
         dest="generate_index",
-        default=False,
-        help="Generate an index file for all produced fixtures.",
+        default=True,
+        help="Skip generating an index file for all produced fixtures.",
     )
 
     debug_group = parser.getgroup("debug", "Arguments defining debug behavior")
@@ -532,27 +532,6 @@ def create_properties_file(
         config.write(f)
 
 
-@pytest.fixture(scope="session", autouse=True)
-def create_tarball(
-    request: pytest.FixtureRequest, output_dir: Path, is_output_tarball: bool
-) -> Generator[None, None, None]:
-    """
-    Create a tarball of json files the output directory if the configured
-    output ends with '.tar.gz'.
-
-    Only include .json and .ini files in the archive.
-    """
-    yield
-    if is_output_tarball:
-        source_dir = output_dir
-        tarball_filename = request.config.getoption("output")
-        with tarfile.open(tarball_filename, "w:gz") as tar:
-            for file in source_dir.rglob("*"):
-                if file.suffix in {".json", ".ini"}:
-                    arcname = Path("fixtures") / file.relative_to(source_dir)
-                    tar.add(file, arcname=arcname)
-
-
 @pytest.fixture(scope="function")
 def dump_dir_parameter_level(
     request: pytest.FixtureRequest, base_dump_dir: Path | None, filler_path: Path
@@ -590,11 +569,6 @@ def get_fixture_collection_scope(fixture_name, config):
     return "module"
 
 
-@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,
@@ -603,29 +577,11 @@ 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]:
     """
     Return 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"),
@@ -638,19 +594,6 @@ def fixture_collector(
     if do_fixture_verification:
         fixture_collector.verify_fixture_files(evm_fixture_verification)
 
-    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=False, disable_infer_format=False
-        )
-
 
 @pytest.fixture(autouse=True, scope="session")
 def filler_path(request: pytest.FixtureRequest) -> Path:
@@ -809,3 +752,41 @@ def pytest_collection_modifyitems(config: pytest.Config, items: List[pytest.Item
                     item.add_marker(mark)
         if "yul" in item.fixturenames:  # type: ignore
             item.add_marker(pytest.mark.yul_test)
+
+
+def pytest_sessionfinish(session: pytest.Session, exitstatus: int):
+    """
+    Perform session finish tasks.
+
+    - Remove any lock files that may have been created.
+    - Generate index file for all produced fixtures.
+    - Create tarball of the output directory if the output is a tarball.
+    """
+    if xdist.is_xdist_worker(session):
+        return
+
+    output: Path = session.config.getoption("output")
+    if is_output_stdout(output):
+        return
+
+    output_dir = strip_output_tarball_suffix(output)
+    # Remove any lock files that may have been created.
+    for file in output_dir.rglob("*.lock"):
+        file.unlink()
+
+    # Generate index file for all produced fixtures.
+    if session.config.getoption("generate_index"):
+        generate_fixtures_index(
+            output_dir, quiet_mode=True, force_flag=False, disable_infer_format=False
+        )
+
+    # Create tarball of the output directory if the output is a tarball.
+    is_output_tarball = output.suffix == ".gz" and output.with_suffix("").suffix == ".tar"
+    if is_output_tarball:
+        source_dir = output_dir
+        tarball_filename = output
+        with tarfile.open(tarball_filename, "w:gz") as tar:
+            for file in source_dir.rglob("*"):
+                if file.suffix in {".json", ".ini"}:
+                    arcname = Path("fixtures") / file.relative_to(source_dir)
+                    tar.add(file, arcname=arcname)

src/pytest_plugins/filler/tests/test_filler.py

@@ -79,7 +79,7 @@ def test_shanghai_two(state_test, x):
 
 @pytest.mark.run_in_serial
 @pytest.mark.parametrize(
-    "args, expected_fixture_files, expected_fixture_counts, expected_index",
+    "args, expected_fixture_files, expected_fixture_counts",
     [
         pytest.param(
             [],
@@ -102,11 +102,33 @@ 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(
-            ["--index", "--build-name", "test_build"],
+            ["--skip-index"],
+            [
+                Path("fixtures/blockchain_tests/paris/module_paris/paris_one.json"),
+                Path("fixtures/blockchain_tests_engine/paris/module_paris/paris_one.json"),
+                Path("fixtures/state_tests/paris/module_paris/paris_one.json"),
+                Path("fixtures/blockchain_tests/paris/module_paris/paris_two.json"),
+                Path("fixtures/blockchain_tests_engine/paris/module_paris/paris_two.json"),
+                Path("fixtures/state_tests/paris/module_paris/paris_two.json"),
+                Path("fixtures/blockchain_tests/shanghai/module_shanghai/shanghai_one.json"),
+                Path(
+                    "fixtures/blockchain_tests_engine/shanghai/module_shanghai/shanghai_one.json"
+                ),
+                Path("fixtures/state_tests/shanghai/module_shanghai/shanghai_one.json"),
+                Path("fixtures/blockchain_tests/shanghai/module_shanghai/shanghai_two.json"),
+                Path(
+                    "fixtures/blockchain_tests_engine/shanghai/module_shanghai/shanghai_two.json"
+                ),
+                Path("fixtures/state_tests/shanghai/module_shanghai/shanghai_two.json"),
+            ],
+            [2, 2, 2, 2, 2, 2, 2, 2, 2, 6, 6, 6],
+            id="skip-index",
+        ),
+        pytest.param(
+            ["--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"),
@@ -126,11 +148,10 @@ 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", "--index"],
+            ["--flat-output"],
             [
                 Path("fixtures/blockchain_tests/paris_one.json"),
                 Path("fixtures/blockchain_tests_engine/paris_one.json"),
@@ -146,11 +167,10 @@ 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", "--index", "--output", "other_fixtures"],
+            ["--flat-output", "--output", "other_fixtures"],
             [
                 Path("other_fixtures/blockchain_tests/paris_one.json"),
                 Path("other_fixtures/blockchain_tests_engine/paris_one.json"),
@@ -166,11 +186,10 @@ 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", "--index"],
+            ["--single-fixture-per-file"],
             [
                 Path(
                     "fixtures/blockchain_tests/paris/module_paris/paris_one__fork_Paris_blockchain_test.json"
@@ -282,11 +301,10 @@ def test_shanghai_two(state_test, x):
                 ),
             ],
             [1] * 36,
-            True,
             id="single-fixture-per-file",
         ),
         pytest.param(
-            ["--single-fixture-per-file", "--index", "--output", "other_fixtures"],
+            ["--single-fixture-per-file", "--output", "other_fixtures"],
             [
                 Path(
                     "other_fixtures/blockchain_tests/paris/module_paris/paris_one__fork_Paris_blockchain_test.json"
@@ -398,11 +416,10 @@ def test_shanghai_two(state_test, x):
                 ),
             ],
             [1] * 36,
-            True,
             id="single-fixture-per-file_custom_output_dir",
         ),
         pytest.param(
-            ["--flat-output", "--index", "--single-fixture-per-file"],
+            ["--flat-output", "--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"),
@@ -478,13 +495,12 @@ 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, expected_index
+    testdir, args, expected_fixture_files, expected_fixture_counts
 ):
     """
     Test:
@@ -571,7 +587,7 @@ def test_fixture_output_based_on_command_line_args(
     config = configparser.ConfigParser()
     config.read(ini_file)
 
-    if expected_index:
+    if "--skip-index" not in args:
         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")}

stubs/xdist/__init__.pyi

@@ -0,0 +1,3 @@
+from .methods import is_xdist_worker
+
+__all__ = ("is_xdist_worker",)

stubs/xdist/methods.pyi

@@ -0,0 +1,3 @@
+import pytest
+
+def is_xdist_worker(session: pytest.Session) -> bool: ...