Skip to content

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

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from
+22 −3

Conversation

felix314159
Copy link
Collaborator

@felix314159 felix314159 commented Aug 8, 2025 •
edited by danceratopz
Loading

🗒️ Description

Fixes #1613 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 #1613

✅ 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
marioevz reviewed Aug 11, 2025
View reviewed changes
Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

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

src/pytest_plugins/filler/filler.py Outdated
# * no tests were filled
html_output_is_enabled = getattr(session.config.option, "htmlpath", None)
if not html_output_is_enabled:
shutil.rmtree("fixtures")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@felix314159 felix314159 Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes you are right, thanks. I have added it

@felix314159 felix314159 force-pushed the fixtures-dir-nuked-when-nothing-was-filled-and-html-disabled branch from c1019a3 to 8190086 Compare August 12, 2025 08:47
@danceratopz
Copy link
Member

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
Copy link
Collaborator Author

felix314159 commented Aug 13, 2025 •
edited
Loading

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
Copy link
Member

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
Copy link
Member

danceratopz commented Aug 14, 2025 •
edited
Loading

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:

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
Copy link
Collaborator Author

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

@danceratopz danceratopz mentioned this pull request Aug 14, 2025
Draft
5 tasks
@danceratopz
Copy link
Member

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marioevz marioevz marioevz left review comments

Assignees
No one assigned
Labels
P-low
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug(fill): fill generates a .meta directory even if no tests executed
3 participants
@felix314159 @danceratopz @marioevz