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

Add OIDCClient for django tests #318

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kerrermanisNL
Copy link
Contributor

Add an OIDCClient for django tests. This can be useful when you need to use the oidc_id_token in your views. The default Django test client does not provide this. By making use of this new client your requests will contain a oidc_id_token key with a random value (which you can specify). Because there is no easy way to do this yourself for the Django test client (you can either fix it by writing a custom client or by using a RequestFactory, both are not very ideal) I thought it would be useful to provide a default client that comes with the mozilla library.

Add an OIDCClient for django tests. This can be useful when you need to use the oidc_id_token in your views.
The default Django test client does not provide this. By making use of this client your requests will contain
a `oidc_id_token` key with a random value (which you can specify). Because there is no easy way to do
this yourself for the Django test client (you can either fix it by writing a custom client or by using
a `RequestFactory`, both are not very ideal) I thought it would be useful to provide a default client
that comes with the mozilla library.

def test_sets_specified_id_token_for_session(self):
class StubOIDCClient(OIDCClient):
oidc_id_token = 'some_other_oidc_token'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I tested it like this because the session property of the django test client is a little weird to wrap your head around. So instead of instantiating and specifying a new oidc token and refreshing the session, I thought a test like this would be better.

@codecov-io
Copy link

Codecov Report

Merging #318 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #318     +/-   ##
=========================================
+ Coverage   88.36%   88.57%   +0.2%     
=========================================
  Files           7        8      +1     
  Lines         447      455      +8     
=========================================
+ Hits          395      403      +8     
  Misses         52       52
Impacted Files Coverage Δ
mozilla_django_oidc/test.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb3d4fb...c14f1ad. Read the comment docs.

@kerrermanisNL
Copy link
Contributor Author

Usage when specifying this class remains the same for the test client, you just have to specify it in the test case:

class TestSomething(TestCase):
    client_class = OIDCClient

    def test_calls_something_with_oidc_token(self):
        # Functionality remains exactly the same, except the view does not die when 
        # accessing `request.session['oidc_id_token']`
        self.client.get('/someting/')

@kerrermanisNL
Copy link
Contributor Author

Hi there, could someone have a look at this?

@kerrermanisNL
Copy link
Contributor Author

Could someone have a look at this?

@kerrermanisNL
Copy link
Contributor Author

Bump :)

from mozilla_django_oidc.test import OIDCClient


class TestOIDCClient(SimpleTestCase):

Choose a reason for hiding this comment

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

I think deriving from TestCase may work better since none of the things added by SimpleTestCase seem needed here — are they?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to make use of unittest.Testcase. I assume that's what you mean, since the Django TestCase class inherits from SimpleTestCase :)

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's what I meant :) django.TestCase inherits from django.SimpleTestCase which inherits from unittest.TestCase 😄 Anyhoo, no worries, thanks for the feedback :)

@gene1wood
Copy link
Member

gene1wood commented Feb 25, 2020

@kerrermanisNL I've removed the set of reviewers you requested review from as that group are the org owners. They don't have context on this mozilla-django-oidc repo and don't perform PR reviews in it. I'd recommend reaching out to contributors if you're looking for review.

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.

None yet

4 participants