Skip to content
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.

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

Use tmp_path in ghostwriter tests #4050

Merged
merged 7 commits into from
Jul 20, 2024

Conversation

pganssle
Copy link
Contributor

Before creating the temporary functions, moves the local directory to a clean temporary directory; this should guarantee that the files to be created don't exist, and handles cleanup automatically.

This also works when the local directory that the tests are executed from is not writable, but the temporary directory is.

I have tested that the way fixture resolution works, it seems that in_temp_file will only be executed once if you include both of the file-creating fixtures.

@pganssle pganssle force-pushed the ghostwriter_temp_dirs branch from b381394 to 2762b7e Compare July 17, 2024 19:40
@pganssle
Copy link
Contributor Author

pganssle commented Jul 17, 2024

Most of the failures seem to be random flakiness (which maybe is not great considering I keep telling people "Oh hypothesis doesn't make your test suite flaky!"), but there's this lint rule that seems... odd:

hypothesis-python/tests/ghostwriter/test_ghostwriter.py:454:5: PT004 Fixture `in_temp_path` does not return anything, add leading underscore
    |
453 | @pytest.fixture
454 | def in_temp_path(tmp_path):
    |     ^^^^^^^^^^^^ PT004
455 |     """Fixture to execute tests in a temporary path."""
456 |     old_path = Path.cwd()
    |

Is that a real thing that you actually want addressed? I have never heard of this convention before. It doesn't seem to be in evidence in the pytest documentation, and it seems misleading, since it seems to imply that the fixture is private or something.

Edit Seems like at least one pytest maintainer (and several other people chiming in) agrees with me that this rule is incorrect.

Before creating the temporary functions, moves the local directory to a
clean temporary directory; this should guarantee that the files to be
created don't exist, and handles cleanup automatically.

This also works when the local directory that the tests are executed
from is not writable, but the temporary directory is.
@pganssle pganssle force-pushed the ghostwriter_temp_dirs branch from 2762b7e to 769f575 Compare July 17, 2024 20:15
Zac-HD added 5 commits July 20, 2024 11:12
As a test-only change, this doesn't need to be released, and we can simply ignore the (bad) lint rule.
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Thanks Paul! I've just disabled that lint rule, and stacked in some other small test fixes that I had in a branch - merging now 🙂

@Zac-HD Zac-HD enabled auto-merge July 20, 2024 18:18
@Zac-HD
Copy link
Member

Zac-HD commented Jul 20, 2024

Most of the failures seem to be random flakiness (which maybe is not great considering I keep telling people "Oh hypothesis doesn't make your test suite flaky!")

Well, most people aren't writing a large number of tests like "the default strategy will always generate $specific_edge_case", or "if you do $thing you get a health-check failure"; we're pushing hard enough to hit a lot of nondeterministic or timing-dependent edge cases, and with the backend evolving (#3921) we haven't finished shaking out all the flakes yet 😞

@Zac-HD Zac-HD merged commit d76d74f into HypothesisWorks:master Jul 20, 2024
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants