Fix/tests using called_once_with #2363
Conversation
| db_value, | ||
| ) | ||
| else: | ||
| mocked_set.assert_not_called() |
There was a problem hiding this comment.
We're mocking the code that involves the set (fetch_todays_email_count()) so the set isn't actually called. That's ok since this code is tested above, so it's cleaner to mock it out in this test (we just can't test the set)
| db_value, | ||
| ) | ||
| else: | ||
| mocked_set.assert_not_called() |
There was a problem hiding this comment.
same comment as above.
| ) | ||
| else: | ||
| mocked_set.assert_not_called() | ||
| with set_config(client.application, "REDIS_ENABLED", True): |
There was a problem hiding this comment.
If we don't set REDIS_ENABLED then increment_todays_email_count() immediately returns None, and it turns out that REDIS_ENABLED is false in CI by default.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -161,7 +161,7 @@ def test_process_sns_results_does_not_process_other_providers(sample_template, m | |||
| ) | |||
|
|
|||
| process_sns_results(response=sns_success_callback(reference="ref1")) is None | |||
| assert mock_logger.called_once_with("") | |||
| assert mock_logger.call_count == 1 | |||
There was a problem hiding this comment.
Instead of using mock_logger.call_count == 1, it would be more precise to use mock_logger.assert_called_once() to ensure that the logger was called exactly once.
tests/app/celery/test_tasks.py
Outdated
| @@ -104,14 +104,14 @@ def test_acknowledge_happy_path(self, mocker): | |||
| acknowledge_sms_normal_mock = mocker.patch("app.sms_normal.acknowledge", return_value=True) | |||
| acknowledge_sms_priority_mock = mocker.patch("app.sms_bulk.acknowledge", return_value=False) | |||
| acknowledge_receipt(SMS_TYPE, NORMAL, receipt) | |||
| assert acknowledge_sms_normal_mock.called_once_with(receipt) | |||
| acknowledge_sms_normal_mock.assert_called_once_with(receipt) | |||
| assert acknowledge_sms_priority_mock.not_called() | |||
There was a problem hiding this comment.
The method assert_not_called() should be used instead of assert acknowledge_sms_priority_mock.not_called() to properly assert that the mock was not called.
| ) | ||
| else: | ||
| mocked_set.assert_not_called() | ||
|
|
||
| @pytest.mark.parametrize("redis_value, db_value, increment_by", [(None, 5, 5), ("3", 5, 3)]) | ||
| def test_increment_todays_requested_email_count(self, mocker, sample_service, redis_value, db_value, increment_by): | ||
| def test_increment_todays_requested_email_count(self, client, mocker, sample_service, redis_value, db_value, increment_by): | ||
| cache_key = email_daily_count_cache_key(sample_service.id) | ||
| mocker.patch("app.redis_store.get", lambda x: redis_value if x == cache_key else None) |
There was a problem hiding this comment.
The mocked_set variable is being used in the test but it is not defined. You should add back the line mocked_set = mocker.patch("app.redis_store.set") to ensure the test works correctly.
| @@ -21,29 +21,22 @@ def test_fetch_todays_requested_sms_count(client, mocker, sample_service, redis_ | |||
|
|
|||
| assert actual_result == expected_result | |||
| if redis_value is None: | |||
| assert mocked_set.called_once_with( | |||
| mocked_set.assert_called_once_with( | |||
There was a problem hiding this comment.
The indentation of the ex=7200 argument should be aligned with the previous arguments for better readability.
| ) | ||
| else: | ||
| mocked_set.assert_not_called() | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("redis_value,db_value,increment_by", [(None, 5, 5), ("3", 5, 3)]) | ||
| def test_increment_todays_requested_sms_count(mocker, sample_service, redis_value, db_value, increment_by): | ||
| def test_increment_todays_requested_sms_count(mocker, client, sample_service, redis_value, db_value, increment_by): | ||
| cache_key = sms_daily_count_cache_key(sample_service.id) | ||
| mocker.patch("app.redis_store.get", lambda x: redis_value if x == cache_key else None) |
There was a problem hiding this comment.
The mocked_set variable is removed but still referenced in the test. Either remove the references to mocked_set or reintroduce the variable.
|
oh dear I found more, we also have use lines like |
Summary | Résumé
There's no such thing as
mock.called_once_with()- pytest thinks this is a new mocked function and so lines such aswill always pass. 😢
So there are a few tests that have been passing despite sometimes actually not testing things correctly.
There is a
mock.assert_called_once_with()that we can use instead though. This PR mostly changes to those.Discovered because Python 3.12 doesn't allow this craziness.
Test instructions | Instructions pour tester la modification
none
Release Instructions | Instructions pour le déploiement
None.
Reviewer checklist | Liste de vérification du réviseur