Fixes current implemented E2E tests including MFA login - #4317
Open
Sadeesha-Sath wants to merge 1 commit into
Open
Fixes current implemented E2E tests including MFA login#4317Sadeesha-Sath wants to merge 1 commit into
Sadeesha-Sath wants to merge 1 commit into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR centralizes E2E defaults, updates OAuth and console authentication flows, dynamically discovers the sample application, restores MFA configuration after tests, adds sample-app sign-out configuration, and expands authentication test coverage. ChangesE2E configuration and authentication reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Playwright
participant SampleApp
participant ServerAPI
participant MockSMS
Playwright->>SampleApp: submit registration
SampleApp->>ServerAPI: POST /flow/execute
ServerAPI-->>Playwright: COMPLETE flow response
SampleApp->>MockSMS: deliver OTP
Playwright->>SampleApp: submit MFA login and OTP
SampleApp->>ServerAPI: validate credentials and OTP
ServerAPI-->>SampleApp: authenticated session
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-builder.yml (1)
910-914: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail when Sample App discovery fails.
This writes an empty
SAMPLE_APP_ID, buttests/e2e/tests/applications/application-edit.spec.tsrequires it and then fails later with a less actionable error. Exit this step non-zero instead.🤖 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 @.github/workflows/pr-builder.yml around lines 910 - 914, Update the Sample App discovery branch in the workflow step to exit non-zero after logging the warning, available applications, and empty SAMPLE_APP_ID output when SAMPLE_APP_ID is missing or null. Keep the successful discovery path unchanged so downstream tests receive the discovered ID.
🧹 Nitpick comments (1)
tests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.ts (1)
84-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared admin token getter before querying users.
captureCreatedUserIdstill sends the admin password grant directly to/oauth2/token, so it can drift from the flow-token path used by the rest of the e2e utility layer. Move this into the existing admin-auth utility or pass in the shared token instead of duplicating the request.🤖 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 `@tests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.ts` around lines 84 - 125, Update captureCreatedUserId to obtain the admin token through the existing shared admin-auth utility or receive the already-resolved shared token as an argument, removing its direct password-grant request to /oauth2/token. Preserve the existing user lookup, cleanup-list update, and best-effort error handling.
🤖 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 @.github/workflows/pr-builder.yml:
- Around line 1048-1051: Resolve a single admin username/password pair before
workflow startup using the documented Secret > Variable > defaults.env
precedence, including the corresponding variable names. Reuse those resolved
credentials for server bootstrap, token acquisition, and Playwright environment
values instead of hardcoded or independently resolved credentials, while
preserving the existing default fallback behavior.
In `@tests/e2e/pages/sample-app/sample-app-login.page.ts`:
- Around line 298-308: The getOTPErrorMessage() method currently swallows
Playwright strict-mode violations by converting all textContent() failures to an
empty string. Narrow its locator or explicitly select one matching element, such
as the first match, before calling textContent(), while preserving the
empty-string fallback when no OTP error is present.
In `@tests/e2e/run-e2e.sh`:
- Around line 304-311: Update the environment setup in the E2E runner after both
existing and newly created `.env` files are handled, ensuring the discovered
`SAMPLE_APP_ID` and other run-specific values are written or injected even when
`.env` already exists. Preserve the current defaults creation behavior and
ensure `application-edit.spec.ts` receives the required `SAMPLE_APP_ID`.
In `@tests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.ts`:
- Around line 507-515: Update the OTP page verification block around
verifyOTPPageLoaded in TC003 to let failures propagate instead of catching the
error and returning. Remove the tolerant missing-MFA path so steps 15–19 execute
only after successful OTP-page verification and test failure triggers the normal
cleanup lifecycle, including captureCreatedUserId.
In `@tests/e2e/utils/authentication/console-admin-auth-utils.ts`:
- Around line 217-230: Add tests for the token validation flow surrounding the
userinfo request in the authentication utilities, covering a successful
response, a non-OK response, and a request rejection/error. Verify each outcome
drives the expected fallback-to-inline-login behavior, including false results
for rejected or failed validation, and follow the existing test patterns and
mocks.
---
Outside diff comments:
In @.github/workflows/pr-builder.yml:
- Around line 910-914: Update the Sample App discovery branch in the workflow
step to exit non-zero after logging the warning, available applications, and
empty SAMPLE_APP_ID output when SAMPLE_APP_ID is missing or null. Keep the
successful discovery path unchanged so downstream tests receive the discovered
ID.
---
Nitpick comments:
In `@tests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.ts`:
- Around line 84-125: Update captureCreatedUserId to obtain the admin token
through the existing shared admin-auth utility or receive the already-resolved
shared token as an argument, removing its direct password-grant request to
/oauth2/token. Preserve the existing user lookup, cleanup-list update, and
best-effort error handling.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a83a5b0f-fb12-4951-b7aa-3036fd2c415b
📒 Files selected for processing (11)
.github/workflows/pr-builder.ymltests/e2e/.env.exampletests/e2e/README.mdtests/e2e/defaults.envtests/e2e/pages/sample-app/sample-app-login.page.tstests/e2e/run-e2e.shtests/e2e/tests/sample-app-authentication/sample-app-login.spec.tstests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.tstests/e2e/tests/settings/cors-allowed-origins.spec.tstests/e2e/utils/authentication/console-admin-auth-utils.tstests/e2e/utils/server-setup/mfa-setup.ts
Sadeesha-Sath
force-pushed
the
e2e_tests/fix_current_tests
branch
from
July 24, 2026 08:30
782fffb to
b4a921f
Compare
Sadeesha-Sath
requested review from
DonOmalVindula and
brionmario
as code owners
July 24, 2026 08:30
Contributor
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 `@tests/e2e/run-e2e.sh`:
- Around line 156-163: Remove the unconditional curl -k option from the flow
requests in tests/e2e/run-e2e.sh lines 156-163 so TLS certificate validation
remains enabled when sending admin credentials and challenge tokens; use an
explicit trusted CA bundle only if local certificates require it. Apply the same
certificate-validation-preserving change to the applications request at
tests/e2e/run-e2e.sh line 287 carrying ADMIN_TOKEN.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 684c70c1-a2f7-4179-b8a2-77e95dea34dd
📒 Files selected for processing (12)
.github/workflows/pr-builder.ymlsamples/apps/react-sdk-sample/thunderid-config/thunderid-config.yamltests/e2e/.env.exampletests/e2e/README.mdtests/e2e/defaults.envtests/e2e/pages/sample-app/sample-app-login.page.tstests/e2e/run-e2e.shtests/e2e/tests/sample-app-authentication/sample-app-login.spec.tstests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.tstests/e2e/tests/settings/cors-allowed-origins.spec.tstests/e2e/utils/authentication/console-admin-auth-utils.tstests/e2e/utils/server-setup/mfa-setup.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/e2e/defaults.env
- tests/e2e/tests/settings/cors-allowed-origins.spec.ts
- tests/e2e/tests/sample-app-authentication/sample-app-login.spec.ts
- tests/e2e/.env.example
- tests/e2e/pages/sample-app/sample-app-login.page.ts
- tests/e2e/README.md
- tests/e2e/utils/authentication/console-admin-auth-utils.ts
- tests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.ts
- tests/e2e/utils/server-setup/mfa-setup.ts
Sadeesha-Sath
force-pushed
the
e2e_tests/fix_current_tests
branch
from
July 24, 2026 09:10
b4a921f to
69a1a97
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/pr-builder.yml:
- Around line 722-734: Update .github/workflows/pr-builder.yml lines 722-734 in
the “Resolve Admin Credentials” step to declare vars.PLAYWRIGHT_ADMIN_PASSWORD
and resolve ADMIN_PASSWORD in Secret > Variable > defaults.env order. At lines
1067-1078, add the corresponding vars.PLAYWRIGHT_TEST_USER_* and
vars.SAMPLE_APP_* fallbacks for every credential, placing each variable between
its secret and env.DEFAULT_* value; preserve the existing credential resolution
behavior otherwise.
- Line 807: Update the request payload construction at the credential
authentication step to JSON-serialize the username and password values rather
than interpolating them directly into the JSON string. Preserve the existing
EXEC_ID, CHALLENGE_TOKEN, and action_001 fields while ensuring special
characters in ADMIN_USERNAME and ADMIN_PASSWORD produce valid JSON.
In `@tests/e2e/run-e2e.sh`:
- Around line 159-161: Validate CHALLENGE_TOKEN immediately after parsing
PROMPT_RESP and before the /flow/execute curl request; if it is empty or
invalid, terminate the script without submitting admin credentials. Preserve the
existing token extraction behavior while ensuring the second request only runs
with a non-empty challenge token.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d5533af-5656-450f-b5e8-1bf9b7908174
📒 Files selected for processing (12)
.github/workflows/pr-builder.ymlsamples/apps/react-sdk-sample/thunderid-config/thunderid-config.yamltests/e2e/.env.exampletests/e2e/README.mdtests/e2e/defaults.envtests/e2e/pages/sample-app/sample-app-login.page.tstests/e2e/run-e2e.shtests/e2e/tests/sample-app-authentication/sample-app-login.spec.tstests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.tstests/e2e/tests/settings/cors-allowed-origins.spec.tstests/e2e/utils/authentication/console-admin-auth-utils.tstests/e2e/utils/server-setup/mfa-setup.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/e2e/defaults.env
- samples/apps/react-sdk-sample/thunderid-config/thunderid-config.yaml
- tests/e2e/.env.example
- tests/e2e/pages/sample-app/sample-app-login.page.ts
- tests/e2e/tests/sample-app-authentication/sample-app-login.spec.ts
- tests/e2e/utils/server-setup/mfa-setup.ts
- tests/e2e/tests/settings/cors-allowed-origins.spec.ts
- tests/e2e/README.md
- tests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.ts
Sadeesha-Sath
force-pushed
the
e2e_tests/fix_current_tests
branch
2 times, most recently
from
July 24, 2026 10:17
1e2e71e to
62cb853
Compare
12 tasks
Sadeesha-Sath
force-pushed
the
e2e_tests/fix_current_tests
branch
from
July 24, 2026 17:58
62cb853 to
36e2e78
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/utils/server-setup/mfa-setup.ts (1)
505-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate inline "flow bindings" type.
The same anonymous object type (
authFlowId,registrationFlowId,recoveryFlowId,isRegistrationFlowEnabled) is declared inline in bothupdateApplicationFlows()'s return type andrevertApplicationFlows()'s parameter type. Extracting a sharedinterface/type(e.g.ApplicationFlowBindings) would avoid duplication and keep both signatures in sync if the shape changes.♻️ Suggested refactor
+interface ApplicationFlowBindings { + authFlowId: string; + registrationFlowId: string; + recoveryFlowId: string | null; + isRegistrationFlowEnabled: boolean; +} + private async updateApplicationFlows( adminToken: string, authFlowId: string, registrationFlowId: string - ): Promise<{ - appId: string; - originalFlows: { - authFlowId: string; - registrationFlowId: string; - recoveryFlowId: string | null; - isRegistrationFlowEnabled: boolean; - }; - }> { + ): Promise<{ appId: string; originalFlows: ApplicationFlowBindings }> {private async revertApplicationFlows( adminToken: string, appId: string, - originalFlows: { - authFlowId: string; - registrationFlowId: string; - recoveryFlowId: string | null; - isRegistrationFlowEnabled: boolean; - } + originalFlows: ApplicationFlowBindings ): Promise<void> {Also applies to: 593-598
🤖 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 `@tests/e2e/utils/server-setup/mfa-setup.ts` around lines 505 - 513, Extract the duplicated flow-binding object shape into a shared type or interface, such as ApplicationFlowBindings, near the related declarations. Update both updateApplicationFlows()’s return type and revertApplicationFlows()’s parameter type to reuse it, preserving the existing fields and nullability.
🤖 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 `@tests/e2e/README.md`:
- Around line 78-79: Update the configuration instruction near the run-e2e.sh
description to replace the em dash with a comma or period, preserving the
existing meaning and links while ensuring the text contains no em dash.
---
Nitpick comments:
In `@tests/e2e/utils/server-setup/mfa-setup.ts`:
- Around line 505-513: Extract the duplicated flow-binding object shape into a
shared type or interface, such as ApplicationFlowBindings, near the related
declarations. Update both updateApplicationFlows()’s return type and
revertApplicationFlows()’s parameter type to reuse it, preserving the existing
fields and nullability.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 38e51ab9-e95c-4afc-8292-ede555dd4c2d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.github/workflows/pr-builder.ymlsamples/apps/react-sdk-sample/thunderid-config/thunderid-config.yamltests/e2e/.env.exampletests/e2e/README.mdtests/e2e/defaults.envtests/e2e/pages/sample-app/sample-app-login.page.tstests/e2e/run-e2e.shtests/e2e/tests/sample-app-authentication/sample-app-login.spec.tstests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.tstests/e2e/tests/settings/cors-allowed-origins.spec.tstests/e2e/utils/authentication/console-admin-auth-utils.tstests/e2e/utils/server-setup/mfa-setup.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/e2e/defaults.env
- tests/e2e/.env.example
- samples/apps/react-sdk-sample/thunderid-config/thunderid-config.yaml
- tests/e2e/tests/sample-app-authentication/sample-app-login.spec.ts
- tests/e2e/pages/sample-app/sample-app-login.page.ts
- tests/e2e/tests/settings/cors-allowed-origins.spec.ts
- tests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.ts
- tests/e2e/utils/authentication/console-admin-auth-utils.ts
- .github/workflows/pr-builder.yml
Sadeesha-Sath
force-pushed
the
e2e_tests/fix_current_tests
branch
2 times, most recently
from
July 26, 2026 09:38
1846ceb to
b891b01
Compare
rajithacharith
added
the
trigger-pr-builder
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Sadeesha-Sath
force-pushed
the
e2e_tests/fix_current_tests
branch
from
July 27, 2026 09:47
b891b01 to
dd4bd75
Compare
Sadeesha-Sath
force-pushed
the
e2e_tests/fix_current_tests
branch
from
July 29, 2026 14:32
dd4bd75 to
d66d618
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fix the currently implemented E2E test suite, both locally and in CI, and un-skip the MFA registration + login test that was previously marked fixme.
The suite had several issues currently:
run-e2e.sh, and the CI workflow each carried their own copy of the test dataset, so the data can be inconsistent.run-e2e.shadmin token request was missing the resource parameter (RFC 8707), and the sample app ID was hardcoded, so a config change to the sample could break the script.SAMPLE_APP_URLas its test origin, and its cleanup would strip that origin from future testsrun-e2e.sh's admin token bootstrap didn't account for the console login flow's new SSO check, so it broke as soon as SSO reuse landed on the console appApproach
Config consolidation (single source of truth):
tests/e2e/defaults.envas the canonical fixed dataset. Bothrun-e2e.sh(local) and the PR workflow now load it, so defaults are consistent.pr-builder.yamlto loaddefaults.envand fall back to it, keeping the existing variable priority.tests/e2e/.env.exampleandtests/e2e/README.mdto show that.envis auto-generated and only needed for local overrides.Test stability fixes
waitFor(using a combinedlocator.or()) for both the logged-in indicators and the logout affordance, fixing the redirect race condition on Firefox/WebKit.getOTPErrorMessage()as a reusable helper.run-e2e.sh:resourceparameter to the admin authorize request (tokens are bound to a single resource server per RFC 8707).MFA test fixes
TC003. Since the React SDK's SignUp component does not redirect after sign-up (unlike SignIn), the test now verifies registration by asserting on the/flow/executeresponse body, rather than waiting for the missing URL redirectupdateApplicationFlowsnow returns the original flow bindings, and arevertApplicationFlowscleanup step restores them in afterAll.captureCreatedUserId()for cleanup.Sample-App client initiated logout flow support
post_logout_redirect_uri+id_token_hintto/oauth2/logouton sign-out.thunderid-config.yamlof the sample app and wired the application to itverifyLoggedOut()to assert the app's home pageOther:
SAMPLE_APP_URL, so its cleanup won't affect other functions.verifyAuthStatenow confirms the stored token with a real/oauth2/userinfocall, catching server-rejected tokens that client-side checks previously missed.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests