ci: add playwright e2e testing infrastructure #129
Conversation
📝 WalkthroughWalkthroughThis PR establishes a comprehensive end-to-end testing infrastructure for a Frappe-based UI using Playwright. It includes a GitHub Actions CI/CD workflow, Playwright configuration, authentication and API helper utilities, page object models, and initial test suites covering login flows and session persistence. Changes
Sequence Diagram(s)sequenceDiagram
participant Playwright as Playwright Test<br/>(auth.setup.js)
participant API as Frappe API<br/>(/api/method/login)
participant AppPage as App Page<br/>(to capture CSRF)
participant Storage as Local Storage<br/>(e2e/.auth/)
Playwright->>API: loginViaAPI(credentials)
API-->>Playwright: Session Cookie
Playwright->>API: isLoggedIn() check
API-->>Playwright: User confirmation
Playwright->>AppPage: Navigate to /app
AppPage-->>Playwright: Page loaded with CSRF token
Playwright->>Storage: Save CSRF token to csrf.json
Playwright->>Storage: Save storageState to user.json
Storage-->>Playwright: Persistence complete
Note over Storage: Reusable by subsequent tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In `@dashboard/frappe-ui`:
- Line 1: Update the commit message or PR description to explicitly document the
addition of the dashboard/frappe-ui submodule (commit e239181): state why this
specific commit was chosen (e.g., needed bugfixes or feature set present in
e239181), list the UI components or features from frappe-ui that the dashboard
and E2E tests depend on (identify components by name used in the repo), and
confirm compatibility with the dashboard package.json constraint frappe-ui:
^0.1.254 (state whether e239181 satisfies that semver range or note any
overrides). Include a brief note that the submodule is required for the E2E
testing infrastructure and reference the submodule path dashboard/frappe-ui and
commit hash e239181.
In `@package.json`:
- Around line 27-30: The devDependencies entry pins "@types/node" to a
non-existent 25.0.8; update the package.json devDependencies to use a valid 24.x
version (e.g., change "@types/node": "^25.0.8" to "@types/node": "^24.3.1") so
npm install succeeds; keep "@playwright/test" as-is.
🧹 Nitpick comments (2)
e2e/tests/login.spec.js (2)
6-7: Hardcoded default credentials in test file.While these are test/development credentials and match the workflow's site setup (
admin-password admin), having hardcoded passwords in source code is generally discouraged. Consider documenting that these defaults are only for local development or CI environments.
15-19: Consider adding an assertion on the logged-in user.The test retrieves
frappe.session.userbut only logs it without asserting. Adding an assertion would make the test more robust by verifying the actual authenticated user matches expectations.💡 Suggested improvement
await expect(page).not.toHaveURL(/.*login.*/); await page.waitForLoadState("networkidle"); const loggedInUser = await page.evaluate(() => frappe.session.user); - console.log(`Verified Session User: ${loggedInUser}`); + console.log(`Verified Session User: ${loggedInUser}`); + expect(loggedInUser).toBe(username);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
.github/workflows/ui-tests.yml.gitignoredashboard/frappe-uie2e/tests/login.spec.jspackage.jsonplaywright.config.js
⏰ 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: Playwright E2E Tests
- GitHub Check: Server
🔇 Additional comments (7)
.gitignore (1)
60-66: LGTM!The Playwright-related ignore patterns correctly cover standard artifact directories (
test-results/,playwright-report/,blob-report/) and internal cache/auth directories. These align well with Playwright's default output locations.package.json (1)
10-14: LGTM!Good selection of test scripts covering various execution modes: headless (
test:e2e), interactive UI (test:e2e:ui), headed browser (test:e2e:headed), and debug mode (test:e2e:debug).playwright.config.js (2)
15-41: LGTM!Well-structured Playwright configuration with sensible defaults:
- CI-aware settings for
forbidOnly,retries, andworkers- Appropriate timeouts for actions (15s) and navigation (30s)
- Good debugging aids with trace/video/screenshot on failure
baseURLcorrectly defaults to match the workflow environment
44-79: Consider enabling additional browsers in the future.Starting with Chromium-only is a reasonable approach for initial E2E setup. The commented-out configurations for Firefox, WebKit, and mobile viewports provide a good template for future cross-browser testing expansion.
.github/workflows/ui-tests.yml (3)
113-126: Good approach for starting the Frappe server in CI.The workflow correctly:
- Disables
watchandscheduleprocesses to reduce resource usage- Runs bench in the background with output redirected to a log file
- Uses a timeout-wrapped curl loop to wait for server readiness
140-164: LGTM!Excellent debugging setup:
- Playwright report uploaded on all runs (
if: always())- Test results uploaded only on failure to save storage
- Bench and Frappe logs printed on failure for troubleshooting
41-50: The workflow configuration is correct. Both Python 3.14 and Node.js 24 are valid, stable versions as of January 2026:
- Python 3.14.2 was released on December 5, 2025, and is the current stable version.
- Node.js v24.13.0 is the current LTS release as of January 2026.
No changes are needed.
Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
imgullu786
force-pushed
the
feat/tests
branch
from
d298186 to
a69cb70
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/tests/login.spec.js`:
- Around line 15-19: The test currently reads frappe.session.user into
loggedInUser and only logs it; replace or supplement that log with an assertion
that validates loggedInUser equals the expected test account (e.g., the known
username used by the test or an env/fixture value), using the test framework's
expect to fail the test on mismatch; locate the page.evaluate call that reads
frappe.session.user and add an assertion like
expect(loggedInUser).toBe(expectedUser) (or the appropriate matcher) and ensure
expectedUser is sourced from the test's existing credentials/fixture or an env
var.
🧹 Nitpick comments (2)
e2e/tests/login.spec.js (1)
9-13: Consider removing or reducing debug logging.The
console.logon line 12 may be a debug artifact. If intentional for CI visibility, consider using Playwright's built-in tracing or test annotations instead for cleaner test output..github/workflows/ui-tests.yml (1)
84-87: Minor: Use consistent apt commands.Mixing
apt updatewithapt-get installworks but is inconsistent.🧹 Proposed fix for consistency
- name: Install MariaDB Client run: | - sudo apt update - sudo apt-get install mariadb-client + sudo apt-get update + sudo apt-get install -y mariadb-client
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ui-tests.ymle2e/tests/login.spec.js
⏰ 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: Server
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (7)
e2e/tests/login.spec.js (1)
1-7: LGTM on test setup and credential handling.Environment variable fallbacks for credentials are appropriate for CI/local flexibility.
.github/workflows/ui-tests.yml (6)
1-12: LGTM on workflow triggers and concurrency.Good use of concurrency grouping with
cancel-in-progress: trueto avoid wasted CI resources on superseded commits.
20-35: LGTM on service configuration.Redis and MariaDB services are properly configured with health checks for MariaDB, ensuring the database is ready before tests proceed.
96-106: LGTM on app installation order.Installing
paymentsbeforebuzzaligns with the dependency requirements mentioned in the commit message.
114-127: LGTM on server startup with timeout-based readiness check.The approach of disabling watch/schedule to reduce resource usage and using a timeout-based curl check for server readiness is solid for CI environments.
141-155: LGTM on artifact handling.Uploading the report unconditionally (
if: always()) and test results only on failure is a practical balance between debugging capability and storage efficiency.
41-50: No action required. Python 3.14 and Node.js 24 are stable versions (released October 2025 and May 2025 respectively) and are officially recommended by Frappe v16/develop. These are the intended dependency versions for current Frappe development, not a compatibility risk.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
imgullu786
force-pushed
the
feat/tests
branch
from
a69cb70 to
0b7be35
Compare
|
Check hazelnode for how Logins are handled. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/.auth/user.json`:
- Around line 1-95: The committed user.json contains real session/localStorage
(including the sid cookie and user identifiers); remove sensitive runtime state
and replace user.json with a sanitized placeholder fixture, update the test
bootstrap (auth.setup) to generate the required cookies/localStorage at runtime
instead of relying on committed state, and ensure generated auth JSON files are
ignored by adding the auth JSON pattern to .gitignore so e2e auth artifacts are
not committed.
🧹 Nitpick comments (5)
e2e/helpers/auth.js (1)
23-33: Keep UI login selectors in sync with the LoginPage object.
loginViaUIusesdata-fieldnameselectors whilee2e/pages/login.page.jsuses#login_email/#login_password; consider delegating toLoginPage(or shared selectors) to avoid drift and please verify the current DOM exposes both. As per coding guidelines, use absolute imports.e2e/helpers/index.js (1)
1-2: Switch re-exports to absolute paths to match import policy.
Using relative paths here conflicts with the absolute-import guideline; please confirm the resolver supportse2e/*and then swap to absolute paths. As per coding guidelines, use absolute imports.♻️ Proposed refactor
-export * from "./auth"; -export * from "./frappe"; +export * from "e2e/helpers/auth"; +export * from "e2e/helpers/frappe";e2e/tests/auth.setup.js (1)
37-49: CSRF token extraction could be more robust with an explicit wait.The comment mentions "Wait for frappe to be defined," but the code only uses optional chaining without actually waiting. If
frappeinitializes afternetworkidle, the token won't be captured. Consider adding a retry or explicit wait:♻️ Suggested improvement
- // Wait for frappe to initialize and extract CSRF token - const csrfToken = await page.evaluate(() => { - // Wait for frappe to be defined - return window.frappe?.csrf_token; - }); + // Wait for frappe to initialize and extract CSRF token + const csrfToken = await page.evaluate(async () => { + // Wait for frappe to be defined (up to 5 seconds) + for (let i = 0; i < 50; i++) { + if (window.frappe?.csrf_token) { + return window.frappe.csrf_token; + } + await new Promise((r) => setTimeout(r, 100)); + } + return null; + });e2e/tests/auth.spec.js (2)
33-40: Consider using environment variables for test credentials.The setup file (
auth.setup.js) usesprocess.env.FRAPPE_USER || "Administrator"for credentials, but this test hardcodes them. For consistency and flexibility across environments:♻️ Suggested change
test("should login via UI", async ({ page }) => { const loginPage = new LoginPage(page); - await loginPage.login("Administrator", "admin"); + await loginPage.login( + process.env.FRAPPE_USER || "Administrator", + process.env.FRAPPE_PASSWORD || "admin" + ); // Should be redirected away from login await expect(page).not.toHaveURL(/.*login.*/); });
42-51: Consider also asserting the error message is displayed.The
LoginPageclass providesexpectLoginError()which verifies the error message is visible. Adding this assertion would make the test more thorough:♻️ Suggested addition
test("should show error for invalid credentials", async ({ page }) => { const loginPage = new LoginPage(page); await loginPage.goto(); await loginPage.fillCredentials("invalid@example.com", "wrongpassword"); await loginPage.submit(); + // Should show error message + await loginPage.expectLoginError(); + // Should stay on login page await loginPage.expectToBeOnLoginPage(); });
imgullu786
force-pushed
the
feat/tests
branch
from
f2b70b5 to
a63a587
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/helpers/frappe.js`:
- Around line 170-176: The docExists function currently catches all errors which
can hide auth/network issues; change it to only return false when the error
indicates a 404/not-found from getDoc and rethrow any other errors. Locate
docExists and the call to getDoc(request, doctype, name), inspect the thrown
error (e.g., error.response?.status === 404 or a known NotFoundError
class/message) and return false in that case, otherwise throw the original error
so auth/network failures bubble up.
🧹 Nitpick comments (3)
e2e/pages/login.page.js (1)
43-48: Consider env-driven defaults for credentials.
Hardcoding admin defaults can make CI/environment setup brittle. Prefer env overrides with safe fallbacks.♻️ Proposed adjustment
- async login(email = "Administrator", password = "admin") { + async login( + email = process.env.E2E_USER ?? "Administrator", + password = process.env.E2E_PASSWORD ?? "admin" + ) {e2e/helpers/frappe.js (2)
14-33: Optional: avoid noisy console warnings.
Consider throwing (or using a test logger) instead ofconsole.warnso CSRF file issues fail fast and are easier to trace.
140-165: Handlelimit = 0explicitly.
The current truthy check skips0, which may be a valid limit.♻️ Proposed fix
- if (options.limit) { + if (options.limit !== undefined && options.limit !== null) { params.set("limit_page_length", options.limit.toString()); }
| export async function docExists(request, doctype, name) { | ||
| try { | ||
| await getDoc(request, doctype, name); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Don’t swallow non-404 failures in docExists.
Catching all errors can hide auth or network regressions. Consider only returning false on a known “not found” case and rethrow otherwise.
💡 Safer pattern
export async function docExists(request, doctype, name) {
try {
await getDoc(request, doctype, name);
return true;
- } catch {
- return false;
+ } catch (error) {
+ if (error?.message?.includes("404")) return false;
+ throw error;
}
}🤖 Prompt for AI Agents
In `@e2e/helpers/frappe.js` around lines 170 - 176, The docExists function
currently catches all errors which can hide auth/network issues; change it to
only return false when the error indicates a 404/not-found from getDoc and
rethrow any other errors. Locate docExists and the call to getDoc(request,
doctype, name), inspect the thrown error (e.g., error.response?.status === 404
or a known NotFoundError class/message) and return false in that case, otherwise
throw the original error so auth/network failures bubble up.
2 participants
Description
This PR introduces the End-to-End testing infrastructure using Playwright.
Changes
Summary by CodeRabbit
Release Notes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.