From 739adf5c2ad5ba6b9c2fe1e66d48e7eec6295d4e Mon Sep 17 00:00:00 2001 From: Devin Rousso Date: Tue, 25 Aug 2026 10:58:11 -0600 Subject: [PATCH] feat(ui-mode): update individual snapshots reviewing image diffs currently requires rerunning tests with snapshot updates enabled add a button for each diff that copies the actual image over the expected snapshot --- .../src/isomorphic/testServerConnection.ts | 4 ++ .../src/isomorphic/testServerInterface.ts | 5 ++ packages/playwright/src/runner/testServer.ts | 40 +++++++++--- .../trace-viewer/src/ui/attachmentsTab.css | 14 ++++ .../trace-viewer/src/ui/attachmentsTab.tsx | 64 +++++++++++++++++-- .../trace-viewer/src/ui/uiModeTraceView.tsx | 43 +++++++++++-- packages/trace-viewer/src/ui/uiModeView.tsx | 1 + packages/trace-viewer/src/ui/workbench.tsx | 13 +++- tests/playwright-test/ui-mode-trace.spec.ts | 41 +++++++++--- 9 files changed, 193 insertions(+), 32 deletions(-) diff --git a/packages/playwright/src/isomorphic/testServerConnection.ts b/packages/playwright/src/isomorphic/testServerConnection.ts index 17159a4edfb2f..748c0b3a3677e 100644 --- a/packages/playwright/src/isomorphic/testServerConnection.ts +++ b/packages/playwright/src/isomorphic/testServerConnection.ts @@ -219,6 +219,10 @@ export class TestServerConnection implements TestServerInterface, TestServerInte return await this._sendMessage('clearCache', params); } + async updateSnapshot(params: Parameters[0]): ReturnType { + return await this._sendMessage('updateSnapshot', params); + } + async listFiles(params: Parameters[0]): ReturnType { return await this._sendMessage('listFiles', params); } diff --git a/packages/playwright/src/isomorphic/testServerInterface.ts b/packages/playwright/src/isomorphic/testServerInterface.ts index 2450cbdc67ea8..dfdb8a548c526 100644 --- a/packages/playwright/src/isomorphic/testServerInterface.ts +++ b/packages/playwright/src/isomorphic/testServerInterface.ts @@ -57,6 +57,11 @@ export interface TestServerInterface { clearCache(params: {}): Promise; + updateSnapshot(params: { + actualPath: string; + expectedPath: string; + }): Promise; + listFiles(params: { projects?: string[]; }): Promise<{ diff --git a/packages/playwright/src/runner/testServer.ts b/packages/playwright/src/runner/testServer.ts index 5a31cd6f98bde..03a2e27364040 100644 --- a/packages/playwright/src/runner/testServer.ts +++ b/packages/playwright/src/runner/testServer.ts @@ -14,13 +14,15 @@ * limitations under the License. */ -import util from 'util'; import debug from 'debug'; +import fs from 'fs'; import open from 'open'; +import util from 'util'; import { server as coreServer } from 'playwright-core/lib/coreBundle'; import { ManualPromise } from '@isomorphic/manualPromise'; import { isUnderTest } from '@utils/debug'; +import { isPathInside } from '@utils/fileUtils'; import { HttpServer } from '@utils/httpServer'; import { gracefullyProcessExitDoNotHang } from '@utils/processLauncher'; @@ -49,6 +51,19 @@ const originalStderrWrite = process.stderr.write; const originalStdinIsTTY = process.stdin.isTTY; +function allowedFileRoots(configLocation: ConfigLocation, testRunner?: TestRunner): string[] { + const roots = new Set([process.cwd(), configLocation.configDir]); + const config = testRunner?.lastLoadedConfig(); + if (config) { + for (const project of config.projects) { + roots.add(project.project.outputDir); + roots.add(project.project.snapshotDir); + roots.add(project.project.testDir); + } + } + return [...roots]; +} + class TestServer { private _configLocation: ConfigLocation; private _configCLIOverrides: ipc.ConfigCLIOverrides; @@ -70,15 +85,7 @@ class TestServer { } private _allowedFileRoots(): string[] { - const roots = new Set([process.cwd(), this._configLocation.configDir]); - const config = this._dispatcher?._testRunner.lastLoadedConfig(); - if (config) { - for (const project of config.projects) { - roots.add(project.project.outputDir); - roots.add(project.project.testDir); - } - } - return [...roots]; + return allowedFileRoots(this._configLocation, this._dispatcher?._testRunner); } async stop() { @@ -112,6 +119,7 @@ export type RunTestsParams = { export class TestServerDispatcher implements TestServerInterface { readonly transport: Transport; + private _configLocation: ConfigLocation; private _serializer: string | undefined; private _closeOnDisconnect = false; _testRunner: TestRunner; @@ -119,6 +127,7 @@ export class TestServerDispatcher implements TestServerInterface { readonly _dispatchEvent: TestServerInterfaceEventEmitters['dispatchEvent']; constructor(configLocation: ConfigLocation, configCLIOverrides: ipc.ConfigCLIOverrides) { + this._configLocation = configLocation; this._testRunner = new TestRunner(configLocation, configCLIOverrides); this.transport = { onconnect: () => {}, @@ -195,6 +204,17 @@ export class TestServerDispatcher implements TestServerInterface { await this._testRunner.clearCache(); } + async updateSnapshot(params: Parameters[0]): ReturnType { + const config = this._testRunner.lastLoadedConfig(); + if (!config) + throw new Error('Cannot update snapshot before loading the configuration'); + if (!config.projects.some(project => isPathInside(project.project.outputDir, params.actualPath))) + throw new Error('Actual snapshot path is outside of the configured output directories'); + if (!allowedFileRoots(this._configLocation, this._testRunner).some(root => isPathInside(root, params.expectedPath))) + throw new Error('Expected snapshot path is outside of the allowed file roots'); + await fs.promises.copyFile(params.actualPath, params.expectedPath); + } + async listFiles(params: Parameters[0]): ReturnType { const { reporter, report } = await this._collectingReporter(); const { status } = await this._testRunner.listFiles(reporter, params.projects); diff --git a/packages/trace-viewer/src/ui/attachmentsTab.css b/packages/trace-viewer/src/ui/attachmentsTab.css index 7d487bb3f246c..b032fb01e75f0 100644 --- a/packages/trace-viewer/src/ui/attachmentsTab.css +++ b/packages/trace-viewer/src/ui/attachmentsTab.css @@ -36,6 +36,20 @@ margin-top: 10px; } +.attachments-image-diff-header { + position: relative; +} + +.attachments-update-snapshot { + background-color: var(--vscode-editor-inactiveSelectionBackground); + font-weight: normal; + padding: 4px 12px; + position: absolute; + right: 5px; + text-transform: none; + top: 5px; +} + .attachment-item { margin: 4px 8px; } diff --git a/packages/trace-viewer/src/ui/attachmentsTab.tsx b/packages/trace-viewer/src/ui/attachmentsTab.tsx index e69a9db980af9..3a81ddda5aa39 100644 --- a/packages/trace-viewer/src/ui/attachmentsTab.tsx +++ b/packages/trace-viewer/src/ui/attachmentsTab.tsx @@ -22,11 +22,14 @@ import { CodeMirrorWrapper, lineHeight } from '@web/components/codeMirrorWrapper import { isTextualMimeType } from '@isomorphic/mimeType'; import { Expandable } from '@web/components/expandable'; import { linkifyText } from '@web/renderUtils'; +import { ToolbarButton } from '@web/components/toolbarButton'; import { clsx, useFlash } from '@web/uiUtils'; import { useTraceModel } from './traceModelContext'; import type { Attachment, TraceModel } from '@isomorphic/trace/traceModel'; +export type UpdateSnapshot = (params: { actualPath: string, expectedPath: string }) => Promise; + type ExpandableAttachmentProps = { attachment: Attachment; reveal: any; @@ -90,9 +93,45 @@ const ExpandableAttachment: React.FunctionComponent = ; }; +function UpdateSnapshotButton({ actualPath, expectedPath, onUpdateSnapshot }: { + actualPath: string, + expectedPath: string, + onUpdateSnapshot: UpdateSnapshot, +}) { + const [saving, setSaving] = React.useState(false); + const [saved, triggerSavedFlash] = useFlash(); + const [error, setError] = React.useState(); + + const updateSnapshot = React.useCallback(async () => { + setSaving(true); + setError(undefined); + try { + await onUpdateSnapshot({ actualPath, expectedPath }); + triggerSavedFlash(); + } catch (error) { + setError(error instanceof Error ? error.message : String(error)); + } finally { + setSaving(false); + } + }, [actualPath, expectedPath, onUpdateSnapshot, triggerSavedFlash]); + + const label = error ? 'Retry save' : 'Save actual as expected'; + + return {label}; +} + export const AttachmentsTab: React.FunctionComponent<{ + localAttachmentPaths?: Map, revealedAttachmentCallId?: { callId: string }, -}> = ({ revealedAttachmentCallId }) => { + onUpdateSnapshot?: UpdateSnapshot, +}> = ({ localAttachmentPaths, revealedAttachmentCallId, onUpdateSnapshot }) => { const model = useTraceModel(); const { diffMap, screenshots, attachments } = React.useMemo(() => { const attachments = new Set(model?.visibleAttachments ?? []); @@ -122,16 +161,27 @@ export const AttachmentsTab: React.FunctionComponent<{ return ; return
- {[...diffMap.values()].map(({ expected, actual, diff }) => { - return <> - {expected && actual &&
Image diff
} - {expected && actual && { + if (!expected || !actual) + return null; + const expectedPath = localAttachmentPaths?.get(expected) || expected.path; + const actualPath = localAttachmentPaths?.get(actual) || actual.path; + return +
+ Image diff + {onUpdateSnapshot && expectedPath && actualPath && } +
+ } - ; + }} /> + ; })} {screenshots.size ?
Screenshots
: undefined} {[...screenshots.values()].map((a, i) => { diff --git a/packages/trace-viewer/src/ui/uiModeTraceView.tsx b/packages/trace-viewer/src/ui/uiModeTraceView.tsx index b167ac74adc11..42f0fb6159c42 100644 --- a/packages/trace-viewer/src/ui/uiModeTraceView.tsx +++ b/packages/trace-viewer/src/ui/uiModeTraceView.tsx @@ -21,19 +21,25 @@ import '@web/third_party/vscode/codicon.css'; import type * as reporterTypes from 'playwright/types/testReporter'; import React from 'react'; import type { ContextEntry } from '@isomorphic/trace/entries'; -import type { SourceLocation } from '@isomorphic/trace/traceModel'; +import type { Attachment, SourceLocation } from '@isomorphic/trace/traceModel'; import { TraceModel } from '@isomorphic/trace/traceModel'; +import type { UpdateSnapshot } from './attachmentsTab'; import { Workbench } from './workbench'; export const TraceView: React.FC<{ item: { treeItem?: TreeItem, testFile?: SourceLocation, testCase?: reporterTypes.TestCase }, rootDir?: string, onOpenExternally?: (location: SourceLocation) => void, + onUpdateSnapshot?: UpdateSnapshot, revealSource?: boolean, pathSeparator: string, onModelChange?: (model: TraceModel | undefined) => void, -}> = ({ item, rootDir, onOpenExternally, revealSource, pathSeparator, onModelChange }) => { - const [model, setModel] = React.useState<{ model: TraceModel, isLive: boolean } | undefined>(undefined); +}> = ({ item, rootDir, onOpenExternally, onUpdateSnapshot, revealSource, pathSeparator, onModelChange }) => { + const [model, setModel] = React.useState<{ + model: TraceModel, + isLive: boolean, + localAttachmentPaths?: Map, + } | undefined>(undefined); const [counter, setCounter] = React.useState(0); const pollTimer = React.useRef(null); @@ -55,7 +61,13 @@ export const TraceView: React.FC<{ // Test finished. const attachment = result && result.duration >= 0 && result.attachments.find(a => a.name === 'trace'); if (attachment && attachment.path) { - loadSingleTraceFile(attachment.path, result.startTime.getTime()).then(model => setModel({ model, isLive: false })); + loadSingleTraceFile(attachment.path, result.startTime.getTime()).then(model => { + setModel({ + model, + isLive: false, + localAttachmentPaths: localAttachmentPathsFromResult(model, result), + }); + }); return; } @@ -95,6 +107,7 @@ export const TraceView: React.FC<{ return ; }; +function localAttachmentPathsFromResult(model: TraceModel, testResult: reporterTypes.TestResult): Map { + const pathsByAttachment = new Map(); + for (const attachment of testResult.attachments) { + if (!attachment.path) + continue; + const key = JSON.stringify([attachment.name, attachment.contentType]); + const paths = pathsByAttachment.get(key) || []; + paths.push(attachment.path); + pathsByAttachment.set(key, paths); + } + + const localAttachmentPaths = new Map(); + for (const attachment of model.attachments) { + const key = JSON.stringify([attachment.name, attachment.contentType]); + const path = pathsByAttachment.get(key)?.shift(); + if (path) + localAttachmentPaths.set(attachment, path); + } + return localAttachmentPaths; +} + const outputDirForTestCase = (testCase: reporterTypes.TestCase): string | undefined => { for (let suite: reporterTypes.Suite | undefined = testCase.parent; suite; suite = suite.parent) { if (suite.project()) diff --git a/packages/trace-viewer/src/ui/uiModeView.tsx b/packages/trace-viewer/src/ui/uiModeView.tsx index 3bf380dd2778b..2506e3e116850 100644 --- a/packages/trace-viewer/src/ui/uiModeView.tsx +++ b/packages/trace-viewer/src/ui/uiModeView.tsx @@ -484,6 +484,7 @@ export const UIModeView: React.FC<{}> = ({ rootDir={testModel?.config?.rootDir} revealSource={revealSource} onOpenExternally={location => testServerConnection?.openNoReply({ location: { file: location.file, line: location.line, column: location.column } })} + onUpdateSnapshot={testServerConnection ? params => testServerConnection.updateSnapshot(params) : undefined} />
} diff --git a/packages/trace-viewer/src/ui/workbench.tsx b/packages/trace-viewer/src/ui/workbench.tsx index ab31fc558b638..4f09bd55c8d1c 100644 --- a/packages/trace-viewer/src/ui/workbench.tsx +++ b/packages/trace-viewer/src/ui/workbench.tsx @@ -21,7 +21,7 @@ import { CallTab } from './callTab'; import { LogTab } from './logTab'; import { ErrorsTab, useErrorsTabModel } from './errorsTab'; import { ConsoleTab, useConsoleTabModel } from './consoleTab'; -import type { TraceModel, SourceLocation, SourceModel } from '@isomorphic/trace/traceModel'; +import type { Attachment, TraceModel, SourceLocation, SourceModel } from '@isomorphic/trace/traceModel'; import type { ActionEntry } from '@isomorphic/trace/entries'; import { NetworkTab, useNetworkTabModel } from './networkTab'; import { SnapshotTabsView } from './snapshotTab'; @@ -32,6 +32,7 @@ import { Timeline } from './timeline'; import { usePlayback, PlaybackScrubber } from './playbackControl'; import { MetadataView } from './metadataView'; import { AttachmentsTab } from './attachmentsTab'; +import type { UpdateSnapshot } from './attachmentsTab'; import { AnnotationsTab } from './annotationsTab'; import type { Boundaries } from './geometry'; import { InspectorTab } from './inspectorTab'; @@ -52,6 +53,7 @@ import type { TreeState } from '@web/components/treeView'; export type WorkbenchProps = { model: TraceModel | undefined; + localAttachmentPaths?: Map; showSourcesFirst?: boolean; rootDir?: string; fallbackLocation?: SourceLocation; @@ -61,6 +63,7 @@ export type WorkbenchProps = { defaultAnnotations?: TestAnnotation[]; inert?: boolean; onOpenExternally?: (location: SourceLocation) => void; + onUpdateSnapshot?: UpdateSnapshot; revealSource?: boolean; testRunMetadata?: MetadataWithCommitInfo; }; @@ -73,7 +76,7 @@ export const Workbench: React.FunctionComponent = props => { }; const PartitionedWorkbench: React.FunctionComponent = props => { - const { partition, model, showSourcesFirst, rootDir, fallbackLocation, isLive, hideTimeline, status, inert, onOpenExternally, revealSource, testRunMetadata } = props; + const { partition, model, localAttachmentPaths, showSourcesFirst, rootDir, fallbackLocation, isLive, hideTimeline, status, inert, onOpenExternally, onUpdateSnapshot, revealSource, testRunMetadata } = props; // Default annotations come from the test model before the test runs, shown for the empty workbench / trace. const annotations = model?.annotations ?? props.defaultAnnotations; @@ -282,7 +285,11 @@ const PartitionedWorkbench: React.FunctionComponent + render: () => }; const tabs: TabbedPaneTabModel[] = [ diff --git a/tests/playwright-test/ui-mode-trace.spec.ts b/tests/playwright-test/ui-mode-trace.spec.ts index 14aa61d3e93a2..63225a69d379a 100644 --- a/tests/playwright-test/ui-mode-trace.spec.ts +++ b/tests/playwright-test/ui-mode-trace.spec.ts @@ -242,18 +242,24 @@ test('should show snapshots for steps', { }); test('should show image diff', async ({ runUITest }) => { - const { page } = await runUITest({ + const firstExpected = createImage(100, 100, 255, 0, 0); + const secondExpected = createImage(100, 100, 0, 255, 0); + const { page, testProcess } = await runUITest({ 'playwright.config.js': ` module.exports = { - snapshotPathTemplate: '{arg}{ext}' + snapshotPathTemplate: 'snapshots/{testFilePath}/{arg}{ext}' }; `, - 'snapshot.png': createImage(100, 100, 255, 0, 0), + 'snapshots/a.test.ts/first.png': firstExpected, + 'snapshots/a.test.ts/second.png': secondExpected, 'a.test.ts': ` import { test, expect } from '@playwright/test'; test('vrt test', async ({ page }) => { await page.setViewportSize({ width: 100, height: 100 }); - await expect(page).toHaveScreenshot('snapshot.png', { timeout: 2000 }); + await page.setContent(''); + await expect.soft(page).toHaveScreenshot('first.png', { timeout: 2000 }); + await page.setContent(''); + await expect.soft(page).toHaveScreenshot('second.png', { timeout: 2000 }); }); `, }); @@ -262,10 +268,29 @@ test('should show image diff', async ({ runUITest }) => { await expect(page.getByTestId('workbench-run-status')).toContainText('Failed'); await page.getByText(/Attachments/).click(); - await expect(page.getByText('Diff', { exact: true })).toBeVisible(); - await expect(page.getByText('Actual', { exact: true })).toBeVisible(); - await expect(page.getByText('Expected', { exact: true })).toBeVisible(); - await expect(page.getByTestId('test-result-image-mismatch').locator('img')).toBeVisible(); + await expect(page.getByText('Diff', { exact: true })).toHaveCount(2); + await expect(page.getByText('Actual', { exact: true })).toHaveCount(2); + await expect(page.getByText('Expected', { exact: true })).toHaveCount(2); + await expect(page.getByTestId('test-result-image-mismatch')).toHaveCount(2); + + const secondActual = await page.getByRole('link', { name: 'second-actual.png' }).evaluate(async link => { + const response = await fetch((link as HTMLAnchorElement).href); + return [...new Uint8Array(await response.arrayBuffer())]; + }); + expect(Buffer.from(secondActual)).not.toEqual(secondExpected); + + const updateSnapshots = page.locator('.attachments-update-snapshot'); + await expect(updateSnapshots).toHaveCount(2); + await expect(updateSnapshots).toHaveText(['Save actual as expected', 'Save actual as expected']); + await updateSnapshots.nth(1).click(); + await expect(updateSnapshots).toHaveText(['Save actual as expected', 'Save actual as expected']); + await expect(updateSnapshots.nth(1).locator('.codicon-check')).toBeVisible(); + await expect(updateSnapshots.nth(1)).toBeEnabled(); + await expect(updateSnapshots.nth(1).locator('.codicon-check')).toBeHidden(); + + const snapshotDir = path.join(testProcess.params.cwd!, 'snapshots', 'a.test.ts'); + expect(fs.readFileSync(path.join(snapshotDir, 'first.png'))).toEqual(firstExpected); + expect(fs.readFileSync(path.join(snapshotDir, 'second.png'))).toEqual(Buffer.from(secondActual)); }); test('should show screenshot', async ({ runUITest }) => {