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.

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

Replace request mocks with pook in tests #2783

Merged
merged 5 commits into from
Aug 15, 2023
Merged

Replace request mocks with pook in tests #2783

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
799d947
Replace request with pook in tests
ashiramin Aug 6, 2023
c2feac0
fixed merge conflicts in pipfile.lock
ashiramin Aug 6, 2023
10b24cc
Synced Pipfile.lock
ashiramin Aug 6, 2023
c704660
Use pook context manager to make sure pook turns off properly
ashiramin Aug 15, 2023
df49e5a
Add pook context manager
ashiramin Aug 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions api/test/unit/utils/test_image_proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
from test.factory.models.image import ImageFactory
from unittest import mock
from unittest.mock import MagicMock
from urllib.parse import urlencode

Expand Down Expand Up @@ -443,16 +442,17 @@ def test_photon_get_raises_by_not_allowed_types(image_type):
"headers, expected_cache_val",
[
({"Content-Type": "image/tiff"}, b"tiff"),
(None, b"unknown"),
({"Content-Type": "unknown"}, b"unknown"),
],
)
def test_photon_get_saves_image_type_to_cache(redis, headers, expected_cache_val):
image_url = TEST_IMAGE_URL.replace(".jpg", "")
image = ImageFactory.create(url=image_url)

with mock.patch("requests.head") as mock_head, pytest.raises(UnsupportedMediaType):
mock_head.return_value.headers = headers
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()
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

1 change: 1 addition & 0 deletions ingestion_server/Pipfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pytest = "~=7.4"
pytest-order = "~=1.1"
pytest-sugar = "~=0.9"
remote-pdb = "~=2.1"
pook = "~=1.0"

[packages]
aws-requests-auth = "~=0.4"
Expand Down
Loading