chores(src): nuke fixtures dir when html is disabled and no tests were filled

[felix314159]

🗒️ Description

Fixes ethereum/execution-specs#1548 but only under the condition that you add --no-html to your command.
LMK what you think, another alternative would have been to somehow attach to the pytest-html plugin and at the end of that detect whether any tests have been written or not and let it perform the nuke, but that seemed overly complicated.

The general problem is that the html plugin just assumes that it can write to the fixtures dir, so nuking it no matter what would create problems if this PR would not also check whether --no-html had been passed or not.

LMK what you think.

How to reproduce

uv run fill --fork Frontier tests/istanbul/eip1344_chainid/ -s -v --clean --no-html --output=./testoutput

• Before: creates useless testoutput folder with html report etc
• After: No new folder is created

uv run fill --fork Frontier tests/istanbul/eip1344_chainid/ -s -v --clean

• Before & after: creates a bunch of useless files

uv run fill --fork istanbul tests/istanbul/eip1344_chainid/ -s -v --clean

• Before & after: Fills tests as expected

LMK what you think

🔗 Related Issues or PRs

Fixes ethereum/execution-specs#1548

• bug(fill): fill generates a .meta directory even if no tests executed execution-specs#1548

✅ Checklist

• All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:

  uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint

• All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
• All: Considered adding an entry to CHANGELOG.md.
• All: Considered updating the online docs in the ./docs/ directory.
• All: Set appropriate labels for the changes (only maintainers can apply labels).
• Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
• Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
• Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

[marioevz]

Hey, just one comment to make this a bit safer, but in general I think it's a good idea 👍

[marioevz]

I think we should use the FixtureOutput object from src/pytest_plugins/filler/fixture_output.py to determine the appropriate folder to drop here because, while the default is indeed fixtures, it really depends on the --output flag of the command.

[felix314159]

Yes you are right, thanks. I have added it

[danceratopz]

Thanks for tackling this @felix314159!

It's a reasonable approach, but I'd prefer if we could avoid output even without having to specify --no-html. As-is I think this change is of relatively limited use to the average user and wondering if it could be solved entirely.

The general problem is that the html plugin just assumes that it can write to the fixtures dir, so nuking it no matter what would create problems if this PR would not also check whether --no-html had been passed or not.

Are you insisting on --no-html because otherwise pytest-html complains that the directory is no longer there? If so, could you try a hookimpl marker on fill's pytest_sessionfinish() to try and force our hook to run after pytest-html's sessionfinish hook?
https://docs.pytest.org/en/latest/how-to/writing_hook_functions.html#hook-function-ordering-call-example
It might not work, but worth a try.

[felix314159]

Thanks for tackling this @felix314159!

It's a reasonable approach, but I'd prefer if we could avoid output even without having to specify --no-html. As-is I think this change is of relatively limited use to the average user and wondering if it could be solved entirely.

The general problem is that the html plugin just assumes that it can write to the fixtures dir, so nuking it no matter what would create problems if this PR would not also check whether --no-html had been passed or not.

Are you insisting on --no-html because otherwise pytest-html complains that the directory is no longer there? If so, could you try a hookimpl marker on fill's pytest_sessionfinish() to try and force our hook to run after pytest-html's sessionfinish hook? https://docs.pytest.org/en/latest/how-to/writing_hook_functions.html#hook-function-ordering-call-example It might not work, but worth a try.

yes i agree it would be better if it worked no matter whether an html flag passed or not, but i couldn't get it to work that way. i already tried what you linked, adding the trylast thing did not resolve it. but i also am not an expert at pytest hooks. how about this: my experience with html summaries has been that either i don't look at them or that i want to look at them but they are so large in size (>50MB) that they crash my browser when i try to open them. so another option to make everyone happy would be to invert the html flag (the report generation is disabled by default, but you can toggle it on with --enable-html). this way the average user is happy with the ducktape solution this PR is and advanced users know that they can the html flag to get the old behavior back.

[danceratopz]

Thanks for tackling this @felix314159!
It's a reasonable approach, but I'd prefer if we could avoid output even without having to specify --no-html. As-is I think this change is of relatively limited use to the average user and wondering if it could be solved entirely.

The general problem is that the html plugin just assumes that it can write to the fixtures dir, so nuking it no matter what would create problems if this PR would not also check whether --no-html had been passed or not.

Are you insisting on --no-html because otherwise pytest-html complains that the directory is no longer there? If so, could you try a hookimpl marker on fill's pytest_sessionfinish() to try and force our hook to run after pytest-html's sessionfinish hook? https://docs.pytest.org/en/latest/how-to/writing_hook_functions.html#hook-function-ordering-call-example It might not work, but worth a try.

yes i agree it would be better if it worked no matter whether an html flag passed or not, but i couldn't get it to work that way. i already tried what you linked, adding the trylast thing did not resolve it. but i also am not an expert at pytest hooks. how about this: my experience with html summaries has been that either i don't look at them or that i want to look at them but they are so large in size (>50MB) that they crash my browser when i try to open them. so another option to make everyone happy would be to invert the html flag (the report generation is disabled by default, but you can toggle it on with --enable-html). this way the average user is happy with the ducktape solution this PR is and advanced users know that they can the html flag to get the old behavior back.

Yes, I think it's time to remove enabling the html reports by default. In that case, I would not patch the pytest-html plugin's target output option in fill and just let the user define where the html report should go. I.e., use the default pytest-html behavior: --html=report.html. Wdyt?

[danceratopz]

Yes, I think it's time to remove enabling the html reports by default. In that case, I would not patch the pytest-html plugin's target output option in fill and just let the user define where the html report should go. I.e., use the default pytest-html behavior: --html=report.html. Wdyt?

@felix314159 It crossed my mind why using trylast doesn't work (we additionally need a hookwrapper) and came up with this:

• chore(filler): always delete output directory with no tests (without specifying --no-html) felix314159/execution-spec-tests#4

Still open to disabling html output by default instead of the PR above, but I think this PR, as it stands, doesn't fully solve the underlying issue.

[felix314159]

Thanks for looking into this, I am fine with either solution. I will close this now

[danceratopz]

@felix314159 Aha, the PR above was a PR to yours, sorry, didn't mean to hijack this PR entirely. But ok, I pointed the branch from

• chore(filler): always delete output directory with no tests (without specifying --no-html) felix314159/execution-spec-tests#4
  to ethereum/execution-spec-tests main:
• chore(fill): delete output directory if no tests were ran #2037