-
Notifications
You must be signed in to change notification settings - Fork 3
SDKS-4053: Add OIDC client tests #368
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
Conversation
🦋 Changeset detectedLatest commit: 5fe1f95 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
View your CI Pipeline Execution ↗ for commit 5fe1f95
☁️ Nx Cloud last updated this comment at |
Deployed ba29fba to https://ForgeRock.github.io/ping-javascript-sdk/pr-368/ba29fbaf97ff4b3a2039026af965a9a3f93e30dd branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis📊 Minor Changes📈 @forgerock/oidc-client - 21.4 KB (+0.3 KB) ➖ No Changes➖ @forgerock/protect - 152.3 KB 11 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
1243ac3
to
cc4af7c
Compare
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.
This looks good. I would like to add some negative cases to ensure we're returning the right errors in common use cases.
cc4af7c
to
d88e202
Compare
|
@cerebrl The only negative test case I can think of that would work with the e2e app is testing the state mismatch error from token exchange by navigating to a page with an invalid code and state. Any others you can think of? I think |
d88e202
to
09b24e1
Compare
0a9d365
to
bc6a1e5
Compare
bc6a1e5
to
0242180
Compare
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.
This looks good, just a few comments
*/ | ||
import { CustomLogger } from '@forgerock/sdk-logger'; | ||
import { GetAuthorizationUrlOptions } from '@forgerock/sdk-oidc'; | ||
import { GetAuthorizationUrlOptions } from '@forgerock/sdk-types'; |
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.
should we have a type
import here?
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 can update this but I don't think we're very consistent with type imports. I'm definitely guilty of this but I'll try to be better.
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.
This is often a side-effect of VS Code auto-importing the types. It does not include the type
keyword. I've been routinely going back and fixing the missing keyword. It's not a huge deal, but it's good to fix them when you come across them.
}); | ||
}); | ||
|
||
it.effect('handleTokenResponseµ with no data fails', () => { |
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.
👍
import { iFrameManager, ResolvedParams } from '@forgerock/iframe-manager'; | ||
|
||
import type { WellKnownResponse } from '@forgerock/sdk-types'; | ||
import type { WellKnownResponse, GetAuthorizationUrlOptions } from '@forgerock/sdk-types'; |
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.
Awesome, thanks!
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 think this is great. I have no criticisms to leave here.
0242180
to
1cb5e73
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #368 +/- ##
==========================================
- Coverage 57.58% 55.47% -2.11%
==========================================
Files 32 32
Lines 2044 2044
Branches 321 340 +19
==========================================
- Hits 1177 1134 -43
- Misses 867 910 +43 🚀 New features to boost your workflow:
|
4b1a820
to
24d6c31
Compare
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.
This is unbelievable good. Approved.
24d6c31
to
5fe1f95
Compare
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4053
Description
Added changeset