feat: Implemented playwright-reporter and helper#33
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an “assertive” helper API for test metadata and introduces a custom Playwright reporter that collects that metadata, uploads traces on failures, and reports results to an API with retry + disk fallback.
Changes:
- Added
assertivehelper (id/tags/owner/priority/type/custom fields/attachments) with framework auto-detection (Playwright vs Jest/Vitest). - Implemented Playwright reporter with CI detection, batching, retry logic, and pending-results persistence.
- Implemented trace upload via pre-signed URL when a Playwright test fails.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Bumps/records Playwright-related dependency versions. |
| apps/npm/tsconfig.json | Tweaks TS config for the npm app build (rootDir, Node types). |
| apps/npm/src/reporter/playwright/trace-handler.ts | Adds trace.zip detection and upload via pre-signed URL. |
| apps/npm/src/reporter/playwright/index.ts | Implements the Playwright reporter, batching, retries, CI detection, and pending-results fallback. |
| apps/npm/src/helper/index.ts | Adds the public assertive helper singleton API. |
| apps/npm/src/helper/flush.ts | Adds annotation parsing + global-store flush helper. |
| apps/npm/src/helper/context.ts | Adds framework detection + metadata routing (Playwright annotations vs global store). |
| apps/npm/package.json | Adds @playwright/test (currently as a devDependency). |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import type { | ||
| ReporterConfig, | ||
| ResolvedConfig, | ||
| TestRunPayload, | ||
| RunBatchPayload, | ||
| TestStatus, | ||
| CIInfo, | ||
| } from '../../helper/types'; | ||
| import { parseAnnotations } from '../../helper/flush'; | ||
| import { uploadTraceIfNeeded } from './trace-handler'; | ||
|
|
There was a problem hiding this comment.
This reporter imports several types from ../../helper/types, but there is no apps/npm/src/helper/types.ts in the repo, which will break compilation/publishing. Add the missing helper/types module or update these imports to the correct location.
| import type { | |
| ReporterConfig, | |
| ResolvedConfig, | |
| TestRunPayload, | |
| RunBatchPayload, | |
| TestStatus, | |
| CIInfo, | |
| } from '../../helper/types'; | |
| import { parseAnnotations } from '../../helper/flush'; | |
| import { uploadTraceIfNeeded } from './trace-handler'; | |
| import { parseAnnotations } from '../../helper/flush'; | |
| import { uploadTraceIfNeeded } from './trace-handler'; | |
| type TestStatus = 'passed' | 'failed' | 'skipped' | 'timed_out' | 'not_run'; | |
| interface ReporterConfig { | |
| apiUrl?: string; | |
| apiKey?: string; | |
| uploadTraces?: boolean; | |
| environment?: string; | |
| [key: string]: unknown; | |
| } | |
| interface ResolvedConfig { | |
| apiUrl: string; | |
| apiKey: string; | |
| uploadTraces: boolean; | |
| environment: string; | |
| [key: string]: unknown; | |
| } | |
| interface CIInfo { | |
| provider?: string; | |
| buildId?: string; | |
| buildUrl?: string; | |
| branch?: string; | |
| commitSha?: string; | |
| pullRequestNumber?: string | number; | |
| [key: string]: string | number | boolean | undefined; | |
| } | |
| interface TestRunPayload { | |
| [key: string]: unknown; | |
| } | |
| interface RunBatchPayload { | |
| [key: string]: unknown; | |
| } |
| import type { UploadUrlResponse } from '../../helper/types'; | ||
|
|
There was a problem hiding this comment.
UploadUrlResponse is imported from ../../helper/types, but that module doesn’t exist in the repo (no apps/npm/src/helper/types.ts), so this file won’t compile. Add the missing types module or update the import to the correct path.
| import type { UploadUrlResponse } from '../../helper/types'; | |
| interface UploadUrlResponse { | |
| uploadUrl: string; | |
| traceKey: string; | |
| } |
| "@playwright/test": "^1.59.1", | ||
| "@repo/eslint-config": "workspace:*", |
There was a problem hiding this comment.
@playwright/test is required at runtime by the published reporter (import ... from '@playwright/test/reporter'), but it’s added under devDependencies, which won’t be installed for consumers. Move it to peerDependencies (optionally peerDependenciesMeta.optional) or dependencies so the reporter can be required in downstream projects.
| existing = JSON.parse( | ||
| readFileSync(filePath, 'utf-8'), | ||
| ) as RunBatchPayload[]; |
There was a problem hiding this comment.
If the pending-results file exists but contains invalid JSON, JSON.parse(...) will throw and the outer try will skip persisting the new payload, potentially losing results. Consider handling JSON parse errors separately (e.g., fall back to [] and overwrite the file, or move the corrupt file aside) so new results are still saved.
| existing = JSON.parse( | |
| readFileSync(filePath, 'utf-8'), | |
| ) as RunBatchPayload[]; | |
| try { | |
| existing = JSON.parse( | |
| readFileSync(filePath, 'utf-8'), | |
| ) as RunBatchPayload[]; | |
| } catch (err) { | |
| console.warn( | |
| `[assertive] Pending results file ${PENDING_DIR}/${PENDING_FILE} contains invalid JSON and will be overwritten: ${ | |
| err instanceof Error ? err.message : String(err) | |
| }`, | |
| ); | |
| existing = []; | |
| } |
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const { expect } = require('expect'); | ||
| const state = expect.getState() as { | ||
| currentTestName?: string; | ||
| }; |
There was a problem hiding this comment.
GlobalStoreContext relies on require('expect'), but this package doesn’t declare expect as a dependency/peerDependency. In Vitest projects in particular, expect may be available only as a global, so this require can fail and all metadata falls back to the __default__ bucket (mixing tests). Consider using globalThis.expect when present, and/or documenting/declaring expect as a (peer) dependency for Jest/Vitest support.
| // eslint-disable-next-line @typescript-eslint/no-require-imports | |
| const { expect } = require('expect'); | |
| const state = expect.getState() as { | |
| currentTestName?: string; | |
| }; | |
| type ExpectLike = { | |
| getState(): { | |
| currentTestName?: string; | |
| }; | |
| }; | |
| const globalExpect = ( | |
| globalThis as typeof globalThis & { expect?: ExpectLike } | |
| ).expect; | |
| const expectInstance = | |
| globalExpect ?? | |
| (() => { | |
| // eslint-disable-next-line @typescript-eslint/no-require-imports | |
| const { expect } = require('expect'); | |
| return expect as ExpectLike | undefined; | |
| })(); | |
| if (!expectInstance || typeof expectInstance.getState !== 'function') { | |
| return undefined; | |
| } | |
| const state = expectInstance.getState(); |
| import type { Priority, TestType } from './types'; | ||
| import { getContext } from './context'; |
There was a problem hiding this comment.
This file imports ./types, but there is no apps/npm/src/helper/types.ts (or equivalent) in the repo, so tsc/bundling will fail with “Cannot find module './types'”. Add the missing types module (and export Priority, TestType, etc.) or update the import to the correct existing path.
| import type { AssertiveMetadata } from './types'; | ||
| import { drainGlobalStore } from './context'; | ||
|
|
There was a problem hiding this comment.
./types is imported here but no apps/npm/src/helper/types.ts (or similar) exists in the repository, so this new helper will not compile. Add the missing types module or correct the import path.
| import type { AssertiveMetadata } from './types'; | |
| import { drainGlobalStore } from './context'; | |
| import { drainGlobalStore } from './context'; | |
| export type AssertiveMetadata = { | |
| testId: string | undefined; | |
| tags: string[]; | |
| owner: string | undefined; | |
| priority: string | undefined; | |
| testType: string | undefined; | |
| customFields: Record<string, string>; | |
| attachments: Record<string, string>; | |
| }; |
| let traceUrl: string | undefined; | ||
| if ( | ||
| this.config.uploadTraces && | ||
| result.status === 'failed' && | ||
| result.attachments.length > 0 |
There was a problem hiding this comment.
uploadTraceIfNeeded is documented to return a traceKey, but the reporter stores it in a variable (and later payload field) named traceUrl. This looks like a key/URL contract mismatch and will be confusing/incorrect downstream. Rename to traceKey (and update the payload type) or change uploadTraceIfNeeded to return an actual URL.
| interface TestAttachment { | ||
| name: string; | ||
| contentType: string; | ||
| path?: string; | ||
| body?: Buffer; |
There was a problem hiding this comment.
Playwright reporter attachments can provide body as Buffer | string (not only Buffer). With the current TestAttachment typing and if (trace.body) { traceBuffer = trace.body; }, a string body would cause a runtime/type mismatch. Consider typing body?: Buffer | string and converting string bodies to a Buffer (or rejecting with a clear warning).
Implemented Playwright Reporter and Helper
AssertiveHelperclass with metadata methods and singleton export