Add comprehensive Playwright E2E tests for database verification workflow#78
Conversation
…flow Agent-Logs-Url: https://github.com/MaximumTrainer/OpenDataMask/sessions/c0576442-108f-4e13-b8ac-78825dc03ec3 Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
…back Agent-Logs-Url: https://github.com/MaximumTrainer/OpenDataMask/sessions/c0576442-108f-4e13-b8ac-78825dc03ec3 Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
…gitignore Agent-Logs-Url: https://github.com/MaximumTrainer/OpenDataMask/sessions/c0576442-108f-4e13-b8ac-78825dc03ec3 Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a Playwright E2E test suite to exercise the database verification workflow against the existing verification sandbox environment.
Changes:
- Add Playwright configuration and dev dependency for running E2E tests.
- Introduce shared E2E fixtures/utilities plus multiple UI/API-driven workflow specs.
- Adjust Vitest config to avoid collecting
e2e/**tests and ignore Playwright output in git.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/vite.config.ts | Updates Vitest config to exclude E2E tests (but currently replaces default excludes). |
| frontend/playwright.config.ts | Adds Playwright runner configuration targeting the verification sandbox URL. |
| frontend/package.json | Adds @playwright/test as a dev dependency. |
| frontend/package-lock.json | Locks Playwright-related dependency tree. |
| frontend/e2e/odm-fixtures.ts | Adds shared E2E fixtures + API helpers + seeding utilities. |
| frontend/e2e/auth.spec.ts | Adds authentication-focused E2E coverage. |
| frontend/e2e/connections.spec.ts | Adds connection CRUD/validation E2E coverage. |
| frontend/e2e/masking-definition.spec.ts | Adds masking definition + mapping wizard E2E coverage. |
| frontend/e2e/job-execution.spec.ts | Adds job execution + transfer verification E2E coverage. |
| frontend/e2e/full-workflow.spec.ts | Adds a serial full “verification script” workflow E2E spec. |
| frontend/.gitignore | Ignores Playwright output directories. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { | ||
| ODM_USERNAME, | ||
| ODM_PASSWORD, | ||
| ODM_EMAIL, |
There was a problem hiding this comment.
ODM_EMAIL is imported but never used in this spec. This will fail the repo’s eslint script (no-unused-vars) and should be removed (or used if intended).
| ODM_EMAIL, |
| test('test connection button triggers connection test', async ({ authenticatedPage: page }) => { | ||
| // Seed a connection via API | ||
| let connId: number | ||
| try { | ||
| const conn = await apiCall(`/api/workspaces/${workspaceId}/connections`, { | ||
| method: 'POST', | ||
| body: { | ||
| name: 'test-btn-conn', | ||
| type: 'POSTGRESQL', | ||
| connectionString: `jdbc:postgresql://${SOURCE_DB.host}:${SOURCE_DB.port}/${SOURCE_DB.database}`, | ||
| username: SOURCE_DB.username, | ||
| password: SOURCE_DB.password, | ||
| isSource: true, | ||
| isDestination: false, | ||
| }, | ||
| token, | ||
| }) | ||
| connId = conn.id as number | ||
| } catch { |
There was a problem hiding this comment.
let connId: number is assigned in the try block but never used. This will be flagged by ESLint and should be removed (or asserted against to ensure the seeded connection is the one being tested).
| test('edit connection modal pre-fills existing data', async ({ authenticatedPage: page }) => { | ||
| // Seed a connection via API | ||
| const conn = await apiCall(`/api/workspaces/${workspaceId}/connections`, { | ||
| method: 'POST', | ||
| body: { | ||
| name: 'edit-test-conn', | ||
| type: 'POSTGRESQL', | ||
| connectionString: `jdbc:postgresql://${SOURCE_DB.host}:${SOURCE_DB.port}/${SOURCE_DB.database}`, | ||
| username: SOURCE_DB.username, | ||
| password: SOURCE_DB.password, | ||
| isSource: true, | ||
| isDestination: false, | ||
| }, | ||
| token, | ||
| }) |
There was a problem hiding this comment.
The conn variable returned from apiCall(...) is never used in this test. Keeping the assignment triggers ESLint’s unused-vars rule; remove the variable (just await apiCall(...)) or use it in an assertion.
| import { | ||
| ODM_USERNAME, | ||
| ODM_PASSWORD, | ||
| ODM_EMAIL, | ||
| SOURCE_DB, | ||
| TARGET_DB, | ||
| registerUser, | ||
| loginViaApi, | ||
| apiCall, | ||
| waitForPageHeading, | ||
| waitForLoadingDone, | ||
| waitForJobCompletion, | ||
| type IdResponse, | ||
| type WorkspaceResponse, | ||
| type ConnectionResponse, | ||
| type JobResponse, | ||
| type JobLogEntry, | ||
| type ConnectionTestResponse, | ||
| } from './odm-fixtures' |
There was a problem hiding this comment.
ODM_EMAIL is imported but never used in this spec. Since frontend runs eslint . ..., this will fail the lint step; remove the unused import.
| export async function registerUser(): Promise<void> { | ||
| try { | ||
| await apiCall('/api/auth/register', { | ||
| method: 'POST', | ||
| body: { username: ODM_USERNAME, email: ODM_EMAIL, password: ODM_PASSWORD }, | ||
| }) | ||
| } catch { | ||
| // User may already exist — safe to ignore. |
There was a problem hiding this comment.
registerUser currently swallows all failures from the register endpoint (including network errors / 5xx), which can make later test failures hard to diagnose. Consider only ignoring the expected “already exists” case (e.g. based on status/message), and rethrow unexpected errors.
| export async function registerUser(): Promise<void> { | |
| try { | |
| await apiCall('/api/auth/register', { | |
| method: 'POST', | |
| body: { username: ODM_USERNAME, email: ODM_EMAIL, password: ODM_PASSWORD }, | |
| }) | |
| } catch { | |
| // User may already exist — safe to ignore. | |
| function isAlreadyRegisteredError(error: unknown): boolean { | |
| if (!(error instanceof Error)) return false | |
| const message = error.message.toLowerCase() | |
| return ( | |
| message.includes('→ 409:') || | |
| message.includes('already exists') || | |
| message.includes('already registered') | |
| ) | |
| } | |
| export async function registerUser(): Promise<void> { | |
| try { | |
| await apiCall('/api/auth/register', { | |
| method: 'POST', | |
| body: { username: ODM_USERNAME, email: ODM_EMAIL, password: ODM_PASSWORD }, | |
| }) | |
| } catch (error) { | |
| if (!isAlreadyRegisteredError(error)) { | |
| throw error | |
| } |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export async function apiCall<T = Record<string, any>>( | ||
| path: string, | ||
| { method = 'GET', body, token }: ApiOptions = {} | ||
| ): Promise<T> { | ||
| const headers: Record<string, string> = { 'Content-Type': 'application/json' } | ||
| if (token) headers['Authorization'] = `Bearer ${token}` | ||
|
|
||
| const res = await fetch(`${API_BASE}${path}`, { | ||
| method, | ||
| headers, | ||
| body: body ? JSON.stringify(body) : undefined, | ||
| }) | ||
|
|
||
| if (!res.ok) { | ||
| const text = await res.text().catch(() => '') | ||
| throw new Error(`API ${method} ${path} → ${res.status}: ${text}`) | ||
| } | ||
|
|
||
| const text = await res.text() | ||
| return text ? JSON.parse(text) : ({} as T) | ||
| } |
There was a problem hiding this comment.
apiCall defaults T to Record<string, any> and returns ({} as T) for empty responses. This makes it easy for call sites to fall back to any + as number casts (which already happens in the specs) and conflicts with the PR description’s “no unsafe casts” goal. Consider requiring an explicit generic (default to unknown), and returning undefined/null for empty bodies so types stay honest.
| test: { | ||
| environment: 'node' | ||
| environment: 'node', | ||
| exclude: ['e2e/**', 'node_modules/**'] | ||
| } |
There was a problem hiding this comment.
test.exclude here overrides Vitest’s default exclude globs. With only ['e2e/**','node_modules/**'], other default excludes (e.g. dist/output/temp and config files) are no longer excluded, which can make Vitest pick up unintended test files or slow down collection. Consider extending configDefaults.exclude (from vitest/config) and appending e2e/** instead of replacing the list.
| test('expand table to view column generators section', async ({ authenticatedPage: page }) => { | ||
| // Seed a table config via API | ||
| const tbl = await apiCall(`/api/workspaces/${workspaceId}/tables`, { | ||
| method: 'POST', | ||
| body: { | ||
| connectionId: sourceConnectionId, | ||
| tableName: 'users_expand_test', | ||
| schemaName: 'public', | ||
| mode: 'MASK', | ||
| }, | ||
| token, | ||
| }) |
There was a problem hiding this comment.
const tbl = await apiCall(...) is declared but never used in this test, which will fail linting. Either remove the variable or use it to target the specific seeded table row (instead of always expanding the first table).
| waitForLoadingDone, | ||
| type JobResponse, | ||
| type JobLogEntry, | ||
| type ConnectionTestResponse, |
There was a problem hiding this comment.
ConnectionTestResponse is imported but never used in this file. This will be reported by ESLint; remove the unused type import.
| type ConnectionTestResponse, |
|
@copilot apply changes based on the comments in this thread |
…, extend vitest exclude defaults Agent-Logs-Url: https://github.com/MaximumTrainer/OpenDataMask/sessions/e32c6a4c-f45b-4dd1-adc8-c6f4b5e2b288 Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
Applied all 9 review comments in commit
|
@playwright/testas a dev dependency in the frontendplaywright.config.tsconfiguration filee2e/odm-fixtures.ts) — login, API setup, seed utilities with typed API responsese2e/auth.spec.ts— login/register page verification (7 tests)e2e/connections.spec.ts— source & destination database connection setup and test (10 tests)e2e/masking-definition.spec.ts— table configuration, column generators, data mapping wizard (14 tests)e2e/job-execution.spec.ts— job creation, monitoring, completion, and transfer verification (13 tests)e2e/full-workflow.spec.ts— end-to-end flow matching the verification script sequence (10 tests)configDefaults.excludespreadODM_EMAILimport from auth.spec.ts and full-workflow.spec.tsConnectionTestResponseimport from job-execution.spec.tsconnIdvariable in connections.spec.ts test connection testconnvariable in connections.spec.ts edit connection testtblvariable in masking-definition.spec.ts expand testapiCalldefault generic fromRecord<string, any>tounknownregisterUserto only swallow 409/already-exists errorsconfigDefaults.excludefrom vitest/config instead of replacing defaults