Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions src/pytest_plugins/filler/filler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,26 @@ def pytest_configure(config):
config.stash[metadata_key]["Command-line args"] = f"<code>{command_line_args}</code>"


def pytest_sessionstart(session: pytest.Session):
"""Check if the output directory was clean at session start."""
output: Path = session.config.getoption("output")
output_dir = strip_output_tarball_suffix(output)

if is_output_stdout(output) or session.config.option.collectonly:
session.config._output_dir_was_clean = None
Copy link
Member

@danceratopz danceratopz Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return

if output_dir.exists() and any(output_dir.iterdir()):
Copy link
Member

@danceratopz danceratopz Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

session.config._output_dir_was_clean = False
Copy link
Member

@danceratopz danceratopz Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

warnings.warn(
Copy link
Member

@danceratopz danceratopz Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
image

f"[filler] Output directory '{output_dir}' is not empty at session start. "
f"Index generation will be skipped to avoid parsing stale fixtures.",
stacklevel=2,
)
else:
session.config._output_dir_was_clean = True
Copy link
Member

@danceratopz danceratopz Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



@pytest.hookimpl(trylast=True)
def pytest_report_header(config: pytest.Config):
"""Add lines to pytest's console output header."""
Expand Down Expand Up @@ -817,11 +837,19 @@ def pytest_sessionfinish(session: pytest.Session, exitstatus: int):
for file in output_dir.rglob("*.lock"):
file.unlink()

# Generate index file for all produced fixtures.
# Skip index generation if output dir was not clean at session start to avoid stale fixture
if session.config.getoption("generate_index"):
generate_fixtures_index(
output_dir, quiet_mode=True, force_flag=False, disable_infer_format=False
)
was_clean = getattr(session.config, "_output_dir_was_clean", True)
if was_clean is True:
generate_fixtures_index(
output_dir, quiet_mode=True, force_flag=False, disable_infer_format=False
)
elif was_clean is False:
warnings.warn(
f"[filler] Skipping index file generation because the output directory "
f"'{output_dir}' was not clean at session start.",
stacklevel=2,
)

# Create tarball of the output directory if the output is a tarball.
is_output_tarball = output.suffix == ".gz" and output.with_suffix("").suffix == ".tar"
Expand Down
Loading