test(e2e): repair the Playwright suite after the PMS 2.0 has updated - #2200
Closed
renishsurani wants to merge 12 commits into
Closed
renishsurani wants to merge 12 commits into
renishsurani wants to merge 12 commits into
Conversation
The root package.json declares "type": "module", which makes every .js file under tests/ an ES module. Playwright's config loader cannot read the suite that way and fails before a single test runs, at loadConfigFromFile. This file has existed locally since the suite was written but was never committed, so the suite only loads on a machine that already happens to have it -- a fresh clone and CI both get the loader error. Verified by moving it aside: `--list` reports 89 tests with it present, and fails to load the config without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The redesign replaced the HTML tables the suite addressed with flex
containers, moved the filter panel to a query builder, and dropped several
controls outright. Most page objects were addressing a DOM that no longer
exists.
Selectors and page objects
- Team grid rows are flex containers with no table/row/cell roles; address
cells by the row's direct children and depth by the pl-7.5 / pl-13.5 /
pl-19.5 classes rather than by a parent table.
- Transparent row overlays swallow clicks, so dispatch the event instead of
forcing it -- force:true fires the overlay too.
- Project column headers wrap a "<name> column options" button, so their
accessible name reads "Project name Project name column options". The
anchored ^...$ matcher could never match; use a word boundary.
- Rebuild the toast, reject-timesheet and review-pane locators against the
redesigned markup.
Seeding and data
- Wait for the project rollup before reading billing totals: project totals
are recalculated asynchronously after a timesheet saves, so seven billing
tests were reading pre-write values.
- Give the timesheet-review tests their own employee instead of sharing one
account, and register those reviewees in TC53's expected roster after the
seeding loop -- TC53's own turn rewrites its stub, so anything registered
before it is wiped.
- Rename the seeded customer to match staging, and add Aishwarrya Pande to
TC53's roster after a real reporting-line change.
Cleanup
- Every delete went through DELETE /api/resource/..., whose failures each
caller caught and logged, so nothing the suite created was reliably
removed. Route them through one deleteDocument() that reports
{ deleted, reason }, clear dependants first (cancel a submitted timesheet,
then delete it, then the employee), and sweep views, tasks and the
timesheets booked onto shared accounts. The timesheet sweep is bounded by a
run marker read from the server's own clock, since Frappe stamps `creation`
in the server timezone and a UTC cutoff opened the window 5.5 hours early.
- Teardown could not tell "nothing to clean" from "could not reach anything";
both printed a green summary. Count lookups separately from matches.
Auth
- Worker storage states were reused whenever the file existed, never checked.
The stored `sid` expires after ~12h and Chromium drops an expired cookie
while restoring the state, so the context kept every cookie except `sid`,
the first navigation went out as Guest, and the app redirected to /login --
surfacing as a 30s timeout on an unrelated locator. Re-login when the
stored session is expired or absent.
Tests for controls the redesign removed (Columns picker, Combine Week Hours,
user-group filter, sheet-view switcher) are skipped with the reason recorded,
pending a retire-or-repoint decision rather than being rewritten to match the
new behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The suite's failure modes are not visible from the code in front of you -- test-ID-driven seeding that ignores --grep, worker auth cached with no validity check, slowMo against a 30s default timeout -- so reasoning from first principles reliably produces a wrong theory. The skill carries the architecture map, the post-redesign DOM contract, a symptom-to-cause playbook, and three scripts: probe.mjs (inspect the live app in ~1s instead of paying 5-7 minutes of seeding), results.py (parse results.json) and cleanup.mjs. Committed alongside the sibling project skills rather than left local: the README lists it under "authored in-repo", and CLAUDE.md now tells every e2e task to load it first, so it has to travel with the repo. Also ignore /.agents, which holds local agent state. /.claude is deliberately not ignored -- it contains only skills/, all of which are tracked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses the Copilot review on #2186. deleteDocument() was changed to resolve with { deleted, reason } rather than throw, but the eleven existing call sites still only used try/catch. A refusal therefore stopped reaching them -- reintroducing, in a new shape, the silent cleanup failure this branch set out to remove. cleanUpProjects was the worst case: `failures` only ever filled from a catch block, so its summary printed "deleted all N seeded project(s)" while the records were still on staging. - deleteDocument warns at the single point of failure, so no caller can drop a refusal on the floor. - cleanUpProjects records a refused timesheet/task/allocation/project delete as a failure, so the summary reflects what actually went. - deleteTasks, deleteProjects, deleteAllocationsByEmployee, the allocation teardown, deleteLeaveOfEmployee and deleteUserGroupForEmployee check `deleted` instead of logging success unconditionally. Two further problems the review surfaced: - Teardown deleted UI-created tasks before this run's timesheets. Frappe refuses to delete anything a timesheet still points at, so those tasks failed and nothing retried them. The timesheet sweep now runs first. - createAllocationsForTestCases caught a failed allocation creation and continued, letting setup finish with missing fixture data; the test then failed on a selector, which reads as a broken test rather than a seeding gap. It now aborts. Also fixes a ReferenceError found while checking that call site: the allocation teardown logged an undeclared `allocationID`, which threw after a successful delete and was caught and reported as a deletion failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixes the two failing checks on #2186. Frappe Linter: - JS/TS Formatter: ran prettier@4.0.0-alpha.8 with CI's defaults (80 columns) over every .js/.cjs/.json file this PR touches. Whole-file reformatting, so the diff is large but carries no behaviour change. - Python linter: UP028 in the e2e skill's results.py -- a `for` loop that only yielded is now `yield from`. The `Sempgrep: frappe` findings in the same job are the pre-existing backend security items CLAUDE.md lists as out of scope, and are untouched here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous formatting pass used prettier without --no-editorconfig. The root .editorconfig sets max_line_length = 120 for JS, which prettier reads by default and applies as printWidth, so the files came out at 120 columns while the pre-commit hook formats at prettier's 80-column default. Two page objects and tests/package.json were left differing. Also runs ruff-format over the e2e skill's results.py, which the Python formatter hook reported separately from the UP028 lint finding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Frappe Linter's `Sempgrep: frappe` hook flagged results.py as blocking:
frappe-security-file-traversal
76| with open(args.path) as fh:
.semgrepignore already excludes tests/, which is why this PR's JS is not
scanned, but not .claude/ -- so the e2e skill's scripts are. The rule exists to
catch traversal in request-handling code; this is a local CLI reading the
results file the operator names on the command line, with no untrusted input.
Suppressed on the line with that rationale rather than widening .semgrepignore,
which would also hide any future finding under .claude/.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous attempt placed `# nosemgrep` three lines above the finding with the rationale in between, and semgrep only honours the marker on the finding's own line or the one directly above it -- so the hook stayed red on the same `open(args.path)` call. Moved inline; the explanation now sits above it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Retry and cleanup paths can duplicate writes, conceal API failures, leave linked records behind, and exercise saved-view tests against invalid fixtures.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (8)
What changed in this PR
Restores the Playwright E2E suite for the redesigned PMS UI and strengthens authentication, fixture seeding, retries, and cleanup.
Changes:
- Updates page objects, selectors, test cases, and fixture data for PMS 2.0.
- Centralizes API cleanup, authentication validation, retries, and dependency deletion.
- Adds E2E debugging documentation and utilities.
| File | Description |
|---|---|
.claude/skills/next-pms-e2e/SKILL.md |
Adds E2E guidance. |
.claude/skills/next-pms-e2e/references/probing.md |
Documents live-app probing. |
.claude/skills/next-pms-e2e/references/selectors.md |
Documents redesigned selectors. |
.claude/skills/next-pms-e2e/references/traps.md |
Records common failure modes. |
.claude/skills/next-pms-e2e/scripts/cleanup.mjs |
Adds standalone cleanup utility. |
.claude/skills/next-pms-e2e/scripts/probe.mjs |
Adds authenticated probing utility. |
.claude/skills/next-pms-e2e/scripts/results.py |
Adds result summarization utility. |
.gitignore |
Ignores generated E2E artifacts. |
CLAUDE.md |
Registers project E2E guidance. |
tests/e2e/data/employee/timesheet.js |
Updates timesheet fixtures. |
tests/e2e/data/manager/project.js |
Updates project and view fixtures. |
tests/e2e/data/manager/task.js |
Updates task fixtures. |
tests/e2e/data/manager/team.js |
Adds dedicated review fixtures. |
tests/e2e/globals/globalSetup.js |
Reorders and expands fixture seeding. |
tests/e2e/globals/globalTeardown.js |
Adds dependency-aware cleanup. |
tests/e2e/helpers/employeeHelper.js |
Cleans employee dependencies. |
tests/e2e/helpers/leaveHelper.js |
Updates leave cleanup. |
tests/e2e/helpers/resourceManagementHelpers.js |
Improves allocation seeding. |
tests/e2e/helpers/teamTabHelper.js |
Updates user-group cleanup. |
tests/e2e/helpers/timesheetHelper.js |
Expands project, timesheet, and view lifecycle handling. |
tests/e2e/pageObjects/projectPage.js |
Rebuilds project-page interactions. |
tests/e2e/pageObjects/resourceManagement/project.js |
Updates project allocation selectors. |
tests/e2e/pageObjects/resourceManagement/team.js |
Updates team allocation selectors. |
tests/e2e/pageObjects/resourceManagement/timeline.js |
Updates allocation timeline interactions. |
tests/e2e/pageObjects/sidebar.js |
Updates sidebar navigation. |
tests/e2e/pageObjects/taskPage.js |
Updates task-grid interactions. |
tests/e2e/pageObjects/teamPage.js |
Rebuilds team-grid and review interactions. |
tests/e2e/pageObjects/timesheetPage.js |
Updates timesheet interactions. |
tests/e2e/playwright.fixture.cjs |
Validates cached worker sessions. |
tests/e2e/specs/employee/timesheet.spec.js |
Updates employee timesheet tests. |
tests/e2e/specs/employee3/timesheet3.spec.js |
Updates third-employee tests. |
tests/e2e/specs/manager/project.spec.js |
Updates project-list coverage. |
tests/e2e/specs/manager/resourceManagement.spec.js |
Updates allocation coverage. |
tests/e2e/specs/manager/resourceManagement/project.spec.js |
Updates project allocation coverage. |
tests/e2e/specs/manager/task.spec.js |
Updates task tests. |
tests/e2e/specs/manager/team.spec.js |
Updates team and review tests. |
tests/e2e/utils/api/apiClient.js |
Adds shared API and cleanup primitives. |
tests/e2e/utils/api/employeeRequests.js |
Adopts centralized API behavior. |
tests/e2e/utils/api/erpNextRequests.js |
Updates ERPNext request handling. |
tests/e2e/utils/api/frappeRequests.js |
Updates Frappe request handling. |
tests/e2e/utils/api/leaveRequests.js |
Centralizes leave deletion. |
tests/e2e/utils/api/projectRequests.js |
Centralizes project and allocation deletion. |
tests/e2e/utils/api/resourceManagementRequests.js |
Centralizes allocation requests. |
tests/e2e/utils/api/taskRequests.js |
Centralizes task deletion. |
tests/e2e/utils/api/timesheetRequests.js |
Centralizes timesheet deletion. |
tests/e2e/utils/api/userGroupRequests.js |
Centralizes user-group deletion. |
tests/e2e/utils/fileUtils.js |
Reformats file utilities. |
tests/package.json |
Establishes CommonJS scope for tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+43
to
+46
| attempt += 1; | ||
| if (attempt > retries) { | ||
| throw err; | ||
| } |
Comment on lines
822
to
+830
| // Delete Timesheets | ||
| for (const timesheetId of timesheetIds) { | ||
| if (!timesheetId || typeof timesheetId !== "string") { | ||
| console.error(`Invalid timesheetId encountered:`, timesheetId); | ||
| continue; | ||
| } | ||
| try { | ||
| await deleteTimesheetbyID(timesheetId, "admin"); | ||
| const { deleted } = await deleteTimesheetbyID(timesheetId, "admin"); | ||
| if (!deleted) failures.push(`Timesheet ${timesheetId}`); |
Comment on lines
+1195
to
+1198
| // Reuse an existing view with the same label so repeated runs do not pile | ||
| // up duplicates - the label is what the UI shows and the test looks for. | ||
| const existing = await getViewsByLabel(payload.label); | ||
| let viewId = existing?.[0]?.name; |
Comment on lines
+400
to
406
| async clearFilters() { | ||
| if (await this.clearAllFiltersButton.isVisible().catch(() => false)) { | ||
| await this.clearAllFiltersButton.click(); | ||
| await this.page | ||
| .waitForLoadState("networkidle", { timeout: 5000 }) | ||
| .catch(() => {}); | ||
| } |
Comment on lines
+196
to
+201
| if (!res.ok()) { | ||
| console.warn( | ||
| `⚠️ Could not list ${doctype}: ${res.status()} ${res.statusText()}`, | ||
| ); | ||
| return []; | ||
| } |
Comment on lines
+88
to
+90
| if (!deleted && reason === "LinkExistsError") { | ||
| await deleteAllocationsByEmployee(projectId); | ||
| return await deleteWithLockRetry( |
Comment on lines
117
to
+123
| export const deleteAllocation = async (allocationId) => { | ||
| return await apiRequest(`/api/resource/Resource%20Allocation/${allocationId}`, { method: "DELETE" }, "admin"); | ||
| return await deleteWithLockRetry( | ||
| () => deleteDocument("Resource Allocation", allocationId, "admin"), | ||
| { | ||
| label: `Resource Allocation ${allocationId}`, | ||
| }, | ||
| ); |
Comment on lines
+40
to
+46
| **2. Auth state is cached with no validity check.** `playwright.fixture.cjs` | ||
| builds `auth/<role>-w<workerIndex>.json` only when the file is *absent* | ||
| (`fs.access` → catch). It never checks whether the session still works. Once those | ||
| sessions expire, every test silently lands on the login page and the whole suite | ||
| goes red for one reason that has nothing to do with any test. If failures are | ||
| broad and uniform, delete `tests/e2e/auth/*-w*.json` and re-run before debugging | ||
| anything else. |
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
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.
Restores the
tests/e2e/Playwright suite against the redesigned PMS 2.0 UI, and fixes three infrastructure problems that were producing failures unrelated to any test.Why the suite was red
The redesign replaced the HTML tables the suite addressed with flex containers, moved filtering to a query builder, and removed several controls. On top of that, three problems were manufacturing failures that looked like test bugs:
DELETE /api/resource/..., and each caller caught and logged the failure, so seeded projects, tasks and employees accumulated on staging run after run. Frappe refuses to delete a document anything still links to, which is routine here — every seeded employee has a timesheet.sidexpires after ~12h, and Chromium drops an expired cookie while restoring a storageState — so the context kept every cookie exceptsid, the first navigation went out as Guest, the app redirected to/login, and the test timed out after 30s waiting on an unrelated locator. This accounted for 11 of 13 failures in one run.tests/package.jsonwas never committed. The root package declares"type": "module", so without it Playwright cannot load the config at all. The suite only ran on a machine that already had the file.Changes
Selectors / page objects — team grid addressed by row children and
pl-*depth classes instead of table roles;dispatchEventfor rows behind transparent overlays; project column headers matched on a word boundary (their accessible name is"Project name Project name column options", so^...$could never match); toast, reject-timesheet and review-pane locators rebuilt.Seeding — wait for the async project rollup before reading billing totals (this alone was seven failures); dedicated employees for the timesheet-review tests instead of one shared account; reviewees registered in TC53's roster after the seeding loop, since TC53's own turn rewrites its stub.
Cleanup — one
deleteDocument()returning{ deleted, reason }; dependants cleared first (cancel a submitted timesheet → delete it → delete the employee); sweeps for views, tasks and the timesheets booked onto shared accounts, bounded by a run marker taken from the server's own clock (Frappe stampscreationin the server timezone, so a UTC cutoff opened the window 5.5 hours early). Teardown can no longer report success when it reached nothing.Docs — adds the
next-pms-e2eskill (architecture map, DOM contract, failure playbook, plusprobe.mjs/results.py/cleanup.mjs) alongside the sibling project skills.Test status
Full suite on this branch, 4 workers, 9.6 minutes:
Read the 9 skips before treating this as green — one of them is masking a live app defect by design:
Clear all filtersempties the condition rows without committing, leaving 3 filters applied. The test is correct and the app is not; it stays skipped until the app is fixed, at which point it should be re-enabled rather than rewritten.The eight redesign-related skips each record their reason in the spec and need a retire-or-repoint decision rather than having their assertions rewritten to match the new behaviour — rewriting them would delete the coverage they exist for.