tests: Ensure authenticate returns newly created user #380
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a new user is created, `authenticate` should return it. Without the change, the test would also pass if `authenticate` returned `None`, which is incorrect behavior.
Codecov Report
@@ Coverage Diff @@
## master #380 +/- ##
=======================================
Coverage 89.60% 89.60%
=======================================
Files 7 7
Lines 481 481
=======================================
Hits 431 431
Misses 50 50 Continue to review full report at Codecov.
|
akatsoulas
reviewed
Oct 12, 2020
| self.assertEqual(User.objects.all().count(), 1) | ||
| user = User.objects.all()[0] | ||
| self.assertEqual(user, User.objects.get()) | ||
| self.assertEqual(user.email, '[email protected]') |
There was a problem hiding this comment.
Thanks @cfra for this PR.
If the authenticate method returned None here the test would fail. This test is about authenticating and creating a new user. If authenticate was failing somewhere in the flow the user creation wouldn't have happened. In that case the assertEqual would fail because the queryset would have returned an empty set.
There was a problem hiding this comment.
The test doesn't necessarily fail if it is just the return value that is omitted:
[nihilus@hermes mozilla-django-oidc]$ TOXENV=py38-django220 tox -- tests.test_auth.OIDCAuthenticationBackendTestCase.test_successful_authentication_new_user
GLOB sdist-make: /home/nihilus/coding/mozilla-django-oidc/setup.py
py38-django220 inst-nodeps: /home/nihilus/coding/mozilla-django-oidc/.tox/.tmp/package/1/mozilla-django-oidc-1.2.4.zip
py38-django220 installed: certifi==2020.6.20,cffi==1.14.3,chardet==3.0.4,cryptography==3.1.1,Django==2.2.16,djangorestframework==3.12.1,idna==2.10,josepy==1.4.0,mock==2.0.0,mozilla-django-oidc==1.2.4,pbr==5.5.0,pycparser==2.20,pyOpenSSL==19.1.0,pytz==2020.1,requests==2.24.0,six==1.15.0,sqlparse==0.4.1,urllib3==1.25.10
py38-django220 run-test-pre: PYTHONHASHSEED='1952038455'
py38-django220 run-test: commands[0] | django-admin.py test tests.test_auth.OIDCAuthenticationBackendTestCase.test_successful_authentication_new_user
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
.
----------------------------------------------------------------------
Ran 1 test in 0.008s
OK
Destroying test database for alias 'default'...
_________________________________________________________________________________________________________________________________________________________________________________ summary __________________________________________________________________________________________________________________________________________________________________________________
py38-django220: commands succeeded
congratulations :)
[nihilus@hermes mozilla-django-oidc]$ git apply
diff --git a/mozilla_django_oidc/auth.py b/mozilla_django_oidc/auth.py
index b211571..a275693 100644
--- a/mozilla_django_oidc/auth.py
+++ b/mozilla_django_oidc/auth.py
@@ -327,7 +327,6 @@ class OIDCAuthenticationBackend(ModelBackend):
raise SuspiciousOperation(msg)
elif self.get_settings('OIDC_CREATE_USER', True):
user = self.create_user(user_info)
- return user
else:
LOGGER.debug('Login failed: No user with %s found, and '
'OIDC_CREATE_USER is False',
[nihilus@hermes mozilla-django-oidc]$ TOXENV=py38-django220 tox -- tests.test_auth.OIDCAuthenticationBackendTestCase.test_successful_authentication_new_user
GLOB sdist-make: /home/nihilus/coding/mozilla-django-oidc/setup.py
py38-django220 inst-nodeps: /home/nihilus/coding/mozilla-django-oidc/.tox/.tmp/package/1/mozilla-django-oidc-1.2.4.zip
py38-django220 installed: certifi==2020.6.20,cffi==1.14.3,chardet==3.0.4,cryptography==3.1.1,Django==2.2.16,djangorestframework==3.12.1,idna==2.10,josepy==1.4.0,mock==2.0.0,mozilla-django-oidc==1.2.4,pbr==5.5.0,pycparser==2.20,pyOpenSSL==19.1.0,pytz==2020.1,requests==2.24.0,six==1.15.0,sqlparse==0.4.1,urllib3==1.25.10
py38-django220 run-test-pre: PYTHONHASHSEED='2719431438'
py38-django220 run-test: commands[0] | django-admin.py test tests.test_auth.OIDCAuthenticationBackendTestCase.test_successful_authentication_newRevert "tests: Ensure authenticate returns newly created user"
_user
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
F
======================================================================
FAIL: test_successful_authentication_new_user (tests.test_auth.OIDCAuthenticationBackendTestCase)
Test successful authentication and user creation.
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/nihilus/coding/mozilla-django-oidc/.tox/py38-django220/lib/python3.8/site-packages/mock/mock.py", line 1305, in patched
return func(*args, **keywargs)
File "/home/nihilus/coding/mozilla-django-oidc/tests/test_auth.py", line 452, in test_successful_authentication_new_user
self.assertEqual(user, User.objects.get())
AssertionError: None != <User: username_algo>
----------------------------------------------------------------------
Ran 1 test in 0.009s
FAILED (failures=1)
Destroying test database for alias 'default'...
ERROR: InvocationError for command /home/nihilus/coding/mozilla-django-oidc/.tox/py38-django220/bin/django-admin.py test tests.test_auth.OIDCAuthenticationBackendTestCase.test_successful_authentication_new_user (exited with code 1)
_________________________________________________________________________________________________________________________________________________________________________________ summary __________________________________________________________________________________________________________________________________________________________________________________
ERROR: py38-django220: commands failed
[nihilus@hermes mozilla-django-oidc]$ git revert 45d546cbc26a565d7f8fae58667c21ea421610dc
Auto-merging tests/test_auth.py
[fix/test-backend-authenticate 45ec08d] Revert "tests: Ensure authenticate returns newly created user"
1 file changed, 2 insertions(+), 2 deletions(-)
[nihilus@hermes mozilla-django-oidc]$ TOXENV=py38-django220 tox -- tests.test_auth.OIDCAuthenticationBackendTestCase.test_successful_authentication_new_user
GLOB sdist-make: /home/nihilus/coding/mozilla-django-oidc/setup.py
py38-django220 inst-nodeps: /home/nihilus/coding/mozilla-django-oidc/.tox/.tmp/package/1/mozilla-django-oidc-1.2.4.zip
py38-django220 installed: certifi==2020.6.20,cffi==1.14.3,chardet==3.0.4,cryptography==3.1.1,Django==2.2.16,djangorestframework==3.12.1,idna==2.10,josepy==1.4.0,mock==2.0.0,mozilla-django-oidc==1.2.4,pbr==5.5.0,pycparser==2.20,pyOpenSSL==19.1.0,pytz==2020.1,requests==2.24.0,six==1.15.0,sqlparse==0.4.1,urllib3==1.25.10
py38-django220 run-test-pre: PYTHONHASHSEED='2072510053'
py38-django220 run-test: commands[0] | django-admin.py test tests.test_auth.OIDCAuthenticationBackendTestCase.test_successful_authentication_new_user
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
.
----------------------------------------------------------------------
Ran 1 test in 0.008s
OK
Destroying test database for alias 'default'...
_________________________________________________________________________________________________________________________________________________________________________________ summary __________________________________________________________________________________________________________________________________________________________________________________
py38-django220: commands succeeded
congratulations :)However, I have to admit that this error is also caught by other tests.
If you want to argue that verification of the return value is out of scope for this particular test, and should only be validated elsewhere, feel free to close this PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a new user is created,
authenticateshould return it.Without the change, the test would also pass if
authenticatereturnedNone, which is incorrect behavior.