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

Improve test reliability by cleaning the state pollution of test_warning in TestMainView #305

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sturmianseq
Copy link

This PR aims to improve test reliability by cleaning the state pollution in TestMainView.test_warning. HEALTH_CHECK['WARNINGS_AS_ERRORS'] is set to TRUE by default, but test_warning changes it to be FALSE. The fix is to restore the default value after running the test.

@@ -116,6 +116,7 @@ def check_status(self):
assert response.status_code == 200, response.content.decode('utf-8')
assert response['content-type'] == 'text/html; charset=utf-8'
assert b'so so' in response.content, response.content
HEALTH_CHECK['WARNINGS_AS_ERRORS'] = True
Copy link
Contributor

Choose a reason for hiding this comment

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

this will evaluate only when all assert statements will pass.
pytest has neat fixture monkeypatch that can be used to override HEALTH_CHECK dictionary just for one test. Just use monkeypatch.setitem to update WARNINGS_AS_ERRORS key:

monkeypatch.setitem(HEALTH_CHECK, 'WARNINGS_AS_ERRORS', False)

Copy link
Contributor

Choose a reason for hiding this comment

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

This project also uses pytest-django that has great fixture - settings that can be also used to modify django-health-check settings:

settings.HEALTH_CHECK['WARNINGS_AS_ERRORS'] = True

And personally, I think it's the best way to write this test because we are already using pytest-django's client fixture.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks a lot for your comments! monkeypatch is a great solution! I strongly agree that your fix is much better than my original one.

By the way, I tried to use settings but it failed in the following way.

tests/test_views.py:113: in test_warning
    settings.HEALTH_CHECK['WARNINGS_AS_ERRORS'] = True
/home/sturmian/.local/lib/python3.8/site-packages/pytest_django/fixtures.py:347: in __getattr__
    return getattr(settings, item)
/home/sturmian/.local/lib/python3.8/site-packages/django/conf/__init__.py:83: in __getattr__
    val = getattr(self._wrapped, name)
E   AttributeError: 'Settings' object has no attribute 'HEALTH_CHECK'

Please let me know if you would like to improve my current fix, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like django-health-check is not initialized in tests/test_app/settings.py, so let's stick to monkeypatch 👍

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.

2 participants