Repository navigation
test(e2e): add playwright-utils e2e package with a non-blocking CI job - #7188
yz-anaconda wants to merge 2 commits into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The private dependency must be isolated from Actions caches that fork workflows can restore.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Introduces an isolated Playwright package for gradual migration of the web app’s end-to-end tests without adding private-registry requirements to the workspace.
Changes:
- Migrates two legacy specs covering four authentication and accessibility tests.
- Adds test fixtures, page objects, database seeding, and navigation helpers.
- Adds a non-required CI workflow with route warm-up and report uploads.
| File | Description |
|---|---|
e2e/tsconfig.json |
Configures type checking and import aliases. |
e2e/tests/testdata/ui/auth/session-test-data.ts |
Defines authentication endpoints. |
e2e/tests/testdata/ui/auth/get-started-test-data.ts |
Defines redirect expectations. |
e2e/tests/testdata/ui/app-shell/app-shell-test-data.ts |
Defines shell cases and layout expectations. |
e2e/tests/support/core/seed-data.ts |
Creates isolated test users. |
e2e/tests/support/core/hydration.ts |
Adds hydration waits and overlay handling. |
e2e/tests/support/core/db.ts |
Provides database access for tests. |
e2e/tests/specs/ui/auth/get-started-dashboard-link.spec.ts |
Migrates authentication routing tests. |
e2e/tests/specs/ui/app-shell/app-shell-accessibility.spec.ts |
Migrates shell accessibility tests. |
e2e/tests/pages/ui/auth/session-page.ts |
Implements fake-login authentication. |
e2e/tests/pages/ui/auth/get-started-page.ts |
Encapsulates signed-in routing checks. |
e2e/tests/pages/ui/app-shell/app-shell-page.ts |
Encapsulates shell accessibility assertions. |
e2e/tests/pages/api/auth/get-started-api.ts |
Checks signed-out HTTP redirects. |
e2e/tests/fixtures/fixture.ts |
Registers isolated test fixtures. |
e2e/README.md |
Documents installation, execution, and CI. |
e2e/pnpm-lock.yaml |
Locks standalone package dependencies. |
e2e/playwright.config.ts |
Configures Chromium and failure artifacts. |
e2e/package.json |
Defines dependencies and test commands. |
e2e/eslint.config.mjs |
Configures package-specific linting. |
e2e/ci/warm-up.mjs |
Warms tested routes before execution. |
e2e/ci/start-web.mjs |
Starts the development app for CI. |
e2e/.prettierrc |
Defines package formatting settings. |
e2e/.npmrc |
Configures private-registry authentication. |
e2e/.gitignore |
Excludes dependencies and generated artifacts. |
apps/web/tests/e2e/get-started-dashboard-link.spec.ts |
Removes the migrated routing spec. |
apps/web/tests/e2e/app-shell-accessibility.spec.ts |
Removes the migrated accessibility spec. |
.prettierignore |
Excludes the isolated package. |
.oxlintrc.json |
Excludes package-specific linting scope. |
.oxfmtrc.json |
Excludes package-specific formatting scope. |
.github/workflows/e2e.yml |
Adds CI setup, execution, and artifacts. |
.github/CODEOWNERS |
Assigns ownership of the new package. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
103c540 to
c86bcc8
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental changes since the prior review are robustness and accuracy refinements — stricter error handling in Files Reviewed (14 files)
Previous Review Summaries (3 snapshots, latest commit bd5c71e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit bd5c71e)Status: No Issues Found | Recommendation: Merge Executive SummaryThe force-push removed the incremental base commit, so this is a full review of the current HEAD: the isolated Files Reviewed (31 files)
Previous review (commit fd0cb0e)Status: No Issues Found | Recommendation: Merge Executive SummaryThe three prior findings are resolved at HEAD: the E2E pool now has an idle-client Files Reviewed (3 files)
Previous review (commit c86bcc8)Status: 3 Issues Found | Recommendation: Address before merge Executive SummaryOne reliability risk (a pool without an Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (31 files)
No memory leaks identified in the changed code (pool is bounded with Reviewed by deepseek-v4.1-flash · Input: 63K · Output: 11.5K · Cached: 585.2K Review guidance: REVIEW.md from base branch |
c86bcc8 to
fd0cb0e
Compare
| }; | ||
|
|
||
| /** A signed-in visitor lands on `/profile` or on an `/organizations/...` page. */ | ||
| export const signedInDestination = /^https?:\/\/[^/]+\/(profile([?#]|$)|organizations\/)/; |
There was a problem hiding this comment.
I will change the name to something like signedDestinationRegExp or something similar, so from outside you can know straight away the expression.
There was a problem hiding this comment.
Thanks, fixed: renamed to signedInDestinationRegExp.
| "globals": "17.13.0", | ||
| "pg": "8.23.1", | ||
| "prettier": "3.9.4", | ||
| "typescript": "5.9.3" |
There was a problem hiding this comment.
Small suggestion: typescript, prettier, eslint, and @types/node are already regular dependencies of @anaconda/playwright-utils at these same versions, so we may not need to add them separately here.
There was a problem hiding this comment.
That holds for npm, which hoists them. This package uses pnpm: on a clean --frozen-lockfile install without direct entries, validate/lint/format fail with tsc/eslint/prettier: command not found, and tsc can't resolve @types/node. So they stay, pinned to the same versions.
… CI job - e2e/: isolated pnpm package (outside the workspace) on @anaconda/playwright-utils; the private package is read with a token passed as NODE_AUTH_TOKEN, never stored in the repo - migrate the get-started and app-shell accessibility specs (4 tests) and remove their legacy copies; the other legacy specs keep running from apps/web/tests/e2e - .github/workflows/e2e.yml: Postgres, migrations, next dev with fake login, route warm-up, tests; not a required check, fork PRs skipped, token scoped to the install step - CODEOWNERS for e2e/; root oxlint/oxfmt/prettier skip e2e/
fd0cb0e to
bd5c71e
Compare
| }; | ||
|
|
||
| async function signIn() { | ||
| const { csrfToken } = await (await request('/api/auth/csrf', ROUTE_TIMEOUT_MS)).json(); |
There was a problem hiding this comment.
No status/shape checks here — a failed csrf/login/session call won't throw, so the outer catch may never fires and the script silently warms /admin* while signed out.
const csrfRes = await request('/api/auth/csrf', ROUTE_TIMEOUT_MS);
const { csrfToken } = await csrfRes.json();
if (!csrfToken) throw new Error(`csrf fetch failed: ${csrfRes.status}`);
same check might be needed for session.user after the fake-login call
There was a problem hiding this comment.
Fixed: a failed CSRF or session check now throws and shows up as a ::warning:: in the Actions UI. Warm-up still continues signed out, but no longer silently.
| await expect.poll(() => isHydrated(page), { timeout: STANDARD_TIMEOUT }).toBe(true); | ||
| } catch { | ||
| // See above. | ||
| } |
There was a problem hiding this comment.
@yz-anaconda If the hydration fails can we fail it here rather than in test which would hard to trace back
import { STANDARD_TIMEOUT, logger } from '@anaconda/playwright-utils';
async function waitForHydration(page: Page): Promise<void> {
try {
await expect.poll(() => isHydrated(page), { timeout: STANDARD_TIMEOUT }).toBe(true);
} catch {
logger.warn(`Page did not hydrate within ${STANDARD_TIMEOUT}ms: ${page.url()}`);
}
}
There was a problem hiding this comment.
Done as in your snippet: it logs a warning with the page URL. I kept it a warning, not a failure, so an external redirect doesn't fail a valid navigation.
| await page.addStyleTag({ content: 'nextjs-portal { pointer-events: none !important; }' }); | ||
| } catch { | ||
| // The page navigated away before the style could attach. | ||
| } |
There was a problem hiding this comment.
@yz-anaconda
can we try this simplification with one init script instead of re-adding the style after every goto/reload
export function installHydrationWait(page: Page): void {
// Runs at the start of every document, so it survives navigations that bypass goto/reload.
void page.addInitScript(() => {
const sheet = new CSSStyleSheet();
sheet.replaceSync('nextjs-portal { pointer-events: none !important; }');
document.adoptedStyleSheets = [...document.adoptedStyleSheets, sheet];
});
const originalGoto = page.goto.bind(page);
page.goto = async (...args: Parameters<Page['goto']>) => {
const response = await originalGoto(...args);
await waitForHydration(page);
return response;
};
const originalReload = page.reload.bind(page);
page.reload = async (...args: Parameters<Page['reload']>) => {
const response = await originalReload(...args);
await waitForHydration(page);
return response;
};
}
There was a problem hiding this comment.
Agreed, done: one init script on the context (see the fixture thread).
| // Every `goto`/`reload` waits for React hydration (see `tests/support/core/hydration.ts`). | ||
| page: async ({ page }, use) => { | ||
| installHydrationWait(page); | ||
| await use(page); |
There was a problem hiding this comment.
The overlay fix and hydration wait only cover the fixture's page and only goto/reload. Full navigations (form submits, redirects), new tabs and popups get neither. Suggest moving setup to the context so every tab and navigation is covered:
fixture.ts: replace the page: override with:
context: async ({ context }, use) => {
// Runs at the start of every document in every tab, so it survives navigations that bypass goto/reload.
await context.addInitScript(() => {
const sheet = new CSSStyleSheet();
sheet.replaceSync('nextjs-portal { pointer-events: none !important; }');
document.adoptedStyleSheets = [...document.adoptedStyleSheets, sheet];
});
context.on('page', installHydrationWait); // fires for the default page, newPage() and popups
await use(context);
},hydration.ts: drop neutralizeNextDevOverlay, export waitForHydration, and log instead of failing silently:
export async function waitForHydration(page: Page): Promise<void> {
try {
await expect.poll(() => isHydrated(page), { timeout: STANDARD_TIMEOUT }).toBe(true);
} catch {
logger.warn(`Page did not hydrate within ${STANDARD_TIMEOUT}ms: ${page.url()}`);
}
}
export function installHydrationWait(page: Page): void {
const originalGoto = page.goto.bind(page);
page.goto = async (...args: Parameters<Page['goto']>) => {
const response = await originalGoto(...args);
await waitForHydration(page);
return response;
};
const originalReload = page.reload.bind(page);
page.reload = async (...args: Parameters<Page['reload']>) => {
const response = await originalReload(...args);
await waitForHydration(page);
return response;
};
}A popup's first load doesn't go through goto, so wait explicitly there:
const popup = await page.waitForEvent('popup');
await waitForHydration(popup);There was a problem hiding this comment.
Good point, done: the context fixture covers the default page, new tabs and popups, and the overlay fix now applies to every document, redirects and form submits included. The hydration wait still covers goto/reload only, and waitForHydration stays private until a test opens a popup.
| /** | ||
| * Creates the rows a test needs directly in the database, so every test owns its data and never | ||
| * depends on shared dev records. Ids and emails are unique per call. | ||
| */ |
There was a problem hiding this comment.
@yz-anaconda are we cleaning the fake credentials after every test??
There was a problem hiding this comment.
No, on purpose: each test gets a unique user and a fresh browser context, and the CI database is thrown away after the run. I've added a short note about this to the SeedData JSDoc.
| * independent of the monorepo's Drizzle schema; column names mirror packages/db/src/schema.ts. | ||
| */ | ||
| const pool = new Pool({ | ||
| connectionString: process.env.POSTGRES_URL ?? 'postgres://postgres:postgres@localhost:5432/postgres', |
There was a problem hiding this comment.
@yz-anaconda This may fall silently to localhost URL, this may seed a different DB than the app reads
There was a problem hiding this comment.
Right, fixed: no default anymore. The URL comes from POSTGRES_URL or the app's .env.local, otherwise the tests fail with a clear error.
- seed the database the app reads: POSTGRES_URL or the root .env.local, no localhost default;
base URL follows the worktree port offset; DB connect timeout
- overlay fix as one context init script (popups included); hydration wait on every tab via
context.on('page'); log a warning when a page doesn't hydrate
- warm-up and sign-in check every auth response; errors carry statuses, never the session
- exact path match in the app shell checks; visible-then-measure sidebar boxes; Location check
- start-web reads apps/web/.env.test.local; README, comments and testdata docs updated
Summary
Adds
e2e/, a UI test package built on@anaconda/playwright-utils, and a GitHub Actions job that runs it. This PR migrates 2 legacy specs (4 tests) to prove the setup; the rest ofapps/web/tests/e2ekeeps running as before and will move over in small follow-up PRs.@anaconda/playwright-utilsis private and this repo is public, soe2e/is a separate pnpm package outside the workspace with its own lockfile. Nothing else in the repo depends on the private registry.The CI job (
.github/workflows/e2e.yml) starts Postgres andnext devwith fake login, warms up the tested routes, and runs the tests. It is not a required check and is skipped for fork PRs. The package token is used only in thee2e/install step.Verification
apps/web(pnpm test:e2e).Visual Changes
N/A
Reviewer Notes
/get-startedrouting.