test: improve protect e2e tests - #132
Conversation
📝 WalkthroughWalkthroughPingOne Protect initialization now runs at module scope in the login widget. Journey, DaVinci, and login-widget E2E tests now capture authentication requests and validate Protect risk data. Formatting and Playwright dependency declarations were also updated. ChangesProtect authentication flows
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant LoginApplication
participant AuthenticationEndpoint
participant PingOneProtect
Test->>LoginApplication: submit explicit credentials
LoginApplication->>AuthenticationEndpoint: send authentication request
AuthenticationEndpoint-->>Test: return IDToken1signals risk data
Test->>PingOneProtect: verify initialization logs
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
javascript/reactjs-todo-journey/e2e/protect.spec.jsOops! Something went wrong! :( ESLint: 8.57.1 TypeError: Converting circular structure to JSON 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.js`:
- Line 56: In javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.js at
lines 56-56 and 98-98, store each console listener callback in a variable before
registering it with page.on, then pass that same reference to
page.removeListener at both sites so the listeners are actually detached.
In `@javascript/reactjs-todo-login-widget/e2e/protect.spec.js`:
- Around line 40-42: Register each relevant Playwright request waiter before the
triggering submission, then await it afterward: in
javascript/reactjs-todo-login-widget/e2e/protect.spec.js lines 40-42, prepare
the Protect evaluation waiter before clicking Next; in
javascript/reactjs-todo-journey/e2e/protect.spec.js lines 49-56 and 96-103,
prepare the callback and bootstrap evaluation waiters before clicking Sign In
and await them before checking riskData; in
javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.js lines 42-49 and
84-91, wait for the customHTMLTemplate request containing riskSDK before
evaluating requests and riskData.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a2755164-d2b3-4e4e-ad34-9c8341ffdaf2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.jsjavascript/reactjs-todo-journey/e2e/protect.spec.jsjavascript/reactjs-todo-login-widget/client/index.jsjavascript/reactjs-todo-login-widget/e2e/protect.spec.js
| logs.includes('PingOne Protect initialized by collector for data collection'), | ||
| ).toBeFalsy(); | ||
|
|
||
| page.removeListener('console', (msg) => console.log(msg.text())); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.js"
rg -n -C 4 "page\.(on|removeListener)\('console'" "$file"Repository: ForgeRock/sdk-sample-apps
Length of output: 1120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.js"
printf '%s\n' '--- relevant source ---'
cat -n "$file" | sed -n '1,115p'
printf '%s\n' '--- listener cleanup patterns in related tests ---'
rg -n -C 3 "page\.(on|off|removeListener)\('console'|page\.(on|off|removeListener)\(\"console\"" javascript/reactjs-todo-davinci javascript/reactjs-todo-login-widget javascript/reactjs-todo-journey
printf '%s\n' '--- available Playwright references ---'
rg -n -C 3 "playwright|`@playwright/test`" package.json package-lock.json yarn.lock pnpm-lock.yaml javascript 2>/dev/null | head -200 || true
printf '%s\n' '--- callback identity probe ---'
node - <<'JS'
const { EventEmitter } = require('node:events');
const page = new EventEmitter();
const registered = (msg) => console.log(msg);
page.on('console', registered);
const removalCandidate = (msg) => console.log(msg);
page.removeListener('console', removalCandidate);
console.log(JSON.stringify({
registeredListenerCount: page.listenerCount('console'),
sameFunctionObject: registered === removalCandidate,
remainingListenerIsRegistered: page.listeners('console')[0] === registered,
}));
JSRepository: ForgeRock/sdk-sample-apps
Length of output: 23957
Store and reuse each console listener callback.
removeListener receives a new function at lines 56 and 98, so the registered listeners remain attached. Pass the same callback reference to page.on and page.removeListener.
📍 Affects 1 file
javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.js#L56-L56(this comment)javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.js#L98-L98
🤖 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 `@javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.js` at line 56, In
javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.js at lines 56-56 and
98-98, store each console listener callback in a variable before registering it
with page.on, then pass that same reference to page.removeListener at both sites
so the listeners are actually detached.
59910cd to
e75260d
Compare
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 `@javascript/reactjs-todo-journey/client/styles/_custom-variables.scss`:
- Around line 107-108: Update the $font-family-sans-serif declaration by
changing the Arial font family keyword to lowercase arial, preserving the rest
of the fallback font list unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a2acd15d-f4a8-491c-a306-8253f16fff07
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.jsjavascript/reactjs-todo-davinci/package.jsonjavascript/reactjs-todo-journey/client/components/journey/password.jsxjavascript/reactjs-todo-journey/client/components/journey/web-authn.jsxjavascript/reactjs-todo-journey/client/context/README.mdjavascript/reactjs-todo-journey/client/index.htmljavascript/reactjs-todo-journey/client/styles/_custom-variables.scssjavascript/reactjs-todo-journey/e2e/protect.spec.jsjavascript/reactjs-todo-journey/public/callback.htmljavascript/reactjs-todo-login-widget/e2e/protect.spec.jsjavascript/reactjs-todo-login-widget/package.jsonjavascript/reactjs-todo-oidc/package.jsonpackage.json
💤 Files with no reviewable changes (3)
- javascript/reactjs-todo-login-widget/package.json
- javascript/reactjs-todo-oidc/package.json
- javascript/reactjs-todo-davinci/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.js
- javascript/reactjs-todo-journey/e2e/protect.spec.js
- javascript/reactjs-todo-login-widget/e2e/protect.spec.js
| $font-family-sans-serif: | ||
| 'Open Sans', 'Segoe UI', 'Helvetica Neue', Arial, 'Noto Sans', 'Liberation Sans', sans-serif; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Stylelint value-keyword-case error.
Stylelint requires Arial to use lowercase in this declaration. Change it to arial so the stylesheet passes lint.
Proposed fix
$font-family-sans-serif:
- 'Open Sans', 'Segoe UI', 'Helvetica Neue', Arial, 'Noto Sans', 'Liberation Sans', sans-serif;
+ 'Open Sans', 'Segoe UI', 'Helvetica Neue', arial, 'Noto Sans', 'Liberation Sans', sans-serif;📝 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.
| $font-family-sans-serif: | |
| 'Open Sans', 'Segoe UI', 'Helvetica Neue', Arial, 'Noto Sans', 'Liberation Sans', sans-serif; | |
| $font-family-sans-serif: | |
| 'Open Sans', 'Segoe UI', 'Helvetica Neue', arial, 'Noto Sans', 'Liberation Sans', sans-serif; |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 108-108: Expected "Arial" to be "arial" (value-keyword-case)
(value-keyword-case)
🤖 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 `@javascript/reactjs-todo-journey/client/styles/_custom-variables.scss` around
lines 107 - 108, Update the $font-family-sans-serif declaration by changing the
Arial font family keyword to lowercase arial, preserving the rest of the
fallback font list unchanged.
Source: Linters/SAST tools
33e5a68 to
e182571
Compare
| }, | ||
| "devDependencies": { | ||
| "@playwright/test": "^1.60.0", | ||
| "@playwright/test": "~1.60.0", |
There was a problem hiding this comment.
Changing this to only pull patch updates because the latest minor requires node >= 20 while our CI still uses node v18.
e182571 to
5e33ed0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@javascript/reactjs-todo-login-widget/e2e/protect.spec.js`:
- Around line 8-12: Update the setup around the page console listener and login
flow in the test so it waits until logs contains the exact “PingOne Protect
initialized at bootstrap” message before submitting credentials or triggering
the Protect request. Preserve the existing console capture in the logs array and
use that initialization signal as the gate for continuing the test.
- Around line 21-23: Unify authentication-request matching in the listener and
protectPromise by defining or reusing one normalized URL predicate near the
callback logic. Replace the exact-equality check in protectPromise with the same
predicate used by the includes-based listener, so requests with query parameters
are handled consistently.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b76416c1-eb47-4df2-b9f4-49941749ace8
📒 Files selected for processing (3)
javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.jsjavascript/reactjs-todo-journey/e2e/protect.spec.jsjavascript/reactjs-todo-login-widget/e2e/protect.spec.js
🚧 Files skipped from review as they are similar to previous changes (2)
- javascript/reactjs-todo-journey/e2e/protect.spec.js
- javascript/reactjs-todo-davinci/e2e/davinci-protect.spec.js
| const logs = []; | ||
| page.on('console', async (msg) => { | ||
| logs.push(msg.text()); | ||
| return Promise.resolve(true); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- target file ---'
cat -n javascript/reactjs-todo-login-widget/e2e/protect.spec.js
echo '--- client bootstrap references ---'
rg -n -C 8 "console|bootstrap|Protect|protect|initialized|initializ" javascript/reactjs-todo-login-widget/client/index.js
echo '--- related test and client files ---'
git ls-files javascript/reactjs-todo-login-widget | sed -n '1,120p'Repository: ForgeRock/sdk-sample-apps
Length of output: 10321
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
test = Path("javascript/reactjs-todo-login-widget/e2e/protect.spec.js").read_text()
client = Path("javascript/reactjs-todo-login-widget/client/index.js").read_text()
required_test_fragments = [
"page.on('console'",
"logs.includes('PingOne Protect initialized at bootstrap')",
]
for fragment in required_test_fragments:
print(f"test contains {fragment!r}: {fragment in test}")
test_positions = {
"console listener": test.index("page.on('console'"),
"page.goto": test.index("await page.goto"),
"credentials": test.index("await page.getByRole('textbox', { name: 'Username' })"),
"bootstrap assertion": test.index("logs.includes('PingOne Protect initialized at bootstrap')"),
}
for name, position in test_positions.items():
print(f"{name}: {position}")
print("bootstrap log is emitted by client:",
"console.log('PingOne Protect initialized at bootstrap')" in client)
print("assertion precedes credentials:",
test_positions["bootstrap assertion"] < test_positions["credentials"])
PY
echo '--- exact relevant test ordering ---'
sed -n '6,56p' javascript/reactjs-todo-login-widget/e2e/protect.spec.jsRepository: ForgeRock/sdk-sample-apps
Length of output: 2309
Assert bootstrap initialization before the login flow.
The test checks logs only after submitting credentials and receiving the Protect request. Wait for the exact PingOne Protect initialized at bootstrap message before starting the login flow.
🤖 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 `@javascript/reactjs-todo-login-widget/e2e/protect.spec.js` around lines 8 -
12, Update the setup around the page console listener and login flow in the test
so it waits until logs contains the exact “PingOne Protect initialized at
bootstrap” message before submitting credentials or triggering the Protect
request. Preserve the existing console capture in the logs array and use that
initialization signal as the gate for continuing the test.
| if (method === 'POST' && payload && requestUrl.includes(authenticateRequestUrl)) { | ||
| const callback = payload.callbacks?.find( | ||
| (callback) => callback.type === 'PingOneProtectEvaluationCallback', |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file='javascript/reactjs-todo-login-widget/e2e/protect.spec.js'
printf '%s\n' '--- file outline ---'
ast-grep outline "$file" || true
printf '%s\n' '--- target file ---'
cat -n "$file"
printf '%s\n' '--- related identifiers ---'
rg -n -C 4 'authenticateRequestUrl|protectPromise|riskData|PingOneProtectEvaluationCallback|requestUrl' javascript/reactjs-todo-login-widget
printf '%s\n' '--- diff summary ---'
git diff --stat -- "$file"Repository: ForgeRock/sdk-sample-apps
Length of output: 8079
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- authenticate endpoint references ---'
rg -n -C 3 'openam-sdks\.forgeblocks\.com|/am/json/alpha/authenticate|authenticateRequestUrl|PingOneProtectEvaluationCallback' --glob '!**/node_modules/**' .
printf '%s\n' '--- relevant package and configuration files ---'
rg -n -C 3 'PingOneProtect|TEST_Protect|journey=TEST_Protect|authenticate' javascript/reactjs-todo-login-widget --glob '*.js' --glob '*.jsx' --glob '*.ts' --glob '*.tsx' --glob '*.json' --glob '*.env*' --glob '*.yml' --glob '*.yaml'
printf '%s\n' '--- predicate behavior probe ---'
node - <<'JS'
const base = 'https://openam-sdks.forgeblocks.com/am/json/alpha/authenticate';
const urls = [
base,
`${base}?clientId=example`,
`${base}/extra`,
`https://evil.example/?target=${encodeURIComponent(base)}`,
];
for (const url of urls) {
console.log(JSON.stringify({
url,
includes: url.includes(base),
exact: url === base,
mismatch: url.includes(base) && url !== base,
}));
}
JSRepository: ForgeRock/sdk-sample-apps
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -eu
log='/tmp/coderabbit-shell-logs/shell-output-AffoeO'
printf '%s\n' '--- predicate probe output ---'
rg -n -A 40 -- '--- predicate behavior probe ---' "$log" || true
printf '%s\n' '--- target and parallel test URL predicates ---'
rg -n 'requestUrl\.includes\(authenticateRequestUrl\)|req\.url\(\) === authenticateRequestUrl' \
javascript/reactjs-todo-login-widget/e2e/protect.spec.js \
javascript/reactjs-todo-journey/e2e/protect.spec.jsRepository: ForgeRock/sdk-sample-apps
Length of output: 1591
Use one URL predicate for the authentication request.
The listener uses includes, but protectPromise uses exact equality. If the request has query parameters, the listener captures riskData while protectPromise times out. Reuse one normalized predicate at lines 21–23 and 36–43.
🤖 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 `@javascript/reactjs-todo-login-widget/e2e/protect.spec.js` around lines 21 -
23, Unify authentication-request matching in the listener and protectPromise by
defining or reusing one normalized URL predicate near the callback logic.
Replace the exact-equality check in protectPromise with the same predicate used
by the includes-based listener, so requests with query parameters are handled
consistently.
2 participants
https://pingidentity.atlassian.net/browse/SDKS-5240
Summary by CodeRabbit