-
Notifications
You must be signed in to change notification settings - Fork 3
Journey client tests #501
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
Journey client tests #501
Conversation
|
WalkthroughAdds WebAuthn and QR-code mock responses and route branches, integrates PingOne Protect initialization/evaluation and async device-profile collection in the journey app, adds the Protect dependency and TypeScript references, and introduces new and updated Playwright e2e tests for QR, recovery-codes, Protect, and device-profile journeys. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JourneyApp as Journey App
participant AM as AM Mock API
participant Callbacks as Callbacks/UI
User->>JourneyApp: Navigate to WebAuthn journey
JourneyApp->>AM: GET registration init
AM-->>JourneyApp: webAuthnRegistrationInit (MetadataCallback, HiddenValueCallback)
JourneyApp->>User: Render registration UI
User->>JourneyApp: Submit registration (mock)
JourneyApp->>AM: POST registration result
AM-->>JourneyApp: getRecoveryCodesDisplay() (TextOutputCallback + ConfirmationCallback)
JourneyApp->>User: Show recovery codes & prompt
User->>JourneyApp: Confirm saved
JourneyApp->>AM: POST confirmation
AM-->>JourneyApp: authSuccess (token)
JourneyApp->>User: Display completion
sequenceDiagram
participant User
participant JourneyApp as Journey App
participant InitComp as Protect Init Component
participant ProtectLib as Protect Library
participant EvalComp as Protect Eval Component
participant AM as AM Mock API
User->>JourneyApp: Start Protect flow
JourneyApp->>AM: GET PingOneProtectInitializeCallback
AM-->>JourneyApp: Initialize callback
JourneyApp->>InitComp: Render init component
InitComp->>ProtectLib: protect.start(envId, config)
ProtectLib-->>InitComp: instance ready
InitComp->>JourneyApp: Auto-submit init result
JourneyApp->>AM: GET Protect evaluation callback
AM-->>JourneyApp: Evaluation callback
JourneyApp->>EvalComp: Render evaluation component
EvalComp->>ProtectLib: getData()
ProtectLib-->>EvalComp: risk signals
EvalComp->>JourneyApp: Auto-submit evaluation
JourneyApp->>AM: Submit evaluation -> Success
sequenceDiagram
participant User
participant JourneyApp as Journey App
participant AM as AM Mock API
User->>JourneyApp: Start QR code journey
JourneyApp->>AM: GET login
AM-->>JourneyApp: qrCodeLoginResponse
User->>JourneyApp: Submit credentials
JourneyApp->>AM: POST login
AM-->>JourneyApp: qrCodeCallbacksResponse (QR display, hidden URI)
JourneyApp->>User: Show QR enrollment UI
User->>JourneyApp: Confirm enrollment
JourneyApp->>AM: POST confirmation
AM-->>JourneyApp: authSuccess (token)
JourneyApp->>User: Display completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 00143a5
☁️ Nx Cloud last updated this comment at |
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.
Actionable comments posted: 2
🧹 Nitpick comments (9)
e2e/am-mock-api/src/app/response.webauthn.js (1)
167-171: Naming conflict withauthSuccessin responses.js.This file exports
authSuccesswithtokenId: 'webauthn-session-token', whileresponses.jsalso exports anauthSuccesswithtokenId: 'bar'. Whileroutes.auth.jscorrectly aliases this aswebAuthnSuccess, the duplicate naming could cause confusion or accidental misuse.Consider renaming this to
webAuthnAuthSuccessfor clarity and to avoid potential import conflicts.-export const authSuccess = { - tokenId: 'webauthn-session-token', +export const webAuthnAuthSuccess = { + tokenId: 'webauthn-session-token', successUrl: '/console', realm: '/', };e2e/am-mock-api/src/app/responses.js (2)
1408-1492: Code duplication withotpQRCodeCallbacks.
qrCodeCallbacksResponse(lines 1408-1492) is nearly identical tootpQRCodeCallbacks(lines 500-584), differing only in theauthId. Consider reusingotpQRCodeCallbacksor extracting a factory function to reduce duplication.// Alternative: Create a factory function export function createQRCodeCallbacksResponse(authId) { return { authId, callbacks: [/* ... shared callback structure ... */], }; } export const otpQRCodeCallbacks = createQRCodeCallbacksResponse('foo'); export const qrCodeCallbacksResponse = createQRCodeCallbacksResponse('qrcode-journey-confirmation');
1351-1406: Consider extracting a factory for login responses.
qrCodeLoginResponse,deviceProfileLoginResponse, andprotectLoginResponseshare identical structure, differing only inauthId. A helper function could reduce duplication.function createLoginResponse(authId) { return { authId, callbacks: [ { type: 'NameCallback', output: [{ name: 'prompt', value: 'User Name' }], input: [{ name: 'IDToken1', value: '' }], _id: 0, }, { type: 'PasswordCallback', output: [{ name: 'prompt', value: 'Password' }], input: [{ name: 'IDToken2', value: '' }], _id: 1, }, ], stage: 'UsernamePassword', }; } export const qrCodeLoginResponse = createLoginResponse('qrcode-journey-login'); export const deviceProfileLoginResponse = createLoginResponse('device-profile-journey-login'); export const protectLoginResponse = createLoginResponse('protect-journey-login');e2e/journey-suites/src/qr-code.test.ts (1)
18-21: UnnecessaryPromise.resolve(true)return.The console event handler doesn't need to return a value. The
page.on('console', ...)callback is fire-and-forget.page.on('console', async (msg) => { messageArray.push(msg.text()); - return Promise.resolve(true); });e2e/journey-app/components/ping-protect-evaluation.ts (2)
59-65: Auto-submit relies on hardcoded button ID.The auto-submit uses
document.getElementById('submitButton')which is fragile. If the button ID changes or doesn't exist, the flow silently continues without submitting. Consider logging when the button is not found.// Auto-submit the form after successful data collection setTimeout(() => { const submitButton = document.getElementById('submitButton') as HTMLButtonElement; if (submitButton) { submitButton.click(); + } else { + console.warn('Submit button not found - auto-submit skipped'); } }, 500);
27-73: Consider documenting the timeout delays.The
setTimeoutat line 28 (100ms) and line 60 (500ms) use magic numbers. Adding brief comments explaining why these delays are necessary would improve maintainability.- // Automatically trigger Protect data collection - setTimeout(async () => { + // Automatically trigger Protect data collection after brief delay + // to ensure UI has finished rendering the message element + setTimeout(async () => {e2e/journey-app/components/device-profile.ts (1)
27-62: Consider removing nested setTimeout for better maintainability.The current implementation uses nested
setTimeoutcalls with arbitrary delays (100ms and 500ms). While this works for e2e testing, it could be simplified by removing the outer setTimeout and relying on the natural async flow.Consider this refactor:
- // Automatically trigger device profile collection - setTimeout(async () => { + // Automatically trigger device profile collection + (async () => { try { const isLocationRequired = callback.isLocationRequired(); const isMetadataRequired = callback.isMetadataRequired(); console.log('Collecting device profile...', { isLocationRequired, isMetadataRequired }); // Create device instance and collect profile const device = new Device(); const profile = await device.getProfile({ location: isLocationRequired, metadata: isMetadataRequired, }); console.log('Device profile collected successfully'); // Set the profile on the callback callback.setProfile(profile); message.innerText = 'Device profile collected successfully!'; message.style.color = 'green'; - // Auto-submit the form after successful collection - setTimeout(() => { + // Auto-submit the form after successful collection + await new Promise(resolve => setTimeout(resolve, 500)); - const submitButton = document.getElementById('submitButton') as HTMLButtonElement; - if (submitButton) { - submitButton.click(); - } - }, 500); + const submitButton = document.getElementById('submitButton') as HTMLButtonElement; + if (submitButton) { + submitButton.click(); + } } catch (error) { console.error('Device profile collection failed:', error); const errorMessage = error instanceof Error ? error.message : 'Unknown error'; message.innerText = `Collection failed: ${errorMessage}`; message.style.color = 'red'; } - }, 100); + })();e2e/journey-suites/src/protect.test.ts (1)
12-23: Use the navigate helper for consistency.The test uses
page.gotodirectly instead of thenavigatehelper fromasyncEvents, which is inconsistent with other tests in this suite (e.g.,device-profile.test.tsline 22). Thenavigatehelper ensures consistentwaitUntilbehavior across tests.Apply this diff:
-test('Test PingOne Protect journey flow', async ({ page }) => { - const { clickButton } = asyncEvents(page); +test('Test PingOne Protect journey flow', async ({ page }) => { + const { clickButton, navigate } = asyncEvents(page); const messageArray: string[] = []; page.on('console', async (msg) => { messageArray.push(msg.text()); return Promise.resolve(true); }); - await page.goto('/?journey=TEST_LoginPingProtect&clientId=basic', { waitUntil: 'load' }); + await navigate('/?journey=TEST_LoginPingProtect&clientId=basic');e2e/journey-app/components/ping-protect-initialize.ts (1)
38-91: Consider simplifying nested setTimeout structure.Similar to the device-profile component, the nested
setTimeoutcalls with arbitrary delays (100ms and 500ms) could be simplified for better maintainability.Consider this refactor:
- // Automatically trigger Protect initialization - setTimeout(async () => { + // Automatically trigger Protect initialization + (async () => { try { // Get configuration from callback const config = callback.getConfig(); console.log('Protect callback config:', config); if (!config?.envId) { const error = 'Missing envId in Protect configuration'; console.error(error); callback.setClientError(error); message.innerText = `Initialization failed: ${error}`; message.style.color = 'red'; return; } console.log('Initializing Protect with envId:', config.envId); // Create and store protect instance protectInstance = protect({ envId: config.envId }); console.log('Protect instance created'); // Initialize the Protect SDK console.log('Calling protect.start()...'); const result = await protectInstance.start(); console.log('protect.start() result:', result); if (result?.error) { console.error('Error initializing Protect:', result.error); callback.setClientError(result.error); message.innerText = `Initialization failed: ${result.error}`; message.style.color = 'red'; return; } console.log('Protect initialized successfully - no errors'); message.innerText = 'PingOne Protect initialized successfully!'; message.style.color = 'green'; - // Auto-submit the form after successful initialization - setTimeout(() => { + // Auto-submit the form after successful initialization + await new Promise(resolve => setTimeout(resolve, 500)); - const submitButton = document.getElementById('submitButton') as HTMLButtonElement; - if (submitButton) { - submitButton.click(); - } - }, 500); + const submitButton = document.getElementById('submitButton') as HTMLButtonElement; + if (submitButton) { + submitButton.click(); + } } catch (error) { console.error('Protect initialization failed:', error); const errorMessage = error instanceof Error ? error.message : 'Unknown error'; callback.setClientError(errorMessage); message.innerText = `Initialization failed: ${errorMessage}`; message.style.color = 'red'; } - }, 100); + })();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
e2e/am-mock-api/src/app/response.webauthn.js(1 hunks)e2e/am-mock-api/src/app/responses.js(1 hunks)e2e/am-mock-api/src/app/routes.auth.js(5 hunks)e2e/journey-app/components/device-profile.ts(2 hunks)e2e/journey-app/components/ping-protect-evaluation.ts(2 hunks)e2e/journey-app/components/ping-protect-initialize.ts(2 hunks)e2e/journey-app/package.json(1 hunks)e2e/journey-app/tsconfig.app.json(1 hunks)e2e/journey-app/tsconfig.json(1 hunks)e2e/journey-suites/src/device-profile.test.ts(1 hunks)e2e/journey-suites/src/protect.test.ts(1 hunks)e2e/journey-suites/src/qr-code.test.ts(1 hunks)e2e/journey-suites/src/recovery-codes.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-22T15:14:00.044Z
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
Applied to files:
e2e/am-mock-api/src/app/responses.js
📚 Learning: 2025-10-27T17:55:33.855Z
Learnt from: cerebrl
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: e2e/journey-app/main.ts:83-87
Timestamp: 2025-10-27T17:55:33.855Z
Learning: In the e2e/journey-app and similar e2e test applications in this repository, XSS hardening and similar production security practices are not required since these are exclusively test/development applications.
Applied to files:
e2e/journey-suites/src/protect.test.ts
🧬 Code graph analysis (6)
e2e/journey-suites/src/qr-code.test.ts (1)
e2e/am-mock-api/src/app/routes.auth.js (1)
username(137-137)
e2e/am-mock-api/src/app/response.webauthn.js (1)
e2e/am-mock-api/src/app/responses.js (2)
authSuccess(45-49)authSuccess(45-49)
e2e/journey-app/components/ping-protect-initialize.ts (1)
packages/protect/src/lib/protect.ts (1)
protect(30-96)
e2e/am-mock-api/src/app/routes.auth.js (2)
e2e/am-mock-api/src/app/responses.js (20)
protectInitializeResponse(1523-1578)protectInitializeResponse(1523-1578)selectIdPCallback(439-485)selectIdPCallback(439-485)MetadataMarketPlaceInitialize(1205-1250)MetadataMarketPlaceInitialize(1205-1250)qrCodeLoginResponse(1351-1368)qrCodeLoginResponse(1351-1368)deviceProfileLoginResponse(1370-1387)deviceProfileLoginResponse(1370-1387)protectLoginResponse(1389-1406)protectLoginResponse(1389-1406)protectEvaluateResponse(1580-1603)protectEvaluateResponse(1580-1603)deviceProfileCallbackResponse(1494-1521)deviceProfileCallbackResponse(1494-1521)qrCodeCallbacksResponse(1408-1492)qrCodeCallbacksResponse(1408-1492)authSuccess(45-49)authSuccess(45-49)e2e/am-mock-api/src/app/response.webauthn.js (4)
webAuthnRegistrationInit(15-65)webAuthnRegistrationInit(15-65)authSuccess(167-171)authSuccess(167-171)
e2e/journey-suites/src/device-profile.test.ts (2)
e2e/oidc-suites/src/utils/async-events.ts (3)
asyncEvents(7-76)navigate(54-56)clickButton(9-18)e2e/journey-suites/src/utils/demo-user.ts (2)
username(7-7)password(8-8)
e2e/journey-suites/src/protect.test.ts (3)
e2e/oidc-suites/src/utils/async-events.ts (2)
asyncEvents(7-76)clickButton(9-18)e2e/am-mock-api/src/app/routes.auth.js (1)
username(137-137)e2e/journey-suites/src/utils/demo-user.ts (2)
username(7-7)password(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pr
- GitHub Check: Mend Code Security Check
🔇 Additional comments (17)
e2e/journey-app/package.json (1)
19-19: Workspace dependency addition is correct.The
@forgerock/protectdependency is properly added with theworkspace:*protocol, consistent with other local package dependencies. This supports the Protect-related functionality (WebAuthn, recovery codes, etc.) referenced in the PR objectives.e2e/journey-app/tsconfig.app.json (1)
22-24: Reference format is consistent with other library package references.The new protect package reference follows the established pattern in
tsconfig.app.jsonwhere library packages are referenced via theirtsconfig.lib.jsonfiles, consistent with oidc-client and journey-client. The protect package's tsconfig.lib.json file exists at the referenced path.e2e/journey-app/tsconfig.json (1)
23-25: Reference format is consistent with existing package references.The new protect package reference follows the established pattern of other workspace package references (logger, oidc-client, journey-client) by pointing to the package directory without a tsconfig filename extension.
Verify that the protect package directory and its default
tsconfig.jsonexist atpackages/protect/.e2e/am-mock-api/src/app/response.webauthn.js (1)
1-171: LGTM - Well-structured WebAuthn mock responses.The module correctly encapsulates WebAuthn registration initialization and recovery codes display flow. The mock data structure follows the expected AM callback formats.
e2e/am-mock-api/src/app/responses.js (1)
1523-1603: LGTM - New Protect response exports.
protectInitializeResponseandprotectEvaluateResponseare well-structured and follow the existing callback patterns for PingOne Protect flows.e2e/am-mock-api/src/app/routes.auth.js (4)
410-410: Good defensive parsing.Using
JSON.parse(input.value || '{}')is a safer approach than parsing potentially undefined values directly. This prevents runtime errors when the input value is empty or missing.
346-367: LGTM - Protect journey flow logic.The updated
TEST_LoginPingProtectbranch correctly handles the three-step flow: initialization → login → evaluation → success. The optional chaining onauthId?.startsWithsafely handles undefined authId.
434-452: LGTM - WebAuthn with recovery codes flow.The WebAuthn flow correctly handles the multi-step process:
- MetadataCallback + HiddenValueCallback → returns recovery codes display
- ConfirmationCallback → sets session cookie and returns success
The conditional logic properly distinguishes between the different stages.
427-433: No action needed—the QR code condition is sufficiently specific.The identifiers
authIndexValue === 'QRCodeTest'andauthId === 'qrcode-journey-confirmation'are used exclusively for QR code flows (see lines 110, 379, and 1409), and the AND conjunction withConfirmationCallbackfurther restricts the condition to specific QR code states. No other authentication flows use these identifiers.e2e/journey-suites/src/qr-code.test.ts (1)
12-49: LGTM - Comprehensive QR code journey test.The test covers the complete flow: login → QR code display → confirmation → completion → logout. The console message assertions verify the expected application behavior.
e2e/journey-app/components/ping-protect-evaluation.ts (1)
45-51: Good error handling for getData() response.The code correctly checks if the result is an error object and handles it appropriately by setting the client error and updating the UI with visual feedback.
e2e/journey-suites/src/device-profile.test.ts (1)
12-47: LGTM! Well-structured e2e test.The test properly validates the device profile collection flow with appropriate visibility checks, timeouts, and console message assertions.
e2e/journey-suites/src/recovery-codes.test.ts (1)
11-55: LGTM! Comprehensive test coverage for recovery codes flow.The test properly validates the WebAuthn registration with recovery codes, including session token verification and logout flow.
e2e/journey-app/components/device-profile.ts (1)
14-26: LGTM! Clean component initialization.The function properly sets up the message element and appends it to the journey container.
e2e/journey-suites/src/protect.test.ts (1)
24-50: LGTM! Comprehensive Protect flow validation.The test properly validates the PingOne Protect initialization, risk assessment, and completion flow with appropriate visibility checks and console assertions.
e2e/journey-app/components/ping-protect-initialize.ts (2)
10-36: LGTM! Clean global instance management.The global
protectInstancestorage with thegetProtectInstance()getter provides a clean way for the evaluation component to access the initialized Protect SDK instance.
38-76: LGTM! Robust initialization with proper error handling.The initialization flow properly validates configuration, handles errors gracefully, and provides clear user feedback through UI messages and callback error propagation.
7edf5c0 to
2e8bca0
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.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we reran your CI automatically.
🎓 Learn more about Self-Healing CI on nx.dev
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e/journey-suites/src/recovery-codes.test.ts (1)
55-56: Replace fixed timeout with a state-based wait after logoutUsing
await page.waitForTimeout(1000);makes this test slower and potentially flaky. Prefer waiting on a specific UI state that indicates the journey has reset (for example, the initial Submit button returning, or the URL/login prompt).For example:
- // Verify we're back at the beginning - await page.waitForTimeout(1000); + // Verify we're back at the beginning + await expect(page.getByRole('button', { name: 'Submit' })).toBeVisible({ timeout: 10000 });This keeps the test deterministic and aligned with how the other waits in the suite are written.
🧹 Nitpick comments (3)
e2e/journey-app/components/device-profile.ts (1)
7-62: Device profile collection flow looks solid; only minor polish possibleThe async collection using
Device, theisLocationRequired/isMetadataRequiredchecks, and the auto-submit behavior all look correct and well-contained, with appropriate error handling and user feedback. If you ever need to tweak flakiness in tests, consider making the100/500mssetTimeoutdelays configurable or driven by DOM state rather than fixed times, but it’s not necessary for this e2e harness.e2e/am-mock-api/src/app/responses.js (1)
1351-1435: qrCodeCallbacksResponse duplicates otpQRCodeCallbacks – consider sharing a common definition
qrCodeCallbacksResponseis effectively a copy ofotpQRCodeCallbackswith a differentauthId. That’s fine for behavior, but it’s easy for one copy to drift if you ever tweak the script or hidden value.You could factor the shared callback structure once and reuse it:
- export const otpQRCodeCallbacks = { - authId: 'foo', - callbacks: [ - // ... - ], -}; +const baseOtpQrCodeCallbacks = { + callbacks: [ + // existing callbacks array here (TextOutput, HiddenValue, Confirmation) + ], +}; + +export const otpQRCodeCallbacks = { + authId: 'foo', + ...baseOtpQrCodeCallbacks, +}; + +export const qrCodeCallbacksResponse = { + authId: 'qrcode-journey-confirmation', + ...baseOtpQrCodeCallbacks, +};This keeps the QR-code script and callback wiring in one place.
e2e/am-mock-api/src/app/routes.auth.js (1)
400-420: Consider guarding JSON.parse and reusing callback lookups in DeviceProfile handlingThe DeviceProfile branch now does:
const inputArr = deviceCb.input || []; const input = inputArr[0] || {}; const value = JSON.parse(input.value || '{}'); const metadata = value.metadata || {}; // ... const hasIdentifier = value.identifier && value.identifier.length > 0;This is generally fine, but two optional robustness tweaks:
- Wrap
JSON.parsein a try/catch to avoid a 500 ifinput.valueis non-JSON (e.g., a malformed payload from a future test), and fall back to{}on error.- Since you already call
findfor this callback once, you could store and reuse it to avoid multiple linear scans when this branch grows further.Both are entirely optional in this mock/test API, but would make the handler a bit more defensive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
e2e/am-mock-api/src/app/response.webauthn.js(1 hunks)e2e/am-mock-api/src/app/responses.js(1 hunks)e2e/am-mock-api/src/app/routes.auth.js(5 hunks)e2e/journey-app/components/device-profile.ts(2 hunks)e2e/journey-app/components/ping-protect-evaluation.ts(2 hunks)e2e/journey-suites/src/device-profile.test.ts(1 hunks)e2e/journey-suites/src/protect.test.ts(1 hunks)e2e/journey-suites/src/qr-code.test.ts(1 hunks)e2e/journey-suites/src/recovery-codes.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/journey-suites/src/device-profile.test.ts
- e2e/journey-app/components/ping-protect-evaluation.ts
- e2e/journey-suites/src/qr-code.test.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-27T17:55:33.855Z
Learnt from: cerebrl
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: e2e/journey-app/main.ts:83-87
Timestamp: 2025-10-27T17:55:33.855Z
Learning: In the e2e/journey-app and similar e2e test applications in this repository, XSS hardening and similar production security practices are not required since these are exclusively test/development applications.
Applied to files:
e2e/journey-suites/src/protect.test.ts
📚 Learning: 2025-10-22T15:14:00.044Z
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
Applied to files:
e2e/am-mock-api/src/app/responses.js
🧬 Code graph analysis (3)
e2e/journey-app/components/device-profile.ts (1)
packages/davinci-client/src/lib/node.slice.ts (1)
error(72-110)
e2e/am-mock-api/src/app/response.webauthn.js (1)
e2e/am-mock-api/src/app/responses.js (2)
authSuccess(45-49)authSuccess(45-49)
e2e/journey-suites/src/protect.test.ts (2)
e2e/oidc-suites/src/utils/async-events.ts (2)
asyncEvents(7-76)clickButton(9-18)e2e/journey-suites/src/utils/demo-user.ts (2)
username(7-7)password(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (3)
e2e/am-mock-api/src/app/response.webauthn.js (1)
15-171: WebAuthn + recovery-codes mocks align well with existing callback patternsThe
webAuthnRegistrationInitmetadata, hidden-value wiring, andgetRecoveryCodesDisplay()flow (script TextOutput + confirmation) match the existing mock-callback patterns and what the new journey tests expect. The dedicatedauthSuccessobject for WebAuthn keeps this flow nicely scoped. Given this is an e2e mock API, the inline script in the TextOutputCallback is appropriate here. Based on learnings, XSS hardening isn’t required in this test harness.e2e/journey-suites/src/protect.test.ts (1)
12-49: Protect journey e2e flow looks consistent and robustThe test exercises initialization, login, risk evaluation, completion, and logout with clear UI assertions and console checks, and it aligns with the new
TEST_LoginPingProtectrouting. No issues spotted.e2e/am-mock-api/src/app/routes.auth.js (1)
95-108: Journey routing for Protect, QR-code, DeviceProfile, and WebAuthn flows is well-structuredThe new branches for:
TEST_LoginPingProtect+protect-journey-*authIds,- QR-code (
QRCodeTest,qrcode-journey-login,qrcode-journey-confirmation),- device profile (
DeviceProfileCallbackTest,device-profile-journey-*), and- WebAuthn with recovery codes (
TEST_WebAuthnWithRecoveryCodes,webauthn-registration-*,recovery-codes-*)are ordered correctly relative to the more generic PasswordCallback handling and appear to mirror the expectations in the new Playwright tests. Using distinct
authIdprefixes to track sub-states (init → login → eval/confirmation) keeps the flow easy to reason about, and the cookie setting for successful Protect/WebAuthn/QR-code paths matches existing patterns in this mock API.Also applies to: 340-359, 368-375, 422-447
d378c76 to
ed1cb7f
Compare
@forgerock/davinci-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. ❌ Your project status has failed because the head coverage (18.81%) 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 #501 +/- ##
==========================================
+ Coverage 18.79% 18.81% +0.02%
==========================================
Files 140 140
Lines 27640 27647 +7
Branches 980 981 +1
==========================================
+ Hits 5195 5202 +7
Misses 22445 22445 🚀 New features to boost your workflow:
|
|
Deployed f87a564 to https://ForgeRock.github.io/ping-javascript-sdk/pr-501/f87a564a5a77785d991f3258582ff225fc74f3a5 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.4 KB, -100.0%) 📊 Minor Changes📈 @forgerock/journey-client - 82.4 KB (+0.1 KB) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
cerebrl
left a comment
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.
Let's wait for this PR to have a bit better direct, explicit testing, rather than indirect testing for these additional modules.
| const submitButton = document.getElementById('submitButton') as HTMLButtonElement; | ||
| if (submitButton) { | ||
| submitButton.click(); | ||
| } |
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.
Do we not have a way to call the submit function without having to create a button and auto-clicking it?
| await clickButton('Submit', '/authenticate'); | ||
|
|
||
| // Step 2: QR Code step should be displayed with instruction message | ||
| await expect(page.getByText('Scan the QR code image below', { exact: false })).toBeVisible({ |
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 don't think this is testing the dedicated QR Code module since it isn't used in the test app. I don't think testing the message string itself is sufficient.
| // Step 2: Recovery Codes display step | ||
| // Should show the TextOutputCallback with recovery codes | ||
| // and a ConfirmationCallback with radio button option | ||
| await expect(page.getByText('I have saved my recovery codes')).toBeVisible({ timeout: 10000 }); |
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.
Same as the QR Code test; I don't believe this is testing the dedicated Recovery Codes module. So, this should be reworked.
| await expect(page.getByText('Collecting device profile information...')).toBeVisible({ | ||
| timeout: 10000, | ||
| }); | ||
| await expect(page.getByText('Device profile collected successfully!')).toBeVisible({ | ||
| timeout: 15000, | ||
| }); |
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 don't mind testing for these messages, indirectly testing Device Profile, but I think we need to more directly test the feature by grabbing the request body, parsing it and ensuring the JSON at least has the expected base properties with truthy values.
| await expect(page.getByText('Evaluating risk assessment...')).toBeVisible({ timeout: 10000 }); | ||
| await expect(page.getByText('Risk assessment completed successfully!')).toBeVisible({ | ||
| timeout: 15000, | ||
| }); |
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.
Same as the Device Profile test. Let's test the request body directly, rather than just testing for messages.
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
e2e/journey-app/components/recovery-codes.ts (1)
10-114: RecoveryCodes rendering looks correct; consider toning down sensitive debug logsThe step detection and rendering (codes grid, optional device, and confirmation radio wiring) look consistent with the RecoveryCodes API and should behave well in the journey app.
One minor concern: the debug logs currently print the full recovery codes and device name:
- Lines 18–20 log all codes and the device name to the console.
- These values are also indirectly asserted in tests via the log messages.
Given this lives in the e2e app against mock data, it’s acceptable, but if this component were ever reused outside the test harness it would be safer to log only non-sensitive metadata (e.g., code count) or gate these logs behind a debug flag.
e2e/journey-app/components/qr-code.ts (1)
10-87: QR Code step rendering is solid; be cautious with logging full QR dataThe QR step handler correctly:
- Detects QR steps,
- Renders message/URI/use-type elements with stable IDs used by the tests, and
- Wires the ConfirmationCallback radios including the default option.
One small nit: lines 17–19 and 28–41 log the full QR payload and URI, which typically contains a secret. That’s fine for the mock e2e app, but if this component is ever reused beyond tests, consider switching to less-sensitive logging (e.g., logging only the use-type and a redacted URI) or gating these logs behind an explicit debug flag.
e2e/journey-app/components/ping-protect-evaluation.ts (1)
27-72: Protect evaluation flow looks good; consider tightening the error-type guardThe async collection/auto-submit behavior and UI messaging look consistent with the journey tests and give clear feedback for both success and failure. One small robustness tweak: the
'error' in resultcheck can currently be reached even ifresultisnullor another non-object value, relying on the outertry/catchto recover.To make the intent clearer and avoid relying on the catch block for such shape mismatches, you could explicitly guard for an object before checking for
error:- // Collect device and behavioral data - const result = await protectInstance.getData(); - - // Check if result is an error object - if (typeof result !== 'string' && 'error' in result) { + // Collect device and behavioral data + const result = await protectInstance.getData(); + + // Check if result is an error object + if (result && typeof result === 'object' && 'error' in result) { console.error('Error collecting Protect data:', result.error); callback.setClientError(result.error); message.innerText = `Data collection failed: ${result.error}`; message.style.color = 'red'; return; }This keeps the behavior the same for the expected return types while making the error-path handling more explicit and future-proof if
getData()ever returns other falsy values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e/journey-app/components/device-profile.ts(2 hunks)e2e/journey-app/components/ping-protect-evaluation.ts(2 hunks)e2e/journey-app/components/ping-protect-initialize.ts(2 hunks)e2e/journey-app/components/qr-code.ts(1 hunks)e2e/journey-app/components/recovery-codes.ts(1 hunks)e2e/journey-app/main.ts(2 hunks)e2e/journey-suites/src/device-profile.test.ts(1 hunks)e2e/journey-suites/src/protect.test.ts(1 hunks)e2e/journey-suites/src/qr-code.test.ts(1 hunks)e2e/journey-suites/src/recovery-codes.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-22T15:14:00.044Z
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
Applied to files:
e2e/journey-app/main.ts
🧬 Code graph analysis (8)
e2e/journey-app/components/recovery-codes.ts (1)
packages/journey-client/src/lib/callbacks/confirmation-callback.ts (1)
ConfirmationCallback(14-78)
e2e/journey-app/main.ts (3)
e2e/journey-app/components/qr-code.ts (1)
renderQRCodeStep(10-88)e2e/journey-app/components/recovery-codes.ts (1)
renderRecoveryCodesStep(10-115)e2e/journey-app/callback-map.ts (1)
renderCallbacks(153-157)
e2e/journey-app/components/ping-protect-evaluation.ts (2)
e2e/journey-app/components/ping-protect-initialize.ts (1)
getProtectInstance(17-19)packages/davinci-client/src/lib/node.slice.ts (1)
error(72-110)
e2e/journey-app/components/device-profile.ts (1)
packages/journey-client/src/lib/callbacks/device-profile-callback.ts (2)
isLocationRequired(41-43)isMetadataRequired(34-36)
e2e/journey-suites/src/qr-code.test.ts (1)
e2e/am-mock-api/src/app/routes.auth.js (1)
username(131-131)
e2e/journey-app/components/qr-code.ts (1)
packages/journey-client/src/lib/callbacks/confirmation-callback.ts (1)
ConfirmationCallback(14-78)
e2e/journey-suites/src/protect.test.ts (2)
e2e/oidc-suites/src/utils/async-events.ts (2)
asyncEvents(7-76)clickButton(9-18)e2e/journey-suites/src/utils/demo-user.ts (2)
username(7-7)password(8-8)
e2e/journey-suites/src/device-profile.test.ts (2)
e2e/oidc-suites/src/utils/async-events.ts (2)
navigate(54-56)clickButton(9-18)e2e/journey-suites/src/utils/demo-user.ts (2)
username(7-7)password(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (7)
e2e/journey-app/main.ts (1)
14-15: Specialized QR/RecoveryCodes rendering integration looks correctImporting the QR and RecoveryCodes renderers and using:
const stepRendered = renderQRCodeStep(journeyEl, step) || renderRecoveryCodesStep(journeyEl, step); if (!stepRendered) { renderCallbacks(journeyEl, step.callbacks); }provides a clean, non-invasive way to prefer specialized step UIs while preserving the existing callback-based rendering for all other steps. The behavior for non-QR/non-recovery steps remains unchanged, and the submit button is still rendered once per form.
Also applies to: 120-126
e2e/journey-suites/src/qr-code.test.ts (1)
12-56: QR Code e2e test meaningfully exercises the dedicated QRCode moduleThis test does a good job of:
- Driving the QRCodeTest journey end-to-end (login → QR step → completion → logout),
- Verifying the rendered QR UI (message, otpauth URI,
Type: otp), and- Asserting console output that confirms the QRCode module was used and that the journey completed and logged out successfully.
The expectations line up well with the new renderer and main.ts wiring.
e2e/journey-suites/src/recovery-codes.test.ts (1)
11-65: Recovery Codes e2e test now directly validates the RecoveryCodes moduleThis test:
- Navigates the WebAuthn + Recovery Codes journey, submits the registration step, and verifies the recovery-codes UI (container, header text, codes list, at least one non-empty code).
- Confirms the presence of the “I have saved my recovery codes” prompt and successful completion/logout.
- Uses console assertions to confirm the RecoveryCodes module was invoked and the journey completed.
Switching to explicit visibility checks after logout (instead of a fixed timeout) is a nice robustness improvement.
e2e/journey-app/components/device-profile.ts (1)
7-63: Device profile collection component aligns well with callback semantics and testsThe updated component cleanly:
- Derives required flags via
isLocationRequired()/isMetadataRequired(),- Uses the Device helper to collect a profile and sets it on the callback,
- Surfaces status to the user via clear success/error messages, and
- Auto-submits the form after successful collection using
form.requestSubmit(), which ties into the existing submit handler without extra wiring.This fits the journey model and pairs nicely with the new device-profile e2e test that inspects the posted profile payload.
e2e/journey-app/components/ping-protect-initialize.ts (1)
7-91: Protect initialization component provides clear lifecycle and error handlingThe new implementation:
- Maintains a single shared Protect instance at module scope with a simple getter for the evaluation component,
- Reads configuration from the callback and explicitly guards against missing
envId,- Handles both start-time errors (via
result?.error) and thrown exceptions by settingsetClientErrorand updating the UI, and- Auto-submits the form after successful initialization so the journey can proceed without manual interaction.
This structure should make the Protect flows in the e2e journeys predictable and debuggable.
e2e/journey-suites/src/device-profile.test.ts (1)
12-90: Device profile e2e test now robustly validates the collected profile payloadThis test significantly improves coverage by:
- Intercepting POST
/authenticaterequests and extracting the JSON-encoded device profile from callback inputs,- Asserting both UI transitions (“Collecting…”, “Device profile collected successfully!”, “Complete”) and the shape of the posted profile (
identifier,metadata.hardware/browser/platform.deviceName), and- Verifying journey lifecycle through console logs and a full logout/back-to-login cycle.
The defensive parsing and null checks make the interception logic resilient while giving strong guarantees that the DeviceProfileCallback is populated as expected.
e2e/journey-suites/src/protect.test.ts (1)
12-83: E2E Protect journey coverage is strong and addresses prior payload-check feedbackThis test now exercises the full Protect journey end-to-end: it asserts the init/evaluation UI states, waits for the auto-submitted evaluation, and verifies completion plus logout. The
/authenticaterequest listener that drills intoPingOneProtectEvaluationCallbackand checksIDToken1signalsgives a solid, payload-level assertion rather than just relying on messages, and the console-log checks further validate the Protect SDK flow.The structure and timeouts look reasonable for avoiding flakes while keeping the test readable; I don’t see functional issues here.
b1a1a23 to
ac7fe33
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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
e2e/journey-app/components/qr-code.ts (1)
68-75: RedundantsetOptionIndexcall for default option.The default option index is set both in the
changeevent handler (when radio is already checked on mount) and explicitly again on lines 73-75. Since the checked state is set on line 68, the explicit call in the condition block is redundant.radio.addEventListener('change', () => { confirmCb.setOptionIndex(index); }); - if (index === confirmCb.getDefaultOption()) { - confirmCb.setOptionIndex(index); - } + // Initialize the callback with the default option value + if (radio.checked) { + confirmCb.setOptionIndex(index); + }Alternatively, initialize the default once before the loop:
confirmCb.setOptionIndex(confirmCb.getDefaultOption());e2e/journey-suites/src/qr-code.test.ts (1)
16-19: Consider removing the unnecessary return statement in console handler.The
Promise.resolve(true)return is unnecessary since the console event handler doesn't require a return value.page.on('console', async (msg) => { messageArray.push(msg.text()); - return Promise.resolve(true); });e2e/journey-suites/src/recovery-codes.test.ts (1)
18-21: Same cleanup opportunity as QR code test.The
Promise.resolve(true)return is unnecessary in the console handler.page.on('console', async (msg) => { messageArray.push(msg.text()); - return Promise.resolve(true); });e2e/journey-suites/src/device-profile.test.ts (1)
17-20: Same console handler cleanup opportunity.page.on('console', async (msg) => { messageArray.push(msg.text()); - return Promise.resolve(true); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e/journey-app/components/device-profile.ts(2 hunks)e2e/journey-app/components/ping-protect-evaluation.ts(2 hunks)e2e/journey-app/components/ping-protect-initialize.ts(2 hunks)e2e/journey-app/components/qr-code.ts(1 hunks)e2e/journey-app/components/recovery-codes.ts(1 hunks)e2e/journey-app/main.ts(2 hunks)e2e/journey-suites/src/device-profile.test.ts(1 hunks)e2e/journey-suites/src/protect.test.ts(1 hunks)e2e/journey-suites/src/qr-code.test.ts(1 hunks)e2e/journey-suites/src/recovery-codes.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/journey-app/components/recovery-codes.ts
- e2e/journey-app/components/ping-protect-initialize.ts
- e2e/journey-suites/src/protect.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-22T15:14:00.044Z
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
Applied to files:
e2e/journey-app/main.ts
🧬 Code graph analysis (5)
e2e/journey-app/components/qr-code.ts (1)
packages/journey-client/src/lib/callbacks/confirmation-callback.ts (1)
ConfirmationCallback(14-78)
e2e/journey-app/components/ping-protect-evaluation.ts (1)
e2e/journey-app/components/ping-protect-initialize.ts (1)
getProtectInstance(17-19)
e2e/journey-app/main.ts (3)
e2e/journey-app/components/qr-code.ts (1)
renderQRCodeStep(10-88)e2e/journey-app/components/recovery-codes.ts (1)
renderRecoveryCodesStep(10-115)e2e/journey-app/callback-map.ts (1)
renderCallbacks(153-157)
e2e/journey-suites/src/qr-code.test.ts (1)
e2e/am-mock-api/src/app/routes.auth.js (1)
username(131-131)
e2e/journey-suites/src/device-profile.test.ts (1)
e2e/journey-suites/src/utils/demo-user.ts (2)
username(7-7)password(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Mend Code Security Check
- GitHub Check: Mend Security Check
- GitHub Check: pr
🔇 Additional comments (8)
e2e/journey-app/components/ping-protect-evaluation.ts (1)
27-73: Solid async data collection implementation with comprehensive error handling.The implementation properly handles both error objects returned from
getData()and exceptions thrown during execution. The use ofform.requestSubmit()is the correct modern approach for programmatic form submission.One minor observation: the nested
setTimeoutpattern (outer 100ms, inner 500ms for auto-submit) works but could potentially be simplified if the SDK provided a callback-based flow. However, this is acceptable for an e2e test app.e2e/journey-app/components/qr-code.ts (1)
10-88: Well-structured step renderer with proper guard pattern.The implementation correctly uses the
QRCode.isQRCodeStepguard to detect and handle QR code steps, returningfalseearly for non-matching steps. The radio button wiring forConfirmationCallbackproperly usessetOptionIndexper the relevant code snippet fromconfirmation-callback.ts.e2e/journey-suites/src/qr-code.test.ts (1)
12-56: Good test coverage verifying QR Code module integration.The test effectively validates the QR Code module is being used by:
- Verifying the rendered UI elements (
#qr-code-container,#qr-code-uri, etc.)- Asserting console messages include
"QR Code step detected via QRCode module"(lines 50-52), which directly proves theQRCodemodule from@forgerock/journey-client/qr-codeis invokedThis addresses the earlier concern about whether the dedicated QR Code module is actually being tested.
e2e/journey-app/main.ts (1)
120-126: Clean step-type prioritization pattern.The early-out approach with
||short-circuit evaluation correctly tries specialized renderers before falling back torenderCallbacks. This pattern is extensible for future step types.Note: If a step could theoretically contain both QR code and recovery codes elements, only the QR code renderer would execute due to short-circuit. Based on the journey flow design, this appears to be the intended behavior.
e2e/journey-suites/src/recovery-codes.test.ts (1)
11-65: Comprehensive test validating RecoveryCodes module integration.The test properly validates:
- UI elements rendered by the RecoveryCodes module (
#recovery-codes-container,#recovery-codes-header,.recovery-code)- Console message assertion (lines 57-61) confirms
"Recovery Codes step detected via RecoveryCodes module"is logged, proving the dedicated module is invokedThis addresses the earlier concern about directly testing the Recovery Codes module.
e2e/journey-app/components/device-profile.ts (1)
27-62: Proper Device class integration with correct form submission approach.The implementation correctly:
- Uses
Deviceclass from the journey-client SDK- Respects callback requirements via
isLocationRequired()andisMetadataRequired()- Uses
form.requestSubmit()for programmatic submission (not button clicking)- Provides comprehensive error handling with user feedback
The pattern is consistent with
ping-protect-evaluation.ts, which aids maintainability.e2e/journey-suites/src/device-profile.test.ts (2)
22-44: Good implementation of request interception to validate device profile data.This directly addresses the earlier feedback to "more directly test the feature by grabbing the request body." The interception correctly:
- Filters for
/authenticatePOST requests- Parses the
DeviceProfileCallbackfrom callbacks array- Extracts the nested JSON from
IDToken1input value
62-75: Thorough validation of device profile structure.The assertions properly verify the captured device profile contains expected properties:
identifier(string, non-empty)metadata.hardware,metadata.browser,metadata.platformplatform.deviceNameThis provides confidence that the Device class is correctly collecting and serializing profile data.
cerebrl
left a comment
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.
Though I'd prefer to pass a submit form function from parent to child in the test application, I wouldn't call it blocking as what you did works. I just prefer the child component not to have to "know" anything outside of itself.
| const form = document.getElementById('form') as HTMLFormElement; | ||
| if (form) { | ||
| form.requestSubmit(); | ||
| } |
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'd much rather have a function that the parent passes to the child to submit the form without having to select the actual DOM element and calling a function off of that.
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4521
Description
Adds journey client tests to e2e's. uses the mock-api to serve some data
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.