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

Add a pytest fixture for pook #134

Merged
merged 2 commits into from
Jul 1, 2024
Merged

Conversation

wimglenn
Copy link
Contributor

@wimglenn wimglenn commented Jun 29, 2024

I've been copy-pasting this simple fixture around in several project's conftest.py files for years now. I've seen the recent pytest-pook plugin on PyPI/sourcehut, but it doesn't actually have what is the most pytest-thonic usage, simply with a fixture and not needing to import the pook module from your test code at all:

def test_something(pook):
    pook.get("https://example.org/").reply(204)
    res = requests.get("https://example.org/")
    assert res.status_code == 204

And I see no reason why this entrypoint couldn't just go into pook directly, rather than an extra package. It needn't create a pytest dependency because the submodule pytest_fixture.py is not imported except by pytest configure.

Let me know what you think!

Copy link
Collaborator

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

Great, thanks! We had similar fixtures in various WordPress/openverse projects, which led me to write that pytest-pook module. One of things I wanted when creating it was for it to also check that mocks were all exhausted. It's an easy additional check that the mocks created in the test are actually used. On Openverse, we ran into a few cases where tests incorrectly passed when they should have failed, and that would have been immediately noticed had the number of remaining matchers been checked. It can be implemented over top this fixture, of course.

I wanted pytest-pook outside of pook itself so I could feel a bit freer to experiment with it, and try some opinionated things. For example, the start_active=False option can also be useful, and potentially more ergonomic than calling enable_network, depending on the use case, but arguably enable_network is a lot more explicit.

For what it's worth, it would be easy to add passing the engine created in the context manager by pytest-pook to it. 🙂

That being said, I don't see any reason why not to include at least this. Can you add a mention of it in the README, @wimglenn, as well as an entry in History under the drafted v2.0.0 release section? Additionally, I think it would be better to use pook's context manager, rather than just pook.on() and pook.off(). Unfortunately pook.off()'s usage of the global _engine results it in persisting the network mode unless it's explicitly reset during test run. The context manager is much better for a fixture because it uses an entirely new engine in isolation from other tests. Let me know what you think, if there's a reason I haven't noticed to prefer on/off for this fixture instead use().

Happy to merge this with that and with documentation added. Thanks again for the contribution 😀

Comment on lines 9 to 11
pook_mod.on()
yield pook_mod
pook_mod.off()
Copy link
Collaborator

@sarayourfriend sarayourfriend Jun 29, 2024

Choose a reason for hiding this comment

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

Suggested change
pook_mod.on()
yield pook_mod
pook_mod.off()
with pook_mod.use() as engine:
yield pook_mod

(Updated to yield pook_mod as per the discussion)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would not work with intended usage, which look like the example included in this PR.

Now it would be something like

AttributeError: 'Engine' object has no attribute 'get'

How do you use the engine? Can the fixture still yield the module instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the fixture still yield the module instead?

Yes, I think that's right, apologies!

@wimglenn
Copy link
Contributor Author

I've added a context manager and a changelog mention of the pytest fixture. This doesn't necessarily have to wait for a v2.0.0 release, it's not breaking anything. Or is the next release going to be v2.0.0 anyway?

I had already added an example in the section https://pook.readthedocs.io/en/latest/examples.html#py-test-integration - what else do you think the readme right need, and where?

@sarayourfriend
Copy link
Collaborator

Or is the next release going to be v2.0.0 anyway?

Yes, I plan to release v2 later this week.

This looks perfect now, thanks very much @wimglenn 🙏

@sarayourfriend sarayourfriend merged commit b8fb877 into h2non:master Jul 1, 2024
8 checks passed
@wimglenn wimglenn deleted the fixture branch July 1, 2024 01:13
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.

None yet

2 participants