e2e: Improve setup and utils #642
Conversation
WalkthroughAdds Playwright E2E infra: pre-authentication setup producing storage-state files, custom page fixtures and utilities, updated tests to use those fixtures, new users E2E tests, and CI-aware Playwright configuration plus related docs and ignore rules. Changes
Sequence DiagramsequenceDiagram
participant CI as CI Environment
participant SetupProj as Playwright "setup" project
participant BrowserProj as Playwright Browser Projects
participant AuthStore as e2e/.auth (storage files)
participant Tests as E2E Test Suites
CI->>SetupProj: run `playwright test --project=setup` (via e2e:setup)
SetupProj->>SetupProj: navigate /login, authenticate (admin)
SetupProj->>AuthStore: write admin.json
SetupProj->>SetupProj: navigate /login, authenticate (user)
SetupProj->>AuthStore: write user.json
SetupProj-->>BrowserProj: signal setup complete (dependency)
BrowserProj->>AuthStore: load storage-state for tests
BrowserProj->>Tests: run tests using `page.login()` / `page.to()` with pre-auth state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (14)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used🧬 Code graph analysis (3)e2e/utils/types.ts (1)
prisma/seed/book.ts (1)
e2e/login.spec.ts (2)
🪛 LanguageToolREADME.md[style] ~110-~110: Consider shortening or rephrasing this to strengthen your wording. (MAKE_CHANGES) ⏰ 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). (1)
🔇 Additional comments (8)
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 |
ntatoud
force-pushed
the
test/improve-base-for-e2e-tests
branch
from
37e86e5 to
6b9369a
Compare
![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
🧹 Nitpick comments (3)
e2e/utils/types.ts (1)
1-11: Preferimport typeforExtendedPageto avoid unnecessary runtime coupling
CustomFixtureonly needsExtendedPageat the type level. Keeping a value import from'e2e/utils/page'makes the circular dependency betweentypes.tsandpage.tstighter than needed and may introduce a runtime import that isn’t actually used.I’d recommend switching to a type‑only import (and optionally doing the same for the Playwright types):
-import { - PlaywrightTestArgs, - PlaywrightTestOptions, - TestFixture, -} from '@playwright/test'; -import { ExtendedPage } from 'e2e/utils/page'; +import type { + PlaywrightTestArgs, + PlaywrightTestOptions, + TestFixture, +} from '@playwright/test'; +import type { ExtendedPage } from 'e2e/utils/page';This keeps the fixture typing intact while loosening runtime module coupling.
e2e/utils/page.ts (1)
13-28: Extended page utilities design is soundModeling helpers via a
PageUtilsinterface and intersecting them into the Playwrightpagefixture throughExtendedPageprovides a clean, typed surface forpage.loginandpage.to. If you later need to reference these helpers directly in tests or other utils, consider exportingPageUtilsas well, but it’s not strictly necessary right now.playwright.config.ts (1)
35-52: LGTM! Proper setup project configuration.The setup project is correctly configured to run authentication setup before tests in CI. The conditional dependencies (
process.env.CI ? ['setup'] : []) appropriately skip the setup locally, allowing developers to manually runpnpm e2e:setuponce and reuse the authentication state.One minor note: The
sonarjs/slow-regexdisable comment on line 35 appears unnecessary for the simple regex/.*\.setup\.ts/. Consider removing it unless there's a specific reason for the suppression.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitignore(1 hunks)README.md(1 hunks)e2e/api-schema.spec.ts(1 hunks)e2e/login.spec.ts(1 hunks)e2e/setup/auth.setup.ts(1 hunks)e2e/users.spec.ts(1 hunks)e2e/utils/constants.ts(1 hunks)e2e/utils/index.ts(1 hunks)e2e/utils/page-utils.ts(0 hunks)e2e/utils/page.ts(1 hunks)e2e/utils/types.ts(1 hunks)package.json(1 hunks)playwright.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/utils/page-utils.ts
🧰 Additional context used
🧬 Code graph analysis (6)
e2e/login.spec.ts (2)
e2e/utils/constants.ts (2)
ADMIN_EMAIL(7-7)USER_EMAIL(4-4)e2e/utils/page-utils.ts (2)
login(32-59)page(27-61)
e2e/users.spec.ts (2)
e2e/utils/index.ts (1)
test(9-9)prisma/seed/user.ts (1)
createUsers(7-58)
e2e/utils/page.ts (4)
e2e/utils/types.ts (1)
CustomFixture(8-11)src/lib/i18n/constants.ts (1)
DEFAULT_LANGUAGE_KEY(11-11)e2e/utils/page-utils.ts (2)
login(32-59)page(27-61)src/features/auth/page-login.tsx (1)
PageLogin(27-147)
e2e/utils/index.ts (2)
e2e/utils/page.ts (2)
ExtendedPage(28-28)pageWithUtils(30-66)e2e/utils/page-utils.ts (1)
page(27-61)
e2e/setup/auth.setup.ts (2)
e2e/utils/constants.ts (4)
ADMIN_EMAIL(7-7)ADMIN_FILE(6-6)USER_EMAIL(4-4)USER_FILE(3-3)e2e/utils/page-utils.ts (2)
login(32-59)page(27-61)
e2e/utils/types.ts (1)
e2e/utils/page.ts (1)
ExtendedPage(28-28)
🪛 LanguageTool
README.md
[style] ~110-~110: Consider shortening or rephrasing this to strengthen your wording.
Context: ...es contain authentication logic. If you make changes to your local database instance, you shoul...
(MAKE_CHANGES)
🔇 Additional comments (9)
.gitignore (1)
56-60: Ignoring generated auth state looks goodAdding
e2e/.authkeeps local Playwright storage-state files out of git while remaining consistent with the other e2e artifacts.package.json (1)
33-41: New e2e and DB lifecycle scripts are consistent and well‑composed
e2e:setupfollows the existing dotenv/cross-env Playwright pattern scoped to the setup project, anddb:resetsensibly composesdk:clear,dk:init, anddb:initwith&&so failures short‑circuit. Behavior aligns with the rest of the script suite.e2e/api-schema.spec.ts (1)
1-15: Switching toe2e/utilsimport is aligned with the new fixture setupRouting
test/expectthroughe2e/utilskeeps this spec consistent with the custom fixture layer and will make it easier to evolve shared fixtures later without touching individual tests, assuminge2e/utilsre-exports Playwright’stestandexpectas intended.e2e/login.spec.ts (1)
1-24: LGTM! Clean refactoring to the new fixture system.The migration from the helper-based approach to the custom fixture pattern is well-executed. Using
page.to()andpage.login()directly on the page object is more ergonomic and aligns with Playwright's fixture extension patterns.README.md (1)
104-110: LGTM! Clear documentation of the new E2E workflow.The updated commands and the warning about re-running setup when the database changes are helpful additions. The warning appropriately guides users on when to refresh the authentication context.
e2e/setup/auth.setup.ts (1)
14-32: LGTM! Well-structured authentication setup.The setup routines correctly implement Playwright's recommended pattern for multiple signed-in roles. Both admin and user setups follow a consistent pattern: navigate, authenticate, verify, and persist storage state. The assertions before saving state ensure valid authentication contexts.
e2e/utils/constants.ts (1)
1-7: LGTM! Clean constant definitions.The constants are well-organized with an internal base path and public file path exports. This provides a single source of truth for storage state file locations.
e2e/utils/index.ts (1)
1-9: LGTM! Proper fixture extension pattern.The custom fixture setup follows Playwright's recommended approach. Extending the base test with
pageWithUtilsand re-exporting all utilities provides a clean, unified entry point for tests.e2e/users.spec.ts (1)
42-44: Investigate ifforce: trueis necessary.Using
force: truebypasses Playwright's actionability checks (visibility, stability, enabled state). While sometimes necessary for overlays or animations, it can mask timing issues or UI problems that should be fixed.Consider removing
force: trueand ensuring elements are properly actionable. If the clicks fail without it, investigate whether there are underlying timing or UI issues that should be addressed.Also applies to: 58-62
package.json
Outdated
| "dk:stop": "docker compose stop", | ||
| "dk:clear": "docker compose down --volumes", | ||
| "db:init": "pnpm db:push && pnpm db:seed", | ||
| "db:reset": "pnpm dk:clear && pnpm dk:init && pnpm db:init", |
There was a problem hiding this comment.
I think that db:init can fail here if db is not ready in the docker after dk:init 😕
There was a problem hiding this comment.
It was more of a quick workaround to get a "Clean db state" between e2e test runs when running them in local !
We should discuss what's the best path forward to have consistent e2e test runs
ntatoud
force-pushed
the
test/improve-base-for-e2e-tests
branch
from
ede0470 to
c46fa81
Compare
|
Summary by CodeRabbit
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.