test(davinci-client): add polling e2e tests#634
Conversation
|
📝 WalkthroughWalkthroughThis PR establishes infrastructure and comprehensive e2e tests for polling flows. It refactors error handling in the davinci app to support dynamic error display, adds a new polling tenant configuration, and introduces extensive Playwright test coverage for Challenge and Continue polling scenarios with timeout and expiry handling. ChangesPolling Feature Tests and Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 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 b9b8645
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
e2e/davinci-app/style.css (1)
84-86: ⚡ Quick winThe
.errorclass is defined but not used.The error display in
main.ts(Line 196) uses inline styling (errorDiv.style.color = 'red') instead of applying this CSS class. Consider either using the class or removing it.♻️ Proposed refactor to use the CSS class
In
main.ts, replace the inline style with the class:- if (errorDiv && clientInfo?.status === 'error') { - errorDiv.style.color = 'red'; + if (errorDiv && clientInfo?.status === 'error') { + errorDiv.classList.add('error'); errorDiv.innerHTML = `🤖 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 `@e2e/davinci-app/style.css` around lines 84 - 86, The .error CSS rule is defined but not used; update main.ts to stop using inline styling and instead apply the CSS class by replacing the inline assignment (errorDiv.style.color = 'red') with adding the class (e.g., errorDiv.classList.add('error')) so the .error style is used, or if you prefer to keep inline styles remove the .error rule from style.css; change references to the DOM element named errorDiv in main.ts accordingly.
🤖 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 `@e2e/davinci-app/main.ts`:
- Around line 193-200: The current code always appends a div via
formEl.appendChild(...) and uses innerHTML to render
davinciClient.getError()?.message, causing an unnecessary empty `#error-div` when
no error and an XSS risk; change the logic to only create/append the error
element when clientInfo?.status === 'error' and set its content with textContent
(or equivalent safe API) using davinciClient.getError()?.message to avoid HTML
injection, and remove the unconditional append so no empty div remains when not
in error state.
In `@e2e/davinci-suites/src/polling.test.ts`:
- Line 83: The test's assertion passes a boolean to expect causing it to always
succeed; change the assertion to use a Jest matcher on the actual value (e.g.,
replace the current expect(numPollRequests === maxRetries) with an assertion
like expect(numPollRequests).toBe(maxRetries) or
expect(numPollRequests).toEqual(maxRetries)) so the test actually verifies that
numPollRequests equals maxRetries (refer to the variables numPollRequests and
maxRetries in polling.test.ts).
- Line 122: The assertion currently passes a boolean expression into expect
(expect(numPollRequests === 0)) which evaluates before expect and thus the test
always passes; update the assertion to pass the value and use a matcher—replace
the incorrect call with expect(numPollRequests).toBe(0) (or another appropriate
matcher) so the test actually asserts that numPollRequests equals zero.
---
Nitpick comments:
In `@e2e/davinci-app/style.css`:
- Around line 84-86: The .error CSS rule is defined but not used; update main.ts
to stop using inline styling and instead apply the CSS class by replacing the
inline assignment (errorDiv.style.color = 'red') with adding the class (e.g.,
errorDiv.classList.add('error')) so the .error style is used, or if you prefer
to keep inline styles remove the .error rule from style.css; change references
to the DOM element named errorDiv in main.ts accordingly.
🪄 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: 31270dcd-6665-4f0f-acd5-cfca75a226d8
📒 Files selected for processing (7)
e2e/davinci-app/index.htmle2e/davinci-app/main.tse2e/davinci-app/server-configs.tse2e/davinci-app/style.csse2e/davinci-suites/src/polling.test.tse2e/davinci-suites/src/protect.test.tslefthook.yml
| const errorDiv = document.createElement('div'); | ||
| formEl.appendChild(errorDiv).setAttribute('id', 'error-div'); | ||
| if (errorDiv && clientInfo?.status === 'error') { | ||
| errorDiv.style.color = 'red'; | ||
| errorDiv.innerHTML = ` | ||
| <div>${davinciClient.getError()?.message}</div> | ||
| <p><strong>Error</strong>: ${davinciClient.getError()?.message}</p> | ||
| `; | ||
| } |
There was a problem hiding this comment.
Potential XSS vulnerability and unnecessary DOM element creation.
Two concerns:
-
XSS Risk: The error message is inserted via
innerHTMLwithout sanitization. IfdavinciClient.getError()?.messagecan contain user-controlled input, this creates an XSS vulnerability. -
Empty Element: The
errorDivis always appended to the DOM (Line 194) but only populated whenstatus === 'error'(Line 195). This leaves an emptydiv#error-divin the DOM for non-error states.
🔒 Proposed fix to address both concerns
const error = davinciClient.getError();
- if (error) {
+ if (error && clientInfo?.status === 'error') {
const errorDiv = document.createElement('div');
formEl.appendChild(errorDiv).setAttribute('id', 'error-div');
- if (errorDiv && clientInfo?.status === 'error') {
- errorDiv.style.color = 'red';
- errorDiv.innerHTML = `
- <p><strong>Error</strong>: ${davinciClient.getError()?.message}</p>
- `;
- }
+ errorDiv.style.color = 'red';
+
+ const paragraph = document.createElement('p');
+ const strong = document.createElement('strong');
+ strong.textContent = 'Error';
+ paragraph.appendChild(strong);
+ paragraph.appendChild(document.createTextNode(': ' + (error.message || '')));
+ errorDiv.appendChild(paragraph);
}This fix:
- Only creates the error div when actually needed
- Uses
textContentinstead ofinnerHTMLto prevent XSS
🧰 Tools
🪛 ast-grep (0.42.2)
[warning] 196-198: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: errorDiv.innerHTML = <p><strong>Error</strong>: ${davinciClient.getError()?.message}</p>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 196-198: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: errorDiv.innerHTML = <p><strong>Error</strong>: ${davinciClient.getError()?.message}</p>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🤖 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 `@e2e/davinci-app/main.ts` around lines 193 - 200, The current code always
appends a div via formEl.appendChild(...) and uses innerHTML to render
davinciClient.getError()?.message, causing an unnecessary empty `#error-div` when
no error and an XSS risk; change the logic to only create/append the error
element when clientInfo?.status === 'error' and set its content with textContent
(or equivalent safe API) using davinciClient.getError()?.message to avoid HTML
injection, and remove the unconditional append so no empty div remains when not
in error state.
| }); | ||
|
|
||
| // Check max retry count | ||
| expect(numPollRequests === maxRetries); |
There was a problem hiding this comment.
Incorrect assertion - test will always pass.
The assertion expect(numPollRequests === maxRetries) evaluates to a boolean and passes it to expect(), which will always succeed. This doesn't actually assert the condition.
🐛 Proposed fix
// Check max retry count
- expect(numPollRequests === maxRetries);
+ expect(numPollRequests).toBe(maxRetries);📝 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.
| expect(numPollRequests === maxRetries); | |
| expect(numPollRequests).toBe(maxRetries); |
🤖 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 `@e2e/davinci-suites/src/polling.test.ts` at line 83, The test's assertion
passes a boolean to expect causing it to always succeed; change the assertion to
use a Jest matcher on the actual value (e.g., replace the current
expect(numPollRequests === maxRetries) with an assertion like
expect(numPollRequests).toBe(maxRetries) or
expect(numPollRequests).toEqual(maxRetries)) so the test actually verifies that
numPollRequests equals maxRetries (refer to the variables numPollRequests and
maxRetries in polling.test.ts).
| await expect(page.getByText('Error: expired')).toBeVisible(); | ||
|
|
||
| // Check poll count | ||
| expect(numPollRequests === 0); |
There was a problem hiding this comment.
Incorrect assertion - test will always pass.
Same issue as Line 83: expect(numPollRequests === 0) evaluates the boolean expression first and passes it to expect(), which will always succeed.
🐛 Proposed fix
// Check poll count
- expect(numPollRequests === 0);
+ expect(numPollRequests).toBe(0);📝 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.
| expect(numPollRequests === 0); | |
| expect(numPollRequests).toBe(0); |
🤖 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 `@e2e/davinci-suites/src/polling.test.ts` at line 122, The assertion currently
passes a boolean expression into expect (expect(numPollRequests === 0)) which
evaluates before expect and thus the test always passes; update the assertion to
pass the value and use a matcher—replace the incorrect call with
expect(numPollRequests).toBe(0) (or another appropriate matcher) so the test
actually asserts that numPollRequests equals zero.
@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: |
|
Deployed a2d8ffb to https://ForgeRock.github.io/ping-javascript-sdk/pr-634/a2d8ffb78754d428123b778dd7b8ca01f1988d1a branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-10.0 KB, -100.0%) ➖ No Changes➖ @forgerock/storage - 1.5 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
1 participant
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4929
Description
Core Change: New E2E Polling Test Suite
e2e/davinci-suites/src/polling.test.ts— 5 Playwright tests across two polling scenarios:Challenge Polling (3 tests)
Error: timedOutError: expiredContinue Polling (2 tests)
Message: DonemaxRetries(3), assertsError: timedOutSupporting Changes
e2e/davinci-app/server-configs.tse2e/davinci-app/main.tsclientInfo.status === 'error'; error div styled red inlinee2e/davinci-app/index.html#error-div(now created dynamically)e2e/davinci-app/style.css.error { color: red; }utility classlefthook.ymlformatnow runs afterinterface-mapping)e2e/davinci-suites/src/protect.test.tsstring[]type annotations (TypeScript strictness)Summary by CodeRabbit
New Features
Bug Fixes
Tests