test(davinci-client): e2e tests for agreements, single checkbox, rich text#638
test(davinci-client): e2e tests for agreements, single checkbox, rich text#638ancheetah wants to merge 2 commits into
Conversation
|
| Name | Type |
|---|---|
| @forgerock/davinci-client | Minor |
| @forgerock/device-client | Minor |
| @forgerock/journey-client | Minor |
| @forgerock/oidc-client | Minor |
| @forgerock/protect | Minor |
| @forgerock/sdk-types | Minor |
| @forgerock/sdk-utilities | Minor |
| @forgerock/iframe-manager | Minor |
| @forgerock/sdk-logger | Minor |
| @forgerock/sdk-oidc | Minor |
| @forgerock/sdk-request-middleware | Minor |
| @forgerock/storage | Minor |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughSingleCheckboxField error moved to top-level ChangesCheckbox Validation Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 fa84047
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We updated the test fixture in node.reducer.test.ts to use the new flattened errorMessage field on SingleCheckboxField, which fixes the assertion failure caused by the PR's intentional type change. The implementation in collector.utils.ts was already reading field.errorMessage directly, but the test was still supplying the old nested validation: { errorMessage: '...' } shape, causing the message to fall back to 'Value cannot be empty'.
Tip
✅ We verified this fix by re-running @forgerock/davinci-client:test.
diff --git a/packages/davinci-client/src/lib/node.reducer.test.ts b/packages/davinci-client/src/lib/node.reducer.test.ts
index 3db2cbd..75d66e7 100644
--- a/packages/davinci-client/src/lib/node.reducer.test.ts
+++ b/packages/davinci-client/src/lib/node.reducer.test.ts
@@ -1699,7 +1699,7 @@ describe('The node collector reducer with ValidatedBooleanCollector', () => {
key: 'accept-terms',
label: 'Accept Terms',
required: true,
- validation: { errorMessage: 'You must accept the terms' },
+ errorMessage: 'You must accept the terms',
},
],
formData: {},
Or Apply changes locally with:
npx nx-cloud apply-locally 2XKE-yVlk
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/boolean.ts`:
- Around line 51-64: The checkbox handler currently only calls updater when
validation passes, causing collector state to lag when validation fails; always
invoke updater((event.target as HTMLInputElement).checked) (the updater call in
this block) regardless of the validation result so the collector state stays in
sync, then run the existing validation logic that creates/removes the
`${collectorKey}-error` element based on result; keep the existing error logging
path (checking 'error' in the updater return) and ensure you do not
remove/create error DOM nodes prematurely (use updater first, then handle
Array.isArray(result) and error DOM updates using result, collectorKey, formEl).
🪄 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: 59f71200-eefc-4560-b9b1-eea5176a5604
📒 Files selected for processing (10)
.gitignoree2e/davinci-app/components/boolean.tse2e/davinci-app/components/text.tse2e/davinci-app/main.tse2e/davinci-app/server-configs.tse2e/davinci-suites/src/form-fields.test.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.ts
ancheetah
force-pushed
the
SDKS-5000-form-fields-e2e
branch
from
128e211 to
89d04ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/davinci-suites/src/form-fields.test.ts (1)
92-94: ⚡ Quick winUse consistent navigation pattern with
asyncEventshelper.The second test uses
page.goto()with a hardcoded URL, while the first test uses theasyncEventshelper with a relative URL. Using the helper provides consistent event handling and avoids hardcoding the port/protocol/host.♻️ Refactor to use asyncEvents helper
test('should render form validation fields', async ({ page }) => { - await page.goto('http://localhost:5829/?clientId=e4ef2896-8d90-4abd-bf0f-7b8034995927'); + const { navigate } = asyncEvents(page); + await navigate('/?clientId=e4ef2896-8d90-4abd-bf0f-7b8034995927'); await expect(page.getByText('Select Form Fields Test Form')).toBeVisible();🤖 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/form-fields.test.ts` around lines 92 - 94, The test "should render form validation fields" currently calls page.goto with a hardcoded full URL; update it to use the existing asyncEvents helper and a relative path like the other test to keep event handling consistent and avoid embedding host/port. Replace the page.goto('http://localhost:5829/?clientId=...') call inside the test body with the asyncEvents call used elsewhere (refer to the asyncEvents helper and the test name "should render form validation fields") so the test navigates to '/?clientId=e4ef2896-8d90-4abd-bf0f-7b8034995927' via asyncEvents instead of a hardcoded URL.
🤖 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/components/boolean.ts`:
- Line 25: The code constructs DOM selectors using the raw collectorKey (const
collectorKey = collector.output.key) and then passes it to
document.querySelector for id/class selectors, which breaks when keys contain
CSS-special characters (e.g., dots); update each querySelector call that uses
collectorKey (the selectors around where collectorKey is interpolated) to wrap
the key with CSS.escape(collectorKey) when building id or class selectors so the
selectors are valid for any key value.
---
Nitpick comments:
In `@e2e/davinci-suites/src/form-fields.test.ts`:
- Around line 92-94: The test "should render form validation fields" currently
calls page.goto with a hardcoded full URL; update it to use the existing
asyncEvents helper and a relative path like the other test to keep event
handling consistent and avoid embedding host/port. Replace the
page.goto('http://localhost:5829/?clientId=...') call inside the test body with
the asyncEvents call used elsewhere (refer to the asyncEvents helper and the
test name "should render form validation fields") so the test navigates to
'/?clientId=e4ef2896-8d90-4abd-bf0f-7b8034995927' via asyncEvents instead of a
hardcoded URL.
🪄 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: c29145f7-cabe-4a6a-b6d0-2690b2d520dc
📒 Files selected for processing (11)
.gitignoree2e/davinci-app/components/boolean.tse2e/davinci-app/components/text.tse2e/davinci-app/main.tse2e/davinci-app/server-configs.tse2e/davinci-suites/src/form-fields.test.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/node.reducer.test.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
ancheetah
force-pushed
the
SDKS-5000-form-fields-e2e
branch
from
89d04ee to
afc0584
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
e2e/davinci-suites/src/form-fields.test.ts (3)
56-56: 💤 Low valueFix typo in comment.
"abscence" should be "absence".
📝 Proposed fix
- // Toggle the single checkbox and assert that it is optional by the abscence of an error message + // Toggle the single checkbox and assert that it is optional by the absence of an error message🤖 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/form-fields.test.ts` at line 56, Fix the typo in the test comment: replace "abscence" with the correct spelling "absence" in the inline comment that reads "Toggle the single checkbox and assert that it is optional by the abscence of an error message" in the form-fields.test.ts file so the comment reads "...by the absence of an error message".
93-93: 💤 Low valueConsider using the
asyncEventshelper consistently.Line 13 uses the
asyncEventshelper'snavigatemethod, but this test usespage.gotodirectly with a hardcoded base URL. For consistency and to ensure event tracking is uniform across tests, consider refactoring:- await page.goto('http://localhost:5829/?clientId=e4ef2896-8d90-4abd-bf0f-7b8034995927'); + const { navigate } = asyncEvents(page); + await navigate('/?clientId=e4ef2896-8d90-4abd-bf0f-7b8034995927');🤖 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/form-fields.test.ts` at line 93, Replace the direct use of page.goto with the asyncEvents helper to keep navigation and event tracking consistent: locate the test that calls page.goto('http://localhost:5829/?clientId=...') and change it to use asyncEvents.navigate(...) (passing the path/query string or using the test's base URL configuration rather than a hardcoded host) and await the call so navigation is tracked the same way as other tests that use asyncEvents.navigate.
110-113: 💤 Low valueConsider adding an assertion that re-checking removes the error message.
The test validates that unchecking the required checkbox displays the error message, but doesn't verify that checking it again removes the error. For more complete coverage:
// Toggle the single checkbox to assert error message await page.locator('`#single-checkbox-field`').check(); await page.locator('`#single-checkbox-field`').uncheck(); await expect(page.getByText('Select the checkbox to continue.')).toBeVisible(); await page.locator('`#single-checkbox-field`').check(); await expect(page.getByText('Select the checkbox to continue.')).not.toBeVisible();🤖 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/form-fields.test.ts` around lines 110 - 113, Add a follow-up assertion that re-checking the required checkbox clears the validation error: after the existing assert that the message "Select the checkbox to continue." is visible, call the same locator ('`#single-checkbox-field`') to check the checkbox again and then assert that page.getByText('Select the checkbox to continue.') is no longer visible; update the test around the existing calls to page.locator('`#single-checkbox-field`').check()/uncheck() and expect(...) to include this removal assertion.
🤖 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.
Nitpick comments:
In `@e2e/davinci-suites/src/form-fields.test.ts`:
- Line 56: Fix the typo in the test comment: replace "abscence" with the correct
spelling "absence" in the inline comment that reads "Toggle the single checkbox
and assert that it is optional by the abscence of an error message" in the
form-fields.test.ts file so the comment reads "...by the absence of an error
message".
- Line 93: Replace the direct use of page.goto with the asyncEvents helper to
keep navigation and event tracking consistent: locate the test that calls
page.goto('http://localhost:5829/?clientId=...') and change it to use
asyncEvents.navigate(...) (passing the path/query string or using the test's
base URL configuration rather than a hardcoded host) and await the call so
navigation is tracked the same way as other tests that use asyncEvents.navigate.
- Around line 110-113: Add a follow-up assertion that re-checking the required
checkbox clears the validation error: after the existing assert that the message
"Select the checkbox to continue." is visible, call the same locator
('`#single-checkbox-field`') to check the checkbox again and then assert that
page.getByText('Select the checkbox to continue.') is no longer visible; update
the test around the existing calls to
page.locator('`#single-checkbox-field`').check()/uncheck() and expect(...) to
include this removal assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5dce5de-914d-475e-ab88-400f177f1c7b
📒 Files selected for processing (11)
.gitignoree2e/davinci-app/components/boolean.tse2e/davinci-app/components/text.tse2e/davinci-app/main.tse2e/davinci-app/server-configs.tse2e/davinci-suites/src/form-fields.test.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/node.reducer.test.ts
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- packages/davinci-client/api-report/davinci-client.api.md
@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 c67e704 to https://ForgeRock.github.io/ping-javascript-sdk/pr-638/c67e7044a79a12e80550f7c062ce02bebe62b0d5 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%) 📊 Minor Changes📈 @forgerock/davinci-client - 54.3 KB (+0.2 KB) ➖ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (18.26%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #638 +/- ##
==========================================
+ Coverage 18.07% 18.26% +0.18%
==========================================
Files 155 155
Lines 24398 24455 +57
Branches 1203 1217 +14
==========================================
+ Hits 4410 4466 +56
- Misses 19988 19989 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/helper.ts`:
- Line 1: The import of CollectorRichContent is being treated as a runtime
symbol; change it to a type-only import and import from the /types sub-path so
TypeScript will erase it at compile time — replace the current import of
CollectorRichContent with a type import (use the type keyword) from
'`@forgerock/davinci-client/types`' to enable proper tree-shaking and follow the
codebase pattern.
🪄 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: 6649dba0-bccd-45d8-a181-6050c46b4acf
📒 Files selected for processing (13)
.changeset/single-checkbox.mde2e/davinci-app/components/boolean.tse2e/davinci-app/components/label.tse2e/davinci-app/helper.tse2e/davinci-suites/src/form-fields.test.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/collector.types.test-d.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.test.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/node.reducer.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/single-checkbox.md
| @@ -1,3 +1,5 @@ | |||
| import { CollectorRichContent } from '@forgerock/davinci-client'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Where CollectorRichContent is declared =="
rg -n "export (interface|type) CollectorRichContent" --type ts
echo
echo "== Current imports in e2e app =="
rg -n "CollectorRichContent|`@forgerock/davinci-client/types`|`@forgerock/davinci-client`" e2e/davinci-app --type ts
echo
echo "== Package root/type entry re-exports (if present) =="
rg -n "collector.types|CollectorRichContent|export .* from" packages/davinci-client --type tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 7562
Add type keyword to import for TypeScript tree-shaking.
Line 1 imports a type as a runtime symbol. Since CollectorRichContent is an interface, use type keyword to ensure TypeScript strips it at compile time and allows proper tree-shaking. The codebase pattern is to import types from the /types sub-path.
🔧 Proposed fix
-import { CollectorRichContent } from '`@forgerock/davinci-client`';
+import type { CollectorRichContent } from '`@forgerock/davinci-client/types`';🤖 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/helper.ts` at line 1, The import of CollectorRichContent is
being treated as a runtime symbol; change it to a type-only import and import
from the /types sub-path so TypeScript will erase it at compile time — replace
the current import of CollectorRichContent with a type import (use the type
keyword) from '`@forgerock/davinci-client/types`' to enable proper tree-shaking
and follow the codebase pattern.
fa84047 to
4d1fbc0
Compare
2 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-5000
Description
SingleCheckboxFieldtype to match the actual server payload.appearance: stringand optionalrichContenttoValidatedBooleanCollector, following theRichTextCollectorpattern.Key Changes
SingleCheckboxFieldtype — flattenedvalidation.errorMessageinto a top-levelerrorMessage?: stringto match the server shape. Updatedcollector.utils.tsand regenerated API reports.booleanComponent— wired inValidator<ValidatedBooleanCollector>so the checkbox runs client-side validation and renders inline errors on change.server-configs.ts— swapped client ID to a PingOne environment that includes the updated form fields flow.form-fields.test.ts— added assertions for rich text link rendering, agreement visibility, single checkbox default state, and required-field error on uncheck.SingleCheckboxFieldextended with the new properties forappearanceand rich text.ValidatedBooleanCollectorrefactored to an interface extendingValidatedSingleValueCollectorWithValue, intersectingoutputwith the new fields. Factory logic updated to populate them vianormalizeReplacements. API reports regenerated.returnValidatedBooleanCollector(5 cases), a reducer-levelrichContentround-trip test, and updated existing assertions to includeappearance.richContentInterpolationextracted as a shared helper.boolean.tsrenders rich text labels via safe DOM node moves and fixes the error display to only callupdateron valid input.Summary by CodeRabbit
New Features
Tests
Chores