Skip to content

feat(test-server): add acceptSnapshots API endpoint #38326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+100 −1
Open

feat(test-server): add acceptSnapshots API endpoint #38326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/playwright/src/isomorphic/testServerConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,10 @@ export class TestServerConnection implements TestServerInterface, TestServerInte
return await this._sendMessage('runTests', params);
}

async acceptSnapshots(params: Parameters<TestServerInterface['acceptSnapshots']>[0]): ReturnType<TestServerInterface['acceptSnapshots']> {
return await this._sendMessage('acceptSnapshots', params);
}

async findRelatedTestFiles(params: Parameters<TestServerInterface['findRelatedTestFiles']>[0]): ReturnType<TestServerInterface['findRelatedTestFiles']> {
return await this._sendMessage('findRelatedTestFiles', params);
}
Expand Down
10 changes: 10 additions & 0 deletions packages/playwright/src/isomorphic/testServerInterface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,16 @@ export interface TestServerInterface {
status: reporterTypes.FullResult['status'];
}>;

/**
* Accepts snapshot updates by copying actual snapshots to expected locations.
*/
acceptSnapshots(param: { paths: [string, string][]}): Promise<{
status: boolean;
accepted: number;
failed: number;
errors?: string[];
}>;

findRelatedTestFiles(params: {
files: string[];
}): Promise<{ testFiles: string[]; errors?: reporterTypes.TestError[]; }>;
Expand Down
27 changes: 27 additions & 0 deletions packages/playwright/src/runner/testServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
*/

import util from 'util';
import fs from 'fs';
import path from 'path';

import { installRootRedirect, openTraceInBrowser, openTraceViewerApp, startTraceViewerServer } from 'playwright-core/lib/server';
import { ManualPromise, gracefullyProcessExitDoNotHang, isUnderTest } from 'playwright-core/lib/utils';
Expand Down Expand Up @@ -135,6 +137,31 @@ export class TestServerDispatcher implements TestServerInterface {

async ping() {}

async acceptSnapshots(params: Parameters<TestServerInterface['acceptSnapshots']>[0]): ReturnType<TestServerInterface['acceptSnapshots']> {
Copy link
Member

@pavelfeldman pavelfeldman Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really a copyFile rather than accept snapshot endpoint. If we have it in this shape, we need to add some security considerations to prevent data loss and misuse.

Btw, do you know that there is a checkbox setting in the sidebar that allows accepting changed the snapshots on the test runs? Why not to just run it and then explore the diffs?

Copy link
Author

@rassie rassie Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback and the pointer, I'll have to explore that. From your description it sounds like it accepts all changed snapshots on a run, but if the run is restricted to a subset of tests it might be functionally equivalent to what I'm trying to do. Let me get back to you with that.

Copy link
Author

@rassie rassie Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've explored the checkbox for accepting the snapshots, apparently we were still running 1.50.1, so that I haven't seen it yet, thanks again for the pointer. I can say that in general this feature does what I need.

However, it's not a perfect fit. There is at least one signifant downside to current implementation: it can't accept a test snapshot without re-running the test first. Depending on number of tests, number of expected snapshots to accept and tests' runtime, reviewing test runs can (unnecessarily) be very time-consuming and mentally draining. My envisioned solution to the problem is to allow test snapshots to be accepted without re-running the tests.

In terms of improving the security of this PR's implementation, I see two options:

  1. We can validate in testServer that the paths are both located within the project. This is the simpler option, but it would probably still be a restricted copyFile at its core.
  2. We avoid using paths in testServer's API and derive paths from other properties. This would require re-using snapshot path templating functions from src/worker/testInfo.ts in src/runner/testServer.ts. The API could look something like this to allow for path calculation on the server side:
acceptSnapshots(params: {
  snapshots: Array<{
    projectName: string;
    testFilePath: string;
    titlePath: string[];
    snapshotName: string;
    retry?: number;
  }>;
})

That being said, while I consider "accept without re-running" a valid use-case and an improvement, I can fully understand if you'd consider current state good enough to not entertain change.

let accepted = 0;
let failed = 0;
const errors: string[] = [];

for (const [source, dest] of params.paths) {
try {
// Ensure destination directory exists before copying
await fs.promises.mkdir(path.dirname(dest), { recursive: true });
await fs.promises.copyFile(source, dest);
accepted++;
} catch (e) {
failed++;
errors.push(`Failed to copy ${source} to ${dest}: ${(e as Error).message}`);
}
}

return {
status: failed === 0,
accepted,
failed,
errors: errors.length > 0 ? errors : undefined,
};
}

async open(params: Parameters<TestServerInterface['open']>[0]): ReturnType<TestServerInterface['open']> {
if (isUnderTest())
return;
Expand Down
60 changes: 59 additions & 1 deletion tests/playwright-test/test-server.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@
import { test as baseTest, expect } from './ui-mode-fixtures';
import { TestServerConnection } from '../../packages/playwright/lib/isomorphic/testServerConnection';
import { playwrightCtConfigText } from './playwright-test-fixtures';
import ws from 'ws';
import type { TestChildProcess } from '../config/commonFixtures';
import ws from 'ws';
import fs from 'fs';
import path from 'path';

class WSTransport {
private _ws: ws.WebSocket;
Expand Down Expand Up @@ -328,3 +330,59 @@ test('pauseOnError no errors', async ({ startTestServer, writeFiles }) => {
expect(await testServerConnection.runTests({ pauseOnError: true, locations: [] })).toEqual({ status: 'passed' });
expect(testServerConnection.events.filter(e => e[0] === 'testPaused')).toEqual([]);
});

test('should accept snapshots via test server API', async ({ startTestServer, writeFiles }, testInfo) => {
await writeFiles({
'a.test.ts': `
import { test } from '@playwright/test';
test('pass', () => {});
`,
});

const testServerConnection = await startTestServer();

// Create source and destination files to test the copy API
const testDir = testInfo.outputPath();
const sourceFile = path.join(testDir, 'source', 'actual.txt');
const destFile = path.join(testDir, 'dest', 'expected.txt');

fs.mkdirSync(path.dirname(sourceFile), { recursive: true });
fs.writeFileSync(sourceFile, 'snapshot content');

// Accept the snapshot (copy source to dest)
const result = await testServerConnection.acceptSnapshots({
paths: [[sourceFile, destFile]]
});

// Verify the API response
expect(result.status).toBe(true);
expect(result.accepted).toBe(1);
expect(result.failed).toBe(0);
expect(result.errors).toBeUndefined();

// Verify the file was copied and destination directory was created
expect(fs.existsSync(destFile)).toBeTruthy();
expect(fs.readFileSync(destFile, 'utf-8')).toBe('snapshot content');
});

test('should report errors when accepting snapshots fails', async ({ startTestServer, writeFiles }, testInfo) => {
await writeFiles({
'a.test.ts': `
import { test } from '@playwright/test';
test('pass', () => {});
`,
});

const testServerConnection = await startTestServer();
const testDir = testInfo.outputPath();

// Try to copy from non-existent source
const result = await testServerConnection.acceptSnapshots({
paths: [[path.join(testDir, 'nonexistent.txt'), path.join(testDir, 'dest.txt')]]
});

expect(result.status).toBe(false);
expect(result.accepted).toBe(0);
expect(result.failed).toBe(1);
expect(result.errors).toHaveLength(1);
});