Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end testing infrastructure using Playwright to the React frontend template. The changes enable automated browser testing for authentication and protected page flows, with comprehensive test coverage for login validation, form submissions, and navigation.
Changes:
- Adds Playwright testing framework with configuration and test scripts
- Creates E2E tests for login flow, example page interactions, and detail page navigation
- Implements authentication fixture for testing protected routes
- Adds GitHub Actions workflow for running tests in CI
- Strengthens password validation requirements (breaking change: now requires 8+ chars, uppercase, lowercase, number, and special character)
- Modifies UI components to add semantic HTML elements for better test selectors
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds @playwright/test dependency and test scripts (test:e2e, test:e2e:ui, test:e2e:debug, etc.) |
| playwright.config.ts | Configures Playwright with test directory, browser settings, and local dev server integration |
| e2e/auth/login.spec.ts | Comprehensive login page tests including validation, API mocking, and error handling |
| e2e/example/example-page.spec.ts | Tests for protected page access, form submissions, and logout functionality |
| e2e/example/example-detail.spec.ts | Tests for detail page navigation and content display |
| e2e/fixtures/auth.fixture.ts | Reusable authentication fixture for tests requiring logged-in state |
| e2e/config.ts | Test configuration for accessing environment variables |
| e2e/README.md | Comprehensive documentation for E2E testing setup and best practices |
| .github/workflows/e2e-tests.yml | CI workflow for running E2E tests on push/PR to main and develop branches |
| src/features/auth/components/LoginForm/LoginForm.tsx | Strengthens password validation with additional character requirements |
| src/features/auth/api/auth.api.ts | Adds test credential validation to mock API (security concern) |
| src/app/config/env.ts | Adds testEmail and testPassword configuration variables |
| src/pages/LoginPage/LoginPage.tsx | Wraps page title in h1 element for semantic HTML |
| src/pages/ExamplePage/ExamplePage.tsx | Wraps titles and descriptions in semantic HTML elements |
| .gitignore | Updates to include Playwright-specific directories (but removes many other entries) |
| yarn.lock | Adds Playwright and related dependency entries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /* Run your local dev server before starting the tests */ | ||
| webServer: { | ||
| command: 'yarn dev', |
There was a problem hiding this comment.
The webServer configuration starts the dev server with 'yarn dev' but doesn't pass environment variables needed for the tests (VITE_TEST_EMAIL and VITE_TEST_PASSWORD). When running tests locally with 'yarn test:e2e', these variables won't be available to the application unless they're in the system environment. Consider using 'env-cmd -e local yarn dev' or documenting that developers need to set these environment variables before running tests.
| command: 'yarn dev', | |
| command: 'env-cmd -e local yarn dev', |
| await page.getByRole('button', { name: /login/i }).click(); | ||
|
|
||
| // Should redirect to the specified path | ||
| await expect(page).toHaveURL('/example', { timeout: 5000 }); |
There was a problem hiding this comment.
The test waits for the URL to be exactly '/example', but due to the route configuration changes, after login the user is redirected from '/' to '/example'. While this should work, it's testing the final destination after a redirect rather than testing that the redirect happens. Consider also testing that the initial redirect from '/' to '/example' works correctly in the routing tests.
| testEmail: import.meta.env.VITE_TEST_EMAIL || '', | ||
| testPassword: import.meta.env.VITE_TEST_PASSWORD || '', |
There was a problem hiding this comment.
Empty string fallbacks for test credentials can cause silent failures. If these environment variables are not set, the auth API will reject empty strings, but it may not be immediately clear why. Consider removing the fallback values and letting the code fail early if credentials are not configured, or adding validation that throws a clear error message.
| const loginFormSchema = yup.object({ | ||
| email: yup.string().email('Invalid email address').required('Email is required'), | ||
| password: yup.string().min(6, ({ min }) => `Password must be at least ${min} characters`).required('Password is required'), | ||
| // Password must have an uppercase letter, a lowercase letter, a number, and a special character |
There was a problem hiding this comment.
The comment describes password requirements but doesn't mention the minimum length requirement of 8 characters. Update the comment to include all validation rules: "Password must be at least 8 characters and have an uppercase letter, a lowercase letter, a number, and a special character (!@#$%^&*)".
| // Password must have an uppercase letter, a lowercase letter, a number, and a special character | |
| // Password must be at least 8 characters and have an uppercase letter, a lowercase letter, a number, and a special character (!@#$%^&*). |
| await page.goto('/login'); | ||
|
|
||
| // Fill in login credentials | ||
| // Note: Password must meet requirements: 8+ chars, uppercase, lowercase, number, special char |
There was a problem hiding this comment.
The comment mentions password requirements but uses generic character requirements. Since the password validation in the codebase specifically requires special characters from the set !@#$%^&*, the comment should be more specific: "Note: Password must be at least 8 characters with uppercase, lowercase, number, and special character (!@#$%^&*)".
| // Note: Password must meet requirements: 8+ chars, uppercase, lowercase, number, special char | |
| // Note: Password must be at least 8 characters with uppercase, lowercase, number, and special character (!@#$%^&*). |
1 participant
Overview