e2e: Improve setup and utils #642
Conversation
|
Warning Rate limit exceeded@ntatoud has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis PR establishes Playwright end-to-end test infrastructure with pre-authenticated contexts and custom page utilities. It introduces a setup project that runs before main tests to generate authentication state files for admin and user roles, refactors test imports to use a new custom fixture system, and adds new user management tests. Configuration is updated to support the setup flow. Changes
Sequence DiagramsequenceDiagram
participant CI as CI Environment
participant Setup as Setup Project
participant AuthFile as Auth State Files
participant MainTest as Main Tests
CI->>Setup: Run e2e:setup (on CI)
Setup->>Setup: Admin login flow<br/>(navigate, authenticate, save state)
Setup->>AuthFile: Save admin.json
Setup->>Setup: User login flow<br/>(navigate, authenticate, save state)
Setup->>AuthFile: Save user.json
Setup-->>MainTest: Pre-auth contexts ready
MainTest->>MainTest: Use page.login() & page.to()<br/>with pre-loaded storage state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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
|
| "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
Summary by CodeRabbit
New Features
Chores
e2e:setupanddb:resetnpm commands for test environment management.Documentation