WEB-890: Implement storageState authentication bypass for Playwright E2E#3457
Open
DeathGun44 wants to merge 1 commit intoopenMF:devfrom
Open
WEB-890: Implement storageState authentication bypass for Playwright E2E#3457DeathGun44 wants to merge 1 commit intoopenMF:devfrom
DeathGun44 wants to merge 1 commit intoopenMF:devfrom
Conversation
|
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:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Gitignore ./.gitignore |
Added /playwright/.auth/ to ignore persisted Playwright auth artifacts. |
Playwright config playwright.config.ts |
Made use.baseURL environment-configurable (`process.env.E2E_BASE_URL |
Auth setup script playwright/auth.setup.ts |
New global setup setup('authenticate', ...): ensures playwright/.auth exists, clears prior artifacts, reads E2E_USERNAME/E2E_PASSWORD, performs UI login, copies mifosXCredentials from sessionStorage to localStorage, writes storageState to playwright/.auth/user.json, verifies persisted auth. |
Authenticated tests playwright/tests/authenticated-smoke.spec.ts |
New smoke test that skips if playwright/.auth/auth-failed exists; otherwise navigates to /#/, asserts no login redirect and that mat-toolbar is visible. |
Login tests adjustments playwright/tests/login.spec.ts, playwright/tests/login-responsive.spec.ts |
Set file-level test.use({ storageState: { cookies: [], origins: [] } }) for clean state; switched absolute URLs to hash routes (/#/login); removed unconditional waits and improved timeout-based assertions; removed BOM from a file. |
Sequence Diagram(s)
sequenceDiagram
participant Config as Playwright Config
participant Setup as Setup Project
participant Auth as auth.setup.ts
participant Browser as Browser Context
participant Storage as Auth Storage
participant Chromium as Chromium Project
participant Tests as Test Specs
Config->>Setup: run setup project (*.setup.ts)
Setup->>Auth: execute authenticate routine
Auth->>Browser: navigate to /#/login and submit credentials (from env)
Browser->>Auth: render authenticated UI
Auth->>Browser: copy mifosXCredentials sessionStorage -> localStorage
Auth->>Storage: save browserContext.storageState(path: playwright/.auth/user.json)
Auth->>Browser: create verification context using saved state and verify auth
Config->>Chromium: run chromium project (depends on setup)
Chromium->>Browser: launch with storageState from Storage
Tests->>Browser: run authenticated tests against app
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- WEB-891: Add Express proxy server for Playwright CI pipeline #3438: Modifies Playwright config and test files; touches similar config and test-skip behavior.
- establish Playwright E2E testing infrastructure with configuration #2912: Earlier Playwright infra changes (config, .gitignore, setup) related to persisted auth and test setup.
Suggested reviewers
- alberto-art3ch
- IOhacker
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and specifically summarizes the main change: implementing Playwright authentication state persistence via storageState, which is the central purpose of the PR. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
playwright/tests/login-responsive.spec.ts (1)
212-216:⚠️ Potential issue | 🟡 MinorHardcoded URL bypasses configurable baseURL.
This test uses a hardcoded
http://localhost:4200/logininstead of a relative path, breaking compatibility withE2E_BASE_URLconfiguration added inplaywright.config.ts.🔧 Proposed fix
for (const viewport of viewports) { await page.setViewportSize(viewport); - await page.goto('http://localhost:4200/login'); + await page.goto('/#/login'); await page.waitForLoadState('networkidle');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/tests/login-responsive.spec.ts` around lines 212 - 216, The test is using a hardcoded URL in the viewport loop (see the loop using viewports and the page.goto call) which bypasses the configurable baseURL (E2E_BASE_URL in playwright.config.ts); replace the hardcoded 'http://localhost:4200/login' with a relative path so Playwright resolves it from the configured baseURL (e.g., use '/login' with the existing page.goto call), or if you must read the env var, derive base from E2E_BASE_URL and concatenate the path before calling page.goto; update the page.goto invocation in the for (const viewport of viewports) loop accordingly.
🧹 Nitpick comments (3)
playwright/auth.setup.ts (1)
59-63: Add timeout to verification assertion to avoid indefinite hang.The verification assertion at line 62 lacks a timeout, unlike the similar assertions earlier (lines 36-37). If the verification navigation stalls, this could hang indefinitely.
⏱️ Proposed fix
const verifyContext = await browser.newContext({ storageState: authFile }); const verifyPage = await verifyContext.newPage(); await verifyPage.goto('/#/'); - await expect(verifyPage).not.toHaveURL(/.*login.*/); + await expect(verifyPage).not.toHaveURL(/.*login.*/, { timeout: 30000 }); await verifyContext.close();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/auth.setup.ts` around lines 59 - 63, The verification can hang because the expect check on verifyPage has no timeout; update the expect call on verifyPage (the expect(verifyPage).not.toHaveURL(/.*login.*/)) to include a reasonable timeout option (e.g., { timeout: 5000 }) so the test fails fast if navigation stalls, keeping the rest of the flow (verifyContext.close()) unchanged.playwright/tests/login.spec.ts (1)
83-97: Replace brittlewaitForTimeoutwith condition-based wait.
page.waitForTimeout(3000)introduces timing flakiness. Wait for a specific condition like the snackbar/error message visibility or URL stability instead.As per coding guidelines for
**/*.spec.ts: "encourage... minimal brittle timing dependencies."⏱️ Proposed fix - wait for snackbar or URL stability
// Attempt login with wrong password await loginPage.login('mifos', 'wrongpassword'); - // Wait for the login attempt to process and any error notification to appear - // The app shows a snackbar notification for authentication errors - await page.waitForTimeout(3000); + // Wait for snackbar notification to appear (or URL to remain on login) + await expect(page.locator('simple-snack-bar, mat-snack-bar-container')).toBeVisible({ timeout: 10000 }).catch(() => { + // Snackbar may not appear in all error scenarios; URL check suffices + }); // Should remain on login page after failed attempt (URL still contains /login) await expect(page).toHaveURL(/.*login.*/);Alternatively, if snackbar selectors vary, simply rely on the URL assertion with an adequate timeout:
- await page.waitForTimeout(3000); - - // Should remain on login page after failed attempt (URL still contains /login) - await expect(page).toHaveURL(/.*login.*/); + // Should remain on login page after failed attempt + await expect(page).toHaveURL(/.*login.*/, { timeout: 10000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/tests/login.spec.ts` around lines 83 - 97, Replace the brittle page.waitForTimeout(3000) in the "should handle invalid credentials gracefully" test by awaiting a specific condition: either wait for the app's error notification/snackbar to appear (e.g., await expect(loginPage.errorSnackbar).toBeVisible()) after calling loginPage.login('mifos', 'wrongpassword'), or rely on URL stability with a timeout by using await expect(page).toHaveURL(/.*login.*/, { timeout: 5000 }); then proceed with the existing assertions (usernameInput/passwordInput visibility); remove page.waitForTimeout entirely and use the targeted expect-based wait instead.playwright/tests/authenticated-smoke.spec.ts (1)
10-13: Consider adding a timeout to prevent indefinite waits.While the test correctly skips in CI, when running locally against a slow backend, the default assertion timeouts may not be sufficient. Consider explicit timeouts for consistency with
auth.setup.ts.⏱️ Optional enhancement
test('should load dashboard without login redirect', async ({ page }) => { await page.goto('/#/'); - await expect(page).not.toHaveURL(/.*login.*/); - await expect(page.locator('mat-toolbar')).toBeVisible(); + await expect(page).not.toHaveURL(/.*login.*/, { timeout: 30000 }); + await expect(page.locator('mat-toolbar')).toBeVisible({ timeout: 10000 }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/tests/authenticated-smoke.spec.ts` around lines 10 - 13, The suite 'Authenticated Smoke Tests' currently skips in CI but lacks an explicit timeout, so add a consistent test timeout (matching auth.setup.ts) to avoid indefinite waits against slow backends: call test.setTimeout(...) at the top of the file (before test.describe) or set a timeout option on the individual tests in the 'Authenticated Smoke Tests' describe block so that the suite or tests honor the same timeout value used in auth.setup.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@playwright/tests/login-responsive.spec.ts`:
- Around line 212-216: The test is using a hardcoded URL in the viewport loop
(see the loop using viewports and the page.goto call) which bypasses the
configurable baseURL (E2E_BASE_URL in playwright.config.ts); replace the
hardcoded 'http://localhost:4200/login' with a relative path so Playwright
resolves it from the configured baseURL (e.g., use '/login' with the existing
page.goto call), or if you must read the env var, derive base from E2E_BASE_URL
and concatenate the path before calling page.goto; update the page.goto
invocation in the for (const viewport of viewports) loop accordingly.
---
Nitpick comments:
In `@playwright/auth.setup.ts`:
- Around line 59-63: The verification can hang because the expect check on
verifyPage has no timeout; update the expect call on verifyPage (the
expect(verifyPage).not.toHaveURL(/.*login.*/)) to include a reasonable timeout
option (e.g., { timeout: 5000 }) so the test fails fast if navigation stalls,
keeping the rest of the flow (verifyContext.close()) unchanged.
In `@playwright/tests/authenticated-smoke.spec.ts`:
- Around line 10-13: The suite 'Authenticated Smoke Tests' currently skips in CI
but lacks an explicit timeout, so add a consistent test timeout (matching
auth.setup.ts) to avoid indefinite waits against slow backends: call
test.setTimeout(...) at the top of the file (before test.describe) or set a
timeout option on the individual tests in the 'Authenticated Smoke Tests'
describe block so that the suite or tests honor the same timeout value used in
auth.setup.ts.
In `@playwright/tests/login.spec.ts`:
- Around line 83-97: Replace the brittle page.waitForTimeout(3000) in the
"should handle invalid credentials gracefully" test by awaiting a specific
condition: either wait for the app's error notification/snackbar to appear
(e.g., await expect(loginPage.errorSnackbar).toBeVisible()) after calling
loginPage.login('mifos', 'wrongpassword'), or rely on URL stability with a
timeout by using await expect(page).toHaveURL(/.*login.*/, { timeout: 5000 });
then proceed with the existing assertions (usernameInput/passwordInput
visibility); remove page.waitForTimeout entirely and use the targeted
expect-based wait instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4ace1d1-160f-46fb-9abe-b7feb4ae4a45
📒 Files selected for processing (6)
.gitignoreplaywright.config.tsplaywright/auth.setup.tsplaywright/tests/authenticated-smoke.spec.tsplaywright/tests/login-responsive.spec.tsplaywright/tests/login.spec.ts
DeathGun44
force-pushed
the
WEB-890/playwright-auth-bypass
branch
from
358e26c to
5f7a618
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
playwright/tests/login-responsive.spec.ts (1)
214-214: Good: Using relative hash-based route instead of hardcoded URL.This correctly leverages the
baseURLfromplaywright.config.ts. However, other tests in this file useloginPage.navigate()(viabeforeEach). Consider using the same approach here for consistency:♻️ Optional: Use LoginPage for navigation consistency
for (const viewport of viewports) { await page.setViewportSize(viewport); - await page.goto('/#/login'); + await loginPage.navigate(); await page.waitForLoadState('networkidle');This would align with the pattern used in other tests and ensure the URL is sourced from a single location (
LoginPage.url).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/tests/login-responsive.spec.ts` at line 214, Test uses page.goto('/#/login') directly instead of the existing page object helper; replace the direct navigation with the LoginPage helper by calling loginPage.navigate() (or reference LoginPage.url if needed) so navigation is consistent with other tests that use loginPage.navigate() in beforeEach; update the test to use the loginPage.navigate() method and remove the page.goto call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@playwright/tests/login-responsive.spec.ts`:
- Line 214: Test uses page.goto('/#/login') directly instead of the existing
page object helper; replace the direct navigation with the LoginPage helper by
calling loginPage.navigate() (or reference LoginPage.url if needed) so
navigation is consistent with other tests that use loginPage.navigate() in
beforeEach; update the test to use the loginPage.navigate() method and remove
the page.goto call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de922a20-d6ef-4a2f-a469-19dc4d993cad
📒 Files selected for processing (6)
.gitignoreplaywright.config.tsplaywright/auth.setup.tsplaywright/tests/authenticated-smoke.spec.tsplaywright/tests/login-responsive.spec.tsplaywright/tests/login.spec.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (3)
- playwright/tests/authenticated-smoke.spec.ts
- playwright.config.ts
- playwright/auth.setup.ts
DeathGun44
force-pushed
the
WEB-890/playwright-auth-bypass
branch
from
5f7a618 to
0e4425b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
playwright/tests/login-responsive.spec.ts (1)
212-215: Consider using the page object for consistency and stability.This test uses direct
page.goto()andnetworkidlewait while other tests in this file useloginPage.navigate()andloginPage.assertOnLoginPage(). Thenetworkidlewait can be flaky in CI environments.♻️ Suggested refactor for consistency
for (const viewport of viewports) { await page.setViewportSize(viewport); - await page.goto('/#/login'); - await page.waitForLoadState('networkidle'); + await loginPage.navigate(); + await loginPage.assertOnLoginPage(); // Check no horizontal scrollAs per coding guidelines: "For tests: encourage clear Arrange-Act-Assert structure, stable selectors, and minimal brittle timing dependencies."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/tests/login-responsive.spec.ts` around lines 212 - 215, Replace the direct navigation and networkidle waiting in the viewport loop with the page object helpers for consistency and stability: instead of calling page.goto('/#/login') and page.waitForLoadState('networkidle') inside the loop that uses page.setViewportSize(viewport), call loginPage.navigate() after setting the viewport and then loginPage.assertOnLoginPage() to wait for a stable page state; update any assertions that relied on networkidle to use the page object's stable selectors/waits so tests follow the Arrange-Act-Assert pattern and avoid flaky networkidle waits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@playwright/tests/login-responsive.spec.ts`:
- Around line 212-215: Replace the direct navigation and networkidle waiting in
the viewport loop with the page object helpers for consistency and stability:
instead of calling page.goto('/#/login') and
page.waitForLoadState('networkidle') inside the loop that uses
page.setViewportSize(viewport), call loginPage.navigate() after setting the
viewport and then loginPage.assertOnLoginPage() to wait for a stable page state;
update any assertions that relied on networkidle to use the page object's stable
selectors/waits so tests follow the Arrange-Act-Assert pattern and avoid flaky
networkidle waits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ffb80115-67ce-451b-ad58-a724f076b29d
📒 Files selected for processing (6)
.gitignoreplaywright.config.tsplaywright/auth.setup.tsplaywright/tests/authenticated-smoke.spec.tsplaywright/tests/login-responsive.spec.tsplaywright/tests/login.spec.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- playwright/tests/login.spec.ts
- playwright/tests/authenticated-smoke.spec.ts
- playwright.config.ts
- playwright/auth.setup.ts
DeathGun44
force-pushed
the
WEB-890/playwright-auth-bypass
branch
from
0e4425b to
03b5bf5
Compare
Contributor
Author
|
The failed check depends on #3459 ,i will rebase this pr when that one is approved and merged! |
DeathGun44
force-pushed
the
WEB-890/playwright-auth-bypass
branch
from
03b5bf5 to
5a22264
Compare
Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
DeathGun44
force-pushed
the
WEB-890/playwright-auth-bypass
branch
from
5a22264 to
f14a86d
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
1 participant
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.
Description
Every Playwright E2E test currently goes through the full UI login flow, which is slow and fragile in CI. This PR adds a one-time auth setup that logs in once, saves the browser state, and reuses it across all tests — so they start pre-authenticated instantly.
Changes:
playwright/auth.setup.ts— New setup script that logs in once and saves auth state. Falls back gracefully when no backend is available.playwright.config.ts— Added setup project so auth runs before all tests. Made base URL configurable via env var.login.spec.ts&login-responsive.spec.ts— Wipe saved auth so login page tests still work normally.authenticated-smoke.spec.ts— New test proving the bypass works (dashboard loads without redirect to login)..gitignore— Exclude saved auth state files.Related issues and discussion
#WEB-890
Screenshots, if any
N/A — no UI changes.
Checklist
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
Chores
Tests