Conversation
- Add Playwright for E2E testing with page object pattern - Add Vitest configuration for server unit tests - Create 63 unit tests for core business logic: - tokens, accounts, otps, rate-limits, collaborations, permissions - Add test fixtures and utilities for backend testing - Create GitHub Actions CI/CD workflow for automated testing - Update .gitignore with Playwright directories
Add test IDs to login, register, and workspace components: - email-input, password-input, login-button, register-link - name-input, register-button - workspace-name-input, workspace-create-button - workspace-container, workspace-create
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive testing infrastructure to the project, including E2E testing with Playwright, unit testing with Vitest, and automated CI/CD workflows via GitHub Actions.
Key changes:
- Added Playwright for end-to-end browser testing with fixtures and page objects
- Implemented Vitest unit tests for core business logic (permissions, tokens, rate limits, etc.)
- Created CI/CD pipeline with lint, unit test, integration test, and E2E test jobs
- Added
data-testidattributes to UI components for reliable test selectors
Reviewed changes
Copilot reviewed 35 out of 37 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
playwright.config.ts |
Playwright configuration with multi-browser support and webServer setup |
e2e/tests/**/*.spec.ts |
E2E test suites for auth, workspaces, and pages |
e2e/pages/*.page.ts |
Page object models for login, workspace, and editor |
e2e/fixtures/*.fixture.ts |
Test fixtures for authentication and API client |
apps/server/src/lib/__tests__/*.test.ts |
Unit tests for tokens, accounts, rate limits, OTPs, collaborations |
packages/core/src/lib/__tests__/permissions.test.ts |
Unit tests for permission system |
apps/server/vitest.config.ts |
Vitest configuration for server tests |
packages/ui/src/components/**/*.tsx |
Added data-testid attributes for testing |
.github/workflows/test.yml |
CI/CD workflow with lint, unit, integration, and E2E jobs |
docs/TESTING.md |
Comprehensive testing documentation |
package.json |
Added Playwright and test scripts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const editorPage = new EditorPage(page); | ||
| await expect(editorPage.editor).toBeVisible({ timeout: 10000 }); | ||
|
|
||
| await editorPage.typeInEditor('Hello, World!'); |
There was a problem hiding this comment.
The method typeInEditor() is called but the EditorPage class only defines typeContent(). This will cause the test to fail at runtime.
| await editorPage.typeInEditor('Hello, World!'); | |
| await editorPage.typeContent('Hello, World!'); |
| async register(name: string, email: string, password: string) { | ||
| await this.registerLink.click(); | ||
| await this.page.getByTestId('name-input').fill(name); | ||
| await this.emailInput.fill(email); | ||
| await this.passwordInput.fill(password); |
There was a problem hiding this comment.
The register method is missing a parameter for the confirm password field. Line 56 references confirmPassword input that should be included in the method signature to maintain consistency with the form fields.
| async register(name: string, email: string, password: string) { | |
| await this.registerLink.click(); | |
| await this.page.getByTestId('name-input').fill(name); | |
| await this.emailInput.fill(email); | |
| await this.passwordInput.fill(password); | |
| async register(name: string, email: string, password: string, confirmPassword: string) { | |
| await this.registerLink.click(); | |
| await this.page.getByTestId('name-input').fill(name); | |
| await this.emailInput.fill(email); | |
| await this.passwordInput.fill(password); | |
| await this.page.getByTestId('confirm-password-input').fill(confirmPassword); |
| }) => { | ||
| const loginPage = new LoginPage(page); | ||
| await loginPage.goto(); | ||
| await loginPage.goToRegister(); |
There was a problem hiding this comment.
The method goToRegister() is called on line 10 but doesn't exist in the LoginPage class. The class only has a register() method that includes registration logic. Either add a goToRegister() method that only clicks the register link, or update the test to use the existing method.
.github/workflows/test.yml
Outdated
| run: npm run preview & | ||
| working-directory: apps/web | ||
| env: | ||
| VITE_API_URL: http://localhost:3000 | ||
|
|
||
| - name: Wait for services | ||
| run: | | ||
| npx wait-on http://localhost:3000/config http://localhost:4173 --timeout 60000 | ||
|
|
||
| - name: Run E2E tests | ||
| run: npm run e2e -- --project=chromium | ||
| env: | ||
| E2E_BASE_URL: http://localhost:4173 |
There was a problem hiding this comment.
The wait-on command is checking for port 4173, but this doesn't appear to be defined anywhere in the workflow. Vite's preview server typically uses port 4173, but there's no explicit configuration shown. The playwright.config.ts uses port 4000 as the baseURL, creating an inconsistency between the CI configuration and the local test configuration.
| run: npm run preview & | |
| working-directory: apps/web | |
| env: | |
| VITE_API_URL: http://localhost:3000 | |
| - name: Wait for services | |
| run: | | |
| npx wait-on http://localhost:3000/config http://localhost:4173 --timeout 60000 | |
| - name: Run E2E tests | |
| run: npm run e2e -- --project=chromium | |
| env: | |
| E2E_BASE_URL: http://localhost:4173 | |
| run: npm run preview -- --port 4000 & | |
| working-directory: apps/web | |
| env: | |
| VITE_API_URL: http://localhost:3000 | |
| - name: Wait for services | |
| run: | | |
| npx wait-on http://localhost:3000/config http://localhost:4000 --timeout 60000 | |
| - name: Run E2E tests | |
| run: npm run e2e -- --project=chromium | |
| env: | |
| E2E_BASE_URL: http://localhost:4000 |
| "postinstall": "node --no-warnings --loader ts-node/esm scripts/src/postinstall/index.ts" | ||
| }, | ||
| "devDependencies": { | ||
| "@playwright/test": "^1.49.0", |
There was a problem hiding this comment.
The package.json specifies @playwright/test version ^1.49.0, but package-lock.json shows version 1.57.0 is installed. While this is within the semver range, it's worth noting that there's a significant version jump. The caret (^) allows minor version updates, which is working as intended here.
| "@playwright/test": "^1.49.0", | |
| "@playwright/test": "^1.57.0", |
| await createPageButton.click(); | ||
|
|
||
| const editorPage = new EditorPage(page); | ||
| await expect(editorPage.editor).toBeVisible({ timeout: 10000 }); |
There was a problem hiding this comment.
The EditorPage class has properties titleInput and contentEditor, but the test code references editorPage.editor and editorPage.typeInEditor() which don't exist in the class. The class defines contentEditor and typeContent() instead. This mismatch will cause the E2E tests to fail.
Greptile SummaryThis PR establishes comprehensive testing infrastructure for the project, adding Vitest for unit/integration tests and Playwright for E2E tests. The changes include 63 unit tests covering authentication, permissions, rate limiting, and collaboration logic, plus 14 E2E tests for auth flows, workspace management, and page editing. Key additions:
Infrastructure highlights:
Issues addressed from previous review:
The testing foundation is solid and follows best practices. Some E2E tests have conditional logic that could silently pass, and a few page objects lack proper wait conditions, but these are minor issues that don't block the PR. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant Services as CI Services
participant Tests as Test Suites
Dev->>GH: Push/PR to main
GH->>GH: Start CI Pipeline
par Lint & Type Check
GH->>Tests: npm run lint
GH->>Tests: npm run compile
end
par Unit Tests
GH->>Services: Start Redis
Services-->>GH: Redis Ready
GH->>Tests: Run Vitest (server unit tests)
Tests-->>GH: Unit Test Results
end
par Integration Tests
GH->>Services: Start PostgreSQL + Redis + MinIO
Services-->>GH: Services Ready
GH->>Services: Create MinIO bucket
GH->>Services: Run DB migrations
GH->>Tests: Run Vitest (integration tests)
Tests-->>GH: Integration Test Results
end
par E2E Tests
GH->>Services: Start PostgreSQL + Redis + MinIO
Services-->>GH: Services Ready
GH->>GH: Install Playwright browsers
GH->>Services: Create MinIO bucket
GH->>Services: Run DB migrations
GH->>GH: Build packages
GH->>Services: Start server + web app
GH->>Services: Wait for services (wait-on)
GH->>Tests: Run Playwright E2E tests
Tests-->>GH: E2E Test Results
Tests-->>GH: Upload artifacts (reports/screenshots)
end
GH-->>Dev: Pipeline Results
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
packages/ui/src/components/accounts/email-register.tsx, line 172-179 (link)style: confirmPassword field is missing
data-testidattribute while other form fields have test selectorsShould the confirm password field also have a test selector for completeness?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
36 files reviewed, 20 comments
| const { token } = generateToken(DEVICE_ID); | ||
| const parsed = parseToken(token); | ||
|
|
||
| const expectedSecret = token.slice(4 + 28); |
There was a problem hiding this comment.
style: Magic numbers (4 + 28) hardcode token structure assumptions. Consider extracting these from the token implementation or using constants.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/lib/__tests__/tokens.test.ts
Line: 58:58
Comment:
**style:** Magic numbers (4 + 28) hardcode token structure assumptions. Consider extracting these from the token implementation or using constants.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| expect(result.canAccess).toBe(false); | ||
| expect(result.accessSource).toBe('none'); | ||
| }); |
There was a problem hiding this comment.
style: missing assertion for effectiveRole being null when access is denied
| expect(result.canAccess).toBe(false); | |
| expect(result.accessSource).toBe('none'); | |
| }); | |
| expect(result.canAccess).toBe(false); | |
| expect(result.accessSource).toBe('none'); | |
| expect(result.effectiveRole).toBeNull(); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/lib/__tests__/permissions.test.ts
Line: 161:163
Comment:
**style:** missing assertion for effectiveRole being null when access is denied
```suggestion
expect(result.canAccess).toBe(false);
expect(result.accessSource).toBe('none');
expect(result.effectiveRole).toBeNull();
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| const uniqueCodes = new Set(codes); | ||
| expect(uniqueCodes.size).toBeGreaterThan(950); |
There was a problem hiding this comment.
style: The 95% uniqueness threshold might be too lenient for security-critical OTP generation. With 1M possible 6-digit codes, expect near 100% uniqueness from 1000 samples. Is this threshold intentionally conservative to account for potential implementation issues, or should it be stricter?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/lib/__tests__/otps.test.ts
Line: 34:34
Comment:
**style:** The 95% uniqueness threshold might be too lenient for security-critical OTP generation. With 1M possible 6-digit codes, expect near 100% uniqueness from 1000 samples. Is this threshold intentionally conservative to account for potential implementation issues, or should it be stricter?
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| await expect(page.getByText('String must contain at least')).toBeVisible({ | ||
| timeout: 5000, | ||
| }); |
There was a problem hiding this comment.
style: Using partial text match for validation errors is fragile and may break if error messages change. Consider using a more specific test ID or data attribute.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: e2e/tests/auth/register.spec.ts
Line: 25:27
Comment:
**style:** Using partial text match for validation errors is fragile and may break if error messages change. Consider using a more specific test ID or data attribute.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| await page.getByTestId('name-input').fill(testUser.name); | ||
| await page.getByTestId('email-input').fill(testUser.email); | ||
| await page.getByTestId('password-input').fill(testUser.password); | ||
| await page.locator('input[name="confirmPassword"]').fill(testUser.password); |
There was a problem hiding this comment.
style: Inconsistent selector usage - mixing getByTestId with locator('input[name="confirmPassword"]'). Consider adding a test ID to the confirm password field for consistency.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: e2e/tests/auth/register.spec.ts
Line: 56:56
Comment:
**style:** Inconsistent selector usage - mixing `getByTestId` with `locator('input[name="confirmPassword"]')`. Consider adding a test ID to the confirm password field for consistency.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| const createPageButton = page.getByTestId('create-page-button'); | ||
| if ( | ||
| await createPageButton.isVisible({ timeout: 5000 }).catch(() => false) | ||
| ) { | ||
| await createPageButton.click(); | ||
|
|
||
| const editorPage = new EditorPage(page); | ||
| await expect(editorPage.editor).toBeVisible({ timeout: 10000 }); | ||
| } |
There was a problem hiding this comment.
logic: Test silently passes if create-page-button is not visible, potentially hiding UI issues
| const createPageButton = page.getByTestId('create-page-button'); | |
| if ( | |
| await createPageButton.isVisible({ timeout: 5000 }).catch(() => false) | |
| ) { | |
| await createPageButton.click(); | |
| const editorPage = new EditorPage(page); | |
| await expect(editorPage.editor).toBeVisible({ timeout: 10000 }); | |
| } | |
| const createPageButton = page.getByTestId('create-page-button'); | |
| await expect(createPageButton).toBeVisible({ timeout: 5000 }); | |
| await createPageButton.click(); | |
| const editorPage = new EditorPage(page); | |
| await expect(editorPage.editor).toBeVisible({ timeout: 10000 }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: e2e/tests/pages/page.spec.ts
Line: 28:36
Comment:
**logic:** Test silently passes if create-page-button is not visible, potentially hiding UI issues
```suggestion
const createPageButton = page.getByTestId('create-page-button');
await expect(createPageButton).toBeVisible({ timeout: 5000 });
await createPageButton.click();
const editorPage = new EditorPage(page);
await expect(editorPage.editor).toBeVisible({ timeout: 10000 });
```
How can I resolve this? If you propose a fix, please make it concise.| test('should type in the editor', async ({ page }) => { | ||
| const createPageButton = page.getByTestId('create-page-button'); | ||
| if ( | ||
| await createPageButton.isVisible({ timeout: 5000 }).catch(() => false) | ||
| ) { | ||
| await createPageButton.click(); |
There was a problem hiding this comment.
style: Code duplication - the page creation logic is repeated from the previous test
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: e2e/tests/pages/page.spec.ts
Line: 39:44
Comment:
**style:** Code duplication - the page creation logic is repeated from the previous test
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
apps/server/src/test/app.ts
Outdated
| await app.inject({ | ||
| method: 'POST', | ||
| url: '/client/v1/accounts/emails/register', | ||
| payload: { | ||
| email, | ||
| password, | ||
| name: 'Test User', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
logic: Registration request doesn't check for success response - if registration fails, login will also fail but error won't be clear. Should the registration response be validated before proceeding to login?
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/test/app.ts
Line: 45:53
Comment:
**logic:** Registration request doesn't check for success response - if registration fails, login will also fail but error won't be clear. Should the registration response be validated before proceeding to login?
How can I resolve this? If you propose a fix, please make it concise.
apps/server/src/test/app.ts
Outdated
| payload: { email, password }, | ||
| }); | ||
|
|
||
| const data = JSON.parse(loginResponse.body); |
There was a problem hiding this comment.
logic: JSON.parse can throw if response body is invalid JSON - consider wrapping in try/catch
| const data = JSON.parse(loginResponse.body); | |
| try { | |
| const data = JSON.parse(loginResponse.body); | |
| return data.type === 'success' ? data.token : null; | |
| } catch { | |
| return null; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/test/app.ts
Line: 61:61
Comment:
**logic:** `JSON.parse` can throw if response body is invalid JSON - consider wrapping in try/catch
```suggestion
try {
const data = JSON.parse(loginResponse.body);
return data.type === 'success' ? data.token : null;
} catch {
return null;
}
```
How can I resolve this? If you propose a fix, please make it concise.| POSTGRES_URL: postgresql://brainbox:brainbox@localhost:5432/brainbox | ||
| REDIS_URL: redis://localhost:6379/0 |
There was a problem hiding this comment.
logic: Environment variable inconsistency: POSTGRES_URL is defined but migration steps use DATABASE_URL
| POSTGRES_URL: postgresql://brainbox:brainbox@localhost:5432/brainbox | |
| REDIS_URL: redis://localhost:6379/0 | |
| DATABASE_URL: postgresql://brainbox:brainbox@localhost:5432/brainbox | |
| REDIS_URL: redis://localhost:6379/0 |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/test.yml
Line: 15:16
Comment:
**logic:** Environment variable inconsistency: `POSTGRES_URL` is defined but migration steps use `DATABASE_URL`
```suggestion
DATABASE_URL: postgresql://brainbox:brainbox@localhost:5432/brainbox
REDIS_URL: redis://localhost:6379/0
```
How can I resolve this? If you propose a fix, please make it concise.- Add editor alias and typeInEditor method to EditorPage - Add goToRegister method to LoginPage - Fix CI port mismatch (use port 4000 consistently) - Add DATABASE_URL env var alongside POSTGRES_URL - Use crypto.randomUUID for better test ID uniqueness - Extract registerTestUser helper to avoid duplication - Add registration response check and JSON.parse try/catch - Add wait after space creation in WorkspacePage - Log errors in global-setup on final retry - Remove redundant length assertion in collaborations test - Fix logout test to skip gracefully if UI elements missing
|
@greptile |
- Use npx vite preview directly instead of npm run serve - Remove verbose debug logging - Clean up wait-for-services step
…nsion The postgres:16-alpine image doesn't include pgvector extension, causing migration 00017_create_vector_extension to fail.
The preview server needs the same Cross-Origin headers as the dev server for SharedArrayBuffer to work, which is required by SQLite WASM. - Add preview config to vite.config.js with port 4000 and headers - Simplify workflow command since port is in config
No description provided.