-
Notifications
You must be signed in to change notification settings - Fork 12
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
Fix two tests #1994
Fix two tests #1994
Conversation
38784aa
to
ceee6a9
Compare
🧪 Review environmenthttps://v2so6xivu2rh3sb37feonws5o40cvynp.lambda-url.ca-central-1.on.aws/ |
get_page_by_slug_with_cache(endpoint, params) | ||
|
||
assert request_mock.called | ||
|
||
assert mock_redis_method.get.called | ||
assert mock_redis_method.get.call_count == 2 | ||
assert mock_redis_method.get.called_with(cache_key) | ||
assert mock_redis_method.get.call_count == 3 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assert mock_redis_method.get(fallback_cache_key) == json.dumps(response_json)
line should be placed before the assert mock_redis_method.get(fallback_cache_key) is not None
line to ensure that the value is checked for equality first before checking for non-nullity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, I dunno about the order but it's redundant to have both assertions there so I'll remove the non-nullity one and give the ai a 👍 for pointing it out.
get_page_by_slug_with_cache(endpoint, params) | ||
|
||
assert request_mock.called | ||
|
||
assert mock_redis_method.get.called | ||
assert mock_redis_method.get.call_count == 2 | ||
assert mock_redis_method.get.called_with(cache_key) | ||
assert mock_redis_method.get.called_with(fallback_cache_key) | ||
calls = [call(cache_key), call(cache_key)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calls
list should include the fallback_cache_key
as well, since the test is verifying that there is no fallback when a 404 is received. The correct calls should be call(cache_key)
and call(fallback_cache_key)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding and fixing these!
Summary | Résumé
When doing the Python upgrade I found two tests that weren't testing the right things (but passing anyway 😅 ). Figured I'd fix them on their own, the Python PR is complicated enough.
Test instructions | Instructions pour tester la modification
Verify that the tests still pass.