test(iframe-manager): add unit tests with jsdom#625
Conversation
|
|
Warning Review limit reached
More reviews will be available in 48 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a comprehensive test suite for the ChangesgetParamsByRedirect API test suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 1985090 ☁️ Nx Cloud last updated this comment at |
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #625 +/- ##
===========================================
+ Coverage 18.07% 69.13% +51.05%
===========================================
Files 155 22 -133
Lines 24398 1649 -22749
Branches 1203 261 -942
===========================================
- Hits 4410 1140 -3270
+ Misses 19988 509 -19479 🚀 New features to boost your workflow:
|
|
Deployed f33cb45 to https://ForgeRock.github.io/ping-javascript-sdk/pr-625/f33cb45b3b8f17f2131b5ad17b520a178bec47d1 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/device-client - 10.0 KB (new) ➖ No Changes➖ @forgerock/protect - 144.6 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/davinci-client/api-report/davinci-client.api.md`:
- Line 273: The public API renamed the method from poll to pollStatus (seen as
pollStatus: (collector: PollingCollector) => Poller), which is breaking; restore
backwards compatibility by adding a transitional alias named poll that delegates
to pollStatus (e.g., implement a poll method or export a poll symbol that
calls/returns pollStatus(collector)), keep both signatures matching
PollingCollector => Poller, mark the alias as deprecated in source comments, and
then regenerate the API report so the exported contract includes both poll and
pollStatus.
In `@packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.test.ts`:
- Around line 50-61: The test "throws synchronously when successParams or
errorParams is undefined" only exercises the successParams-undefined branch;
update the tests for iFrameManager().getParamsByRedirect to either (a) split
into two assertions or add a second it block: one that passes successParams:
undefined and a second that passes errorParams: undefined (both with other
required fields like url/timeout) and assert they throw 'successParams and
errorParams must be provided'; alternatively rename the existing test to
accurately reflect it only covers successParams if you prefer separate coverage.
Use the manager variable and getParamsByRedirect symbol to locate the test to
modify.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a86d644-55d0-4f24-b007-d93fe6c9959c
📒 Files selected for processing (4)
packages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.test.tspackages/sdk-effects/iframe-manager/vite.config.ts
| update: <T extends SingleValueCollectors | MultiSelectCollector | ObjectValueCollectors | AutoCollectors>(collector: T) => Updater<T>; | ||
| validate: (collector: SingleValueCollectors | ObjectValueCollectors | MultiValueCollectors | AutoCollectors) => Validator; | ||
| poll: (collector: PollingCollector) => Poller; | ||
| pollStatus: (collector: PollingCollector) => Poller; |
There was a problem hiding this comment.
Public API rename is breaking without a compatibility bridge.
Renaming poll to pollStatus changes the exported client contract and will break existing integrations unless you provide a transitional alias (or explicitly gate this behind a major-version release plan).
Suggested compatibility bridge (in source, then regenerate API report)
- pollStatus: (collector: PollingCollector) => Poller;
+ pollStatus: (collector: PollingCollector) => Poller;
+ /** `@deprecated` Use pollStatus instead. */
+ poll: (collector: PollingCollector) => Poller;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/davinci-client/api-report/davinci-client.api.md` at line 273, The
public API renamed the method from poll to pollStatus (seen as pollStatus:
(collector: PollingCollector) => Poller), which is breaking; restore backwards
compatibility by adding a transitional alias named poll that delegates to
pollStatus (e.g., implement a poll method or export a poll symbol that
calls/returns pollStatus(collector)), keep both signatures matching
PollingCollector => Poller, mark the alias as deprecated in source comments, and
then regenerate the API report so the exported contract includes both poll and
pollStatus.
| it('throws synchronously when successParams or errorParams is undefined', () => { | ||
| const manager = iFrameManager(); | ||
| expect(() => | ||
| manager.getParamsByRedirect({ | ||
| url: 'https://example.com', | ||
| timeout: 1000, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| successParams: undefined as any, | ||
| errorParams: ['error'], | ||
| }), | ||
| ).toThrow('successParams and errorParams must be provided'); | ||
| }); |
There was a problem hiding this comment.
Test name is misleading and the errorParams: undefined case is missing.
The description says "successParams or errorParams is undefined" but the test body only exercises successParams: undefined. errorParams: undefined is never passed, so the guard for that branch is untested.
🛠️ Proposed fix: cover both `undefined` cases
- it('throws synchronously when successParams or errorParams is undefined', () => {
- const manager = iFrameManager();
- expect(() =>
- manager.getParamsByRedirect({
- url: 'https://example.com',
- timeout: 1000,
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
- successParams: undefined as any,
- errorParams: ['error'],
- }),
- ).toThrow('successParams and errorParams must be provided');
- });
+ it('throws synchronously when successParams is undefined', () => {
+ const manager = iFrameManager();
+ expect(() =>
+ manager.getParamsByRedirect({
+ url: 'https://example.com',
+ timeout: 1000,
+ // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
+ successParams: undefined as any,
+ errorParams: ['error'],
+ }),
+ ).toThrow('successParams and errorParams must be provided');
+ });
+
+ it('throws synchronously when errorParams is undefined', () => {
+ const manager = iFrameManager();
+ expect(() =>
+ manager.getParamsByRedirect({
+ url: 'https://example.com',
+ timeout: 1000,
+ successParams: ['code'],
+ // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
+ errorParams: undefined as any,
+ }),
+ ).toThrow('successParams and errorParams must be provided');
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('throws synchronously when successParams or errorParams is undefined', () => { | |
| const manager = iFrameManager(); | |
| expect(() => | |
| manager.getParamsByRedirect({ | |
| url: 'https://example.com', | |
| timeout: 1000, | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| successParams: undefined as any, | |
| errorParams: ['error'], | |
| }), | |
| ).toThrow('successParams and errorParams must be provided'); | |
| }); | |
| it('throws synchronously when successParams is undefined', () => { | |
| const manager = iFrameManager(); | |
| expect(() => | |
| manager.getParamsByRedirect({ | |
| url: 'https://example.com', | |
| timeout: 1000, | |
| // eslint-disable-next-line `@typescript-eslint/no-explicit-any` | |
| successParams: undefined as any, | |
| errorParams: ['error'], | |
| }), | |
| ).toThrow('successParams and errorParams must be provided'); | |
| }); | |
| it('throws synchronously when errorParams is undefined', () => { | |
| const manager = iFrameManager(); | |
| expect(() => | |
| manager.getParamsByRedirect({ | |
| url: 'https://example.com', | |
| timeout: 1000, | |
| successParams: ['code'], | |
| // eslint-disable-next-line `@typescript-eslint/no-explicit-any` | |
| errorParams: undefined as any, | |
| }), | |
| ).toThrow('successParams and errorParams must be provided'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.test.ts`
around lines 50 - 61, The test "throws synchronously when successParams or
errorParams is undefined" only exercises the successParams-undefined branch;
update the tests for iFrameManager().getParamsByRedirect to either (a) split
into two assertions or add a second it block: one that passes successParams:
undefined and a second that passes errorParams: undefined (both with other
required fields like url/timeout) and assert they throw 'successParams and
errorParams must be provided'; alternatively rename the existing test to
accurately reflect it only covers successParams if you prefer separate coverage.
Use the manager variable and getParamsByRedirect symbol to locate the test to
modify.
SteinGabriel
left a comment
SteinGabriel
left a comment
There was a problem hiding this comment.
Changes look good. I'd like to point out that there's an empty binary diff for the vite.config.ts file. Probably worth checking if that's something we'd like to address
before merging this.
Looked into this, seems like a git diff thing. the file has a vite config. |
4 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-5028
Description
Adds some tests using jsdom for iframe-manager.
Summary by CodeRabbit