-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Adding oauth test #35417
Adding oauth test #35417
Conversation
038741a
to
981dcc4
Compare
@feanil Please review. |
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.
One small suggestion, otherwise looking good to be merged.
) | ||
return response | ||
|
||
def test_end_points_with_oauth_without_jwt(self): |
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.
We should create a common function to avoid duplicating the code in all test cases.
Example:
Create a function similar to
def run_endpoint_tests(self, expected_status, add_role, use_jwt):
containing all the duplicate code so the actual tests can be changed to something like following:
def test_end_points_with_oauth_without_jwt(self):
self.run_endpoint_tests(expected_status=401, add_role=False, use_jwt=False)
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.
@UsamaSadiq good suggestion, I have implemented.
} | ||
|
||
# endpoints contains all urls with body and role. | ||
self.endpoints = [ |
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.
Why are we only testing these specific endpoints?
Do they belong to a particular feature?
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.
I am converting these endpoints to DRF. So now they all are oauth2 compatible.
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.
Perhaps we can organize this endpoints list in a maintainable way, for example define the endpoints in the constants in next PRs
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
Added OAuth tests for various instructor URLs.
This PR includes authentication of URLs using OAuth2 and testing access to recently converted Django Rest Framework (DRF) endpoints."