test(davinci-client): add polling e2e tests#634
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 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 b8bb92d
☁️ 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
@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 2e97c62 to https://ForgeRock.github.io/ping-javascript-sdk/pr-634/2e97c62c8f5dd1a5109cfc4a138cdbb4191c8e7c 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 |
ancheetah
force-pushed
the
SDKS-4929-polling-e2e
branch
from
b9b8645 to
b8bb92d
Compare
3 participants
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
Chores