test: improve protect e2e tests - #756
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (25)
💤 Files with no reviewable changes (23)
📝 WalkthroughWalkthroughProtect E2E tests now capture risk data from authentication callbacks and validate its format. The journey client is added to the journey suite. Standalone Protect projects are removed. DaVinci API reports reorder existing union members. ChangesProtect E2E consolidation
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: ⚪ Minimal · up to This PR updates end-to-end test coverage and removes the standalone Protect test app without supplied evidence of a current correctness or production-impact risk; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 8970b3a
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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 `@e2e/journey-suites/src/protect.test.ts`:
- Around line 24-30: Move request.postDataJSON() in each request listener after
checking that the method is POST and the URL targets /authenticate (or the
listener’s existing target), so unrelated requests are filtered before body
parsing. Apply this in e2e/journey-suites/src/protect.test.ts:24-30, both
customHTMLTemplate and customForm listeners in
e2e/davinci-suites/src/protect.test.ts:26-34 and 71-79, and
e2e/protect-suites/src/protect-native.test.ts:28-34; preserve the existing
payload validation and handling after the filter.
In `@e2e/protect-suites/playwright.config.ts`:
- Line 3: Correct the copyright headers: in
e2e/protect-suites/playwright.config.ts lines 3-3, use the repository-standard
year range without the extra space and change the wording to “All rights
reserved.”; in e2e/protect-suites/src/protect-native.test.ts lines 3-3, change
“All right reserved.” to “All rights reserved.”
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e5a96dcf-86f5-4a09-8559-8f80beae67df
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
e2e/davinci-app/server-configs.tse2e/davinci-suites/src/protect.test.tse2e/journey-suites/package.jsone2e/journey-suites/src/protect.test.tse2e/journey-suites/tsconfig.jsone2e/protect-suites/package.jsone2e/protect-suites/playwright.config.tse2e/protect-suites/src/protect-native.test.ts
| page.on('request', (request) => { | ||
| if (request.url().includes('/authenticate') && request.method() === 'POST') { | ||
| try { | ||
| const postData = request.postData(); | ||
| if (postData) { | ||
| const body = JSON.parse(postData); | ||
| const callbacks = body.callbacks || []; | ||
| for (const callback of callbacks) { | ||
| if (callback.type === 'PingOneProtectEvaluationCallback') { | ||
| const inputs = callback.input || []; | ||
| for (const input of inputs) { | ||
| if (input.name === 'IDToken1signals' && input.value) { | ||
| protectSignalsData = input.value; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } catch { | ||
| // Ignore parsing errors | ||
| const method = request.method(); | ||
| const requestUrl = request.url(); | ||
| const payload = request.postDataJSON(); | ||
|
|
||
| // Only process POST requests with JSON payloads | ||
| if (method === 'POST' && payload && requestUrl.includes('/authenticate')) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For the Playwright version declared in this repository, does Request.postDataJSON() throw when the request body is not valid JSON?
💡 Result:
Yes, the Request.postDataJSON method in Playwright throws an error if the request body is present but cannot be parsed as valid JSON [1]. Internally, the method first checks if the request has body data [1]. If it does, and the Content-Type is not application/x-www-form-urlencoded, it attempts to parse the data using JSON.parse [1]. If this parsing fails, it explicitly throws an Error with the message "POST data is not a valid JSON object: " followed by the content of the request body [1]. This behavior is confirmed by Playwright's internal source code [1] and its test suite, which includes test cases explicitly verifying that the method throws an exception when provided with invalid JSON [2].
Citations:
- 1: https://github.com/microsoft/playwright/blob/54e92be7/packages/playwright-core/src/client/network.ts
- 2: https://github.com/microsoft/playwright-dotnet/blob/6b41bfe7/src/Playwright.Tests/NetworkPostDataTests.cs
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Playwright declarations ---'
rg -n --hidden -S '"@?playwright/test"|"playwright"|playwright' \
package.json package-lock.json yarn.lock pnpm-lock.yaml \
e2e 2>/dev/null | head -200 || true
printf '%s\n' '--- Target listener implementations ---'
for f in \
e2e/journey-suites/src/protect.test.ts \
e2e/davinci-suites/src/protect.test.ts \
e2e/protect-suites/src/protect-native.test.ts
do
echo "--- $f ---"
sed -n '1,115p' "$f"
doneRepository: ForgeRock/ping-javascript-sdk
Length of output: 15908
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from urllib.request import urlopen
url = "https://raw.githubusercontent.com/microsoft/playwright/v1.59.1/packages/playwright-core/src/client/network.ts"
text = urlopen(url, timeout=10).read().decode()
needle = "postDataJSON()"
start = text.index(needle)
print(text[start:start + 900])
PY
printf '%s\n' '--- Exact lockfile resolution ---'
sed -n '11115,11130p' pnpm-lock.yaml
sed -n '16035,16045p' pnpm-lock.yamlRepository: ForgeRock/ping-javascript-sdk
Length of output: 2453
Filter each request before parsing its body.
request.postDataJSON() can throw when an unrelated request has a non-empty, invalid JSON body. Check the method and target URL before calling it in all four listeners:
e2e/journey-suites/src/protect.test.tse2e/davinci-suites/src/protect.test.ts(customHTMLTemplateandcustomForm)e2e/protect-suites/src/protect-native.test.ts
📍 Affects 3 files
e2e/journey-suites/src/protect.test.ts#L24-L30(this comment)e2e/davinci-suites/src/protect.test.ts#L26-L34e2e/davinci-suites/src/protect.test.ts#L71-L79e2e/protect-suites/src/protect-native.test.ts#L28-L34
🤖 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/journey-suites/src/protect.test.ts` around lines 24 - 30, Move
request.postDataJSON() in each request listener after checking that the method
is POST and the URL targets /authenticate (or the listener’s existing target),
so unrelated requests are filtered before body parsing. Apply this in
e2e/journey-suites/src/protect.test.ts:24-30, both customHTMLTemplate and
customForm listeners in e2e/davinci-suites/src/protect.test.ts:26-34 and 71-79,
and e2e/protect-suites/src/protect-native.test.ts:28-34; preserve the existing
payload validation and handling after the filter.
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/recognize
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
|
Deployed 0472f84 to https://ForgeRock.github.io/ping-javascript-sdk/pr-756/0472f8475e039a74e7155a3e18e0caa2fc66fc30 branch gh-pages in ForgeRock/ping-javascript-sdk |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (24.11%) 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 #756 +/- ##
==========================================
+ Coverage 18.07% 24.11% +6.03%
==========================================
Files 155 162 +7
Lines 24398 25787 +1389
Branches 1203 1686 +483
==========================================
+ Hits 4410 6218 +1808
+ Misses 19988 19569 -419 🚀 New features to boost your workflow:
|
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/journey-client - 92.6 KB (new) ➖ No Changes➖ @forgerock/davinci-client - 59.1 KB 15 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
895d492 to
834c344
Compare
4 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-5240
Note: Maybe we can consider removing the protect app and its suites now since protect is tested through the davinci-app and journey-app.
Summary by CodeRabbit