-
Notifications
You must be signed in to change notification settings - Fork 364
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
[SDK-1649] Fix issue where cache was missed when scope parameter was provided #461
Conversation
__tests__/Auth0Client.test.ts
Outdated
access_token = await auth0.getTokenSilently({ | ||
audience: 'foo', | ||
scope: 'bar' | ||
}); | ||
expect(access_token).toEqual('my_access_token'); | ||
expect(utils.runIframe).toHaveBeenCalledTimes(1); |
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 wonder if it would better here to reset the utils.runIframe
mock for this second call, then use .not.toHaveBeenCalled
for the assertion.
The way the test reads right now at first glance is that we're expecting runIframe
to be called once after each call, whereas we're actually only expecting it to be called once in total.
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.
Yeah 👍 good idea
@@ -512,4 +512,33 @@ describe('Auth0Client', () => { | |||
value: originalUserAgent | |||
}); | |||
}); | |||
|
|||
it('uses the cache for multiple token requests with audience and scope', async () => { |
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.
Just a general question; do we want to migrate tests out of index.test.ts
into this file? I'm conscious that we're starting to develop a set of tests across two places for the same functions.
Perhaps we should dedicate some effort to unifying the tests somewhere to make them more discoverable.
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.
Agreed
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.
Looks good, just had a couple of questions.
Description
Multiple calls to
getTokenSilently
with thescope
argument (eggetTokenSilently({ audience: 'https://api/tv-shows', scope: 'read:shows' })
) will miss the cache because the scope argument is not being applied to the SDKs default scopes correctlyTesting
Subsequent calls to
getTokenSilently
withaudience
andscope
should not use the networkChecklist
master