-
Notifications
You must be signed in to change notification settings - Fork 196
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 test for failed grant token request #218
Conversation
Codecov Report@@ Coverage Diff @@
## master #218 +/- ##
==========================================
+ Coverage 75.11% 75.33% +0.21%
==========================================
Files 114 114
Lines 5691 5729 +38
Branches 930 937 +7
==========================================
+ Hits 4275 4316 +41
+ Misses 1416 1413 -3
Continue to review full report at Codecov.
|
src/sidebar/test/oauth-auth-test.js
Outdated
@@ -53,6 +53,18 @@ describe('oauth auth', function () { | |||
}); | |||
}); | |||
|
|||
it('should raise if a grant token request fails', function () { |
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.
You mean access token request, right?
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.
Sigh, I don't know! You post the grant token to the endpoint in order to get an access token back. Is this a grant token request or an access token request?
I suppose your are requesting an access token as in that's the thing that you're asking for in response.
But that is also true of refresh token requests, so would you call those access token requests as well?
With the way I've done it, if you call the request after the token you're sending, then you have grant token requests and refresh token requests (no ambiguity).
I'm not sure how the spec names these though...
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.
Hmm, looks like the spec calls them "access token requests" and "refresh requests" :)
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 suppose your are requesting an access token as in that's the thing that you're asking for in response.
Yes, exactly.
But that is also true of refresh token requests, so would you call those access token requests as well?
Yes, but the test might be worded as "access token refresh" rather than "access token request" to differentiate the two.
The other option would be to describe this test as a grant token exchange, since that is what the client provides in return for the access token.
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.
Hmm, looks like the spec calls them "access token requests" and "refresh requests" :)
Seems clear enough to me.
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.
Ok, renamed it
8c813b4
to
4087de3
Compare
LGTM. |
No description provided.