fix(fill): skip fixture index generation if output dir is not clean

[mattgidsy]

🗒️ Description

This PR implements a safeguard around fixture index generation to prevent stale fixture parsing errors when switching between branches with schema-breaking changes.

Specifically:

• At pytest_sessionstart, the fixture output directory is checked for cleanliness (empty or not).
• The state is stored on the pytest.config object.
• At pytest_sessionfinish, the generate_fixtures_index() step is skipped if the output directory was not clean.
• A clear warning is issued in both setup and teardown phases when skipping index generation.

🔗 Related Issues

Fixes #1030

✅ Checklist

• All: Set appropriate labels for the changes.
• All: Considered squashing commits to improve commit history.
• All: Added an entry to CHANGELOG.md.
• All: Considered updating the online docs in the ./docs/ directory.
• Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
• Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
• Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
• Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

[danceratopz]

Hey @mattgidsy thanks a lot for this! It's not quite working with default command-line options yet, I think once you're set up with WSL, then you'll be able to run the unit tests locally and have more confidence in your implementation.

It does work if you add --no-html to fill though, so it's very close! More details below!

[danceratopz]

Suggested change

	session.config._output_dir_was_clean = None
	session.config._output_dir_was_clean = None # type: ignore[attr-defined]

[danceratopz]

Suggested change

	session.config._output_dir_was_clean = False
	session.config._output_dir_was_clean = False # type: ignore[attr-defined]

[danceratopz]

Suggested change

	session.config._output_dir_was_clean = True
	session.config._output_dir_was_clean = True # type: ignore[attr-defined]

[danceratopz]

Matt, the following was detected by debugging the failing unit test (https://github.com/ethereum/execution-spec-tests/actions/runs/14051860205/job/39344901008#step:5:48).

Unfortunately, this doesn't quite work due to some funky, non-obvious interaction with the pytest-html plugin. We hijack the pytest-html command-line args in pytest_configure() to write a test report in html format by default:

execution-spec-tests/src/pytest_plugins/filler/filler.py

Lines 216 to 221 in b4e09b1

	if not config.getoption("disable_html") and config.getoption("htmlpath") is None:
	# generate an html report by default, unless explicitly disabled
	config.option.htmlpath = (
	strip_output_tarball_suffix(config.getoption("output"))
	/ default_html_report_file_path()
	)

It seems to me that this plugin ensures that the target directory (which is output / ".meta") for the generated html file exists at the end of its pytest_configure() hook, which means that the output directory is never empty in any(output_dir.iterdir()):) in our pytest_sessionstart() hook (decorating our pytest_sessionstart() with @pytest.hookimpl(tryfirst=true) which tries to prioritize our plugin hook over other plugins doesn't work).

I think the only way to fix this is to unfortunately move this check up to pytest_configure() hook before we modify the pytest-html hook options (which is admittedly a bit brittle), I quite liked it here in pytest_sessionstart(). We also discussed refactoring the --output flag and related output configs (tarball, clean output dir, etc) by parsing it via a dedicated class that tracks this config/state. Once we get this merged, that could be a next step to make this code more readable and robust.

I confirmed this by running the fill command used in the pytest setup fixture

execution-spec-tests/src/pytest_plugins/consume/tests/test_consume_args.py

Lines 55 to 73 in b4e09b1

	def fill_tests(
	fixtures_dir: Path, fill_fork_from: str, fill_fork_until: str, test_paths: List[Path]
	) -> None:
	"""Run fill to generate test fixtures for use with testing consume."""
	fill_result = CliRunner().invoke(
	fill,
	[
	"-c",
	"pytest.ini",
	"--skip-evm-dump",
	"-m",
	"(not blockchain_test_engine) and (not eip_version_check)",
	f"--from={fill_fork_from}",
	f"--until={fill_fork_until}",
	f"--output={str(fixtures_dir)}",
	*[str(p) for p in test_paths],
	# if we fill many tests, it might help to add -n 8/auto
	],
	)

from the problematic unit test:

fill -c pytest.ini --skip-evm-dump -m "(not blockchain_test_engine) and (not eip_version_check)" --from=Paris --until=Cancun --output=/tmp/pytest-of-dtopz/pytest-13/fixtures0 tests/istanbul/eip1344_chainid/test_chainid.py 

-> dir exists

ls -da /tmp/pytest-of-dtopz/pytest-13/fixtures0/.meta/

Adding the --no-html flag cures the problem:

rm -rf /tmp/pytest-of-dtopz/pytest-13/fixtures0
fill -c pytest.ini --skip-evm-dump -m "(not blockchain_test_engine) and (not eip_version_check)" --from=Paris --until=Cancun --output=/tmp/pytest-of-dtopz/pytest-13/fixtures0 tests/istanbul/eip1344_chainid/test_chainid.py 

-> the index file is generated as expected.

This was a tricky one!

[danceratopz]

I know we discussed a warning here, but after seeing the output, which is quite noisy for little information:

/home/dtopz/code/github/new/execution-spec-tests/.venv/lib/python3.12/site-packages/pluggy/_callers.py:103: UserWarning: [filler] Output directory '<function output_dir at 0x7d7572caaac0>' is not empty at session start. Index generation will be skipped to avoid parsing stale fixtures.
  res = hook_impl.function(*args)

I think the better approach would be to append a line to the pytest session header using this hook:

execution-spec-tests/src/pytest_plugins/filler/filler.py

Line 253 in b4e09b1

def pytest_report_header(config: pytest.Config):

Similar to the warning in the header generated from the forks plugin highlighted here:

[winsvega]

hm. shall we require to clean the output directory unless user provide a flag?

[danceratopz]

hm. shall we require to clean the output directory unless user provide a flag?

Hi @winsvega, please see the reasoning here, happy to discuss options in the meeting:
#1030 (comment)

[danceratopz]

Hi @mattgidsy, thanks for your efforts on this! As we discussed, the team revisited this issue and decided to opt for a safer solution which insists that the specified output directory must either not exist or be completely empty. This ensures that an output directory can never be "polluted" with incompatible test fixtures from multiple runs.

This was nonetheless a good effort, do reach out if you'd like to contribute again in the future!

Closing in favor of

• refactor(fill): encapsulate fixture output options in FixtureOutput class #1471
• fix(fill): insist fill's output directory is empty or doesn't exist #1473

[danceratopz]

hm. shall we require to clean the output directory unless user provide a flag?

Yup, following up from our discussions, we've opted for this solution instead:

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