Replace request mocks with pook in tests #2783
Conversation
openverse-bot
added
the
🚦 status: awaiting triage
| pook.on() | ||
| pook.head(image_url, reply=200, response_headers=headers) | ||
| with pytest.raises(UnsupportedMediaType): | ||
| photon_get(image_url, image.identifier) | ||
|
|
||
| key = f"media:{image.identifier}:thumb_type" | ||
| assert redis.get(key) == expected_cache_val | ||
| pook.off() |
There was a problem hiding this comment.
Please use the pook.on decorator for unit tests, otherwise pook won't get turned off properly after the test runs if any of the assertions or code-paths fail (causing pook to leak into other tests that might not expect it).
Although, IIRC, we can't use the decorator here because we need pook to avoid intercepting the Elasticsearch client's HTTP requests in ImageFactory.create. In that case, you can see some fixtures I wrote to handle this case for a PR that explored other changes: https://github.com/WordPress/openverse/blob/980ef96e0e16fc758f78be9f2aadf595f75948f6/api/test/unit/conftest.py#L144C1-L171
To use those fixtures, add then to conftest.py as they are in the linked commit and then add pook_off to this test and call pook.on when needed.
Alternatively, use pook.use as a context manager around photon_get: https://pook.readthedocs.io/en/latest/examples.html#context-manager-for-isolated-mocking
There was a problem hiding this comment.
Thanks for providing the detailed answer and good call on pook not getting turned off properly in case of failure. I'll make the updates soon :)
| pook.on() | ||
| if webhook: | ||
| mock_post = pook.post(webhook) | ||
| slack._message(text, summary) | ||
| assert mock_post.called == should_alert | ||
| if not should_alert: | ||
| return | ||
| data = mock_post.call_args.kwargs["json"] | ||
| assert data["blocks"][0]["text"]["text"] == text | ||
| assert data["text"] == expected_summary | ||
| if environment: | ||
| assert data["username"].endswith(environment.upper()) | ||
| if should_alert: | ||
| assert mock_post.calls > 0 | ||
| data = json.loads(mock_post.matches[0].body) | ||
| assert data["blocks"][0]["text"]["text"] == text | ||
| assert data["text"] == expected_summary | ||
| if environment: | ||
| assert data["username"].endswith(environment.upper()) | ||
| pook.off() |
There was a problem hiding this comment.
Similar to the other comment, we need pook to turn off safely if the test fails. Either use a context manager, the decorator, or the fixtures I shared.
sarayourfriend
added
🟩 priority: low
|
Based on the low urgency of this PR, the following reviewers are being gently reminded to review this PR: @krysal Excluding weekend1 days, this PR was ready for review 5 day(s) ago. PRs labelled with low urgency are expected to be reviewed within 5 weekday(s)2. @ashiramin, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
|
Drafting to avoid re-pings. @ashiramin please mark the PR ready for review once you've made the requested changes 🙏 |
|
@sarayourfriend I made the changes. Decided to go with context manager. Let me know what you think |
|
Thank you! Approved 👍 I've updated the PR description to prevent the issue from closing. We'll update the issue to be blocked on the related pook issue. |
Fixes
Part of #2256 by @sarayourfriend (cannot close the issue to fully remove requests mocks due to h2non/pook#55, described below).
Description
Replaced request mocks with pook in the following files:
test_slack.py(ingestion server)test_image_proxy.pyHad to add
pookin ingestion_server so also updatedPipfileI couldn't update tests in the following files because
pookdoesn't support binary response. Please see heretest_watermarktest_waveformtest_image_viewsTesting Instructions
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin