fix(fill): insist fill's output directory is empty or doesn't exist (ethereum#1608)

* tests(gentest): don't use default output fixture directory in unit test

* feat(fill): refuse to fill to a non-empty output directory; add clean

* test(fill): add tests for fixture output dir args/`FixtureOutput`

* chore(tox): use a dedicated dir in /tmp and --clean when filling tests

* docs: update changelog

* Apply suggestions from code review

---------

Co-authored-by: Mario Vega <marioevz@gmail.com>

Author: 2 people

docs/CHANGELOG.md

@@ -8,12 +8,20 @@ Test fixtures for use by clients are available for each release on the [Github r
 
 ### 💥 Breaking Change
 
+### 💥 Important Change for `fill` Users
+
+The output behavior of `fill` has changed ([#1608](https://github.com/ethereum/execution-spec-tests/pull/1608)):
+
+- Before: `fill` wrote fixtures into the directory specified by the `--output` flag (default: `fixtures`). This could have many unintended consequences, including unexpected errors if old or invalid fixtures existed in the directory (for details see [#1030](https://github.com/ethereum/execution-spec-tests/issues/1030)).
+- Now: `fill` will exit without filling any tests if the specified directory exists and is not-empty. This may be overridden by adding the `--clean` flag, which will first remove the specified directory.
+
 ### 🛠️ Framework
 
 #### `fill`
 
 - 🔀 Refactor: Encapsulate `fill`'s fixture output options (`--output`, `--flat-output`, `--single-fixture-per-file`) into a `FixtureOutput` class ([#1471](https://github.com/ethereum/execution-spec-tests/pull/1471)).
 - ✨ Don't warn about a "high Transaction gas_limit" for `zkevm` tests ([#1598](https://github.com/ethereum/execution-spec-tests/pull/1598)).
+- 🐞 `fill` no longer writes generated fixtures into an existing, non-empty output directory; it must now be empty or `--clean` must be used to delete it first ([#1608](https://github.com/ethereum/execution-spec-tests/pull/1608)).
 
 #### `consume`
 

src/cli/gentest/tests/test_cli.py

@@ -1,5 +1,7 @@
 """Tests for the gentest CLI command."""
 
+from tempfile import TemporaryDirectory
+
 import pytest
 from click.testing import CliRunner
 
@@ -111,5 +113,13 @@ def get_mock_context(self: StateTestProvider) -> dict:
     assert gentest_result.exit_code == 0
 
     ## Fill ##
-    fill_result = runner.invoke(fill, ["-c", "pytest.ini", "--skip-evm-dump", output_file])
+    args = [
+        "-c",
+        "pytest.ini",
+        "--skip-evm-dump",
+        "--output",
+        TemporaryDirectory().name,
+        output_file,
+    ]
+    fill_result = runner.invoke(fill, args)
     assert fill_result.exit_code == 0, f"Fill command failed:\n{fill_result.output}"

src/pytest_plugins/filler/filler.py

@@ -114,12 +114,19 @@ def pytest_addoption(parser: pytest.Parser):
         type=Path,
         default=Path(default_output_directory()),
         help=(
-            "Directory path to store the generated test fixtures. "
+            "Directory path to store the generated test fixtures. Must be empty if it exists. "
             "If the specified path ends in '.tar.gz', then the specified tarball is additionally "
             "created (the fixtures are still written to the specified path without the '.tar.gz' "
             f"suffix). Can be deleted. Default: '{default_output_directory()}'."
         ),
     )
+    test_group.addoption(
+        "--clean",
+        action="store_true",
+        dest="clean",
+        default=False,
+        help="Clean (remove) the output directory before filling fixtures.",
+    )
     test_group.addoption(
         "--flat-output",
         action="store_true",
@@ -221,6 +228,12 @@ def pytest_configure(config):
     # Initialize fixture output configuration
     config.fixture_output = FixtureOutput.from_config(config)
 
+    try:
+        # Check whether the directory exists and is not empty; if --clean is set, it will delete it
+        config.fixture_output.create_directories(is_master=not hasattr(config, "workerinput"))
+    except ValueError as e:
+        pytest.exit(str(e), returncode=pytest.ExitCode.USAGE_ERROR)
+
     if not config.getoption("disable_html") and config.getoption("htmlpath") is None:
         # generate an html report by default, unless explicitly disabled
         config.option.htmlpath = config.fixture_output.directory / default_html_report_file_path()
@@ -521,8 +534,6 @@ def create_properties_file(request: pytest.FixtureRequest, fixture_output: Fixtu
     if fixture_output.is_stdout:
         return
 
-    fixture_output.create_directories()
-
     fixture_properties = {
         "timestamp": datetime.datetime.now().isoformat(),
     }

src/pytest_plugins/filler/fixture_output.py

@@ -1,5 +1,6 @@
 """Fixture output configuration for generated test fixtures."""
 
+import shutil
 import tarfile
 from pathlib import Path
 
@@ -22,6 +23,10 @@ class FixtureOutput(BaseModel):
             "write each fixture to its own file"
         ),
     )
+    clean: bool = Field(
+        default=False,
+        description="Clean (remove) the output directory before filling fixtures.",
+    )
 
     @property
     def directory(self) -> Path:
@@ -53,11 +58,78 @@ def strip_tarball_suffix(path: Path) -> Path:
             return path.with_suffix("").with_suffix("")
         return path
 
-    def create_directories(self) -> None:
-        """Create output and metadata directories if needed."""
+    def is_directory_empty(self) -> bool:
+        """Check if the output directory is empty."""
+        if not self.directory.exists():
+            return True
+
+        return not any(self.directory.iterdir())
+
+    def get_directory_summary(self) -> str:
+        """Return a summary of directory contents for error reporting."""
+        if not self.directory.exists():
+            return "directory does not exist"
+
+        items = list(self.directory.iterdir())
+        if not items:
+            return "empty directory"
+
+        dirs = [d.name for d in items if d.is_dir()]
+        files = [f.name for f in items if f.is_file()]
+
+        max_dirs = 4
+        summary_parts = []
+        if dirs:
+            summary_parts.append(
+                f"{len(dirs)} directories"
+                + (
+                    f" ({', '.join(dirs[:max_dirs])}"
+                    + (f"... and {len(dirs) - max_dirs} more" if len(dirs) > max_dirs else "")
+                    + ")"
+                    if dirs
+                    else ""
+                )
+            )
+        if files:
+            summary_parts.append(
+                f"{len(files)} files"
+                + (
+                    f" ({', '.join(files[:3])}"
+                    + (f"... and {len(files) - 3} more" if len(files) > 3 else "")
+                    + ")"
+                    if files
+                    else ""
+                )
+            )
+
+        return " and ".join(summary_parts)
+
+    def create_directories(self, is_master: bool) -> None:
+        """
+        Create output and metadata directories if needed.
+
+        If clean flag is set, remove and recreate the directory.
+        Otherwise, verify the directory is empty before proceeding.
+        """
         if self.is_stdout:
             return
 
+        # Only the master process should delete/create directories if using pytest-xdist
+        if not is_master:
+            return
+
+        if self.directory.exists() and self.clean:
+            shutil.rmtree(self.directory)
+
+        if self.directory.exists() and not self.is_directory_empty():
+            summary = self.get_directory_summary()
+            raise ValueError(
+                f"Output directory '{self.directory}' is not empty. "
+                f"Contains: {summary}. Use --clean to remove all existing files "
+                "or specify a different output directory."
+            )
+
+        # Create directories
         self.directory.mkdir(parents=True, exist_ok=True)
         self.metadata_dir.mkdir(parents=True, exist_ok=True)
 
@@ -72,22 +144,12 @@ def create_tarball(self) -> None:
                     arcname = Path("fixtures") / file.relative_to(self.directory)
                     tar.add(file, arcname=arcname)
 
-    @classmethod
-    def from_options(
-        cls, output_path: Path, flat_output: bool, single_fixture_per_file: bool
-    ) -> "FixtureOutput":
-        """Create a FixtureOutput instance from pytest options."""
-        return cls(
-            output_path=output_path,
-            flat_output=flat_output,
-            single_fixture_per_file=single_fixture_per_file,
-        )
-
     @classmethod
     def from_config(cls, config: pytest.Config) -> "FixtureOutput":
         """Create a FixtureOutput instance from pytest configuration."""
         return cls(
             output_path=config.getoption("output"),
             flat_output=config.getoption("flat_output"),
             single_fixture_per_file=config.getoption("single_fixture_per_file"),
+            clean=config.getoption("clean"),
         )