fix: do not screenshot the same page again in afterAll (#35079)

packages/playwright/src/index.ts

@@ -524,7 +524,6 @@ type SnapshotRecorderMode = 'on' | 'off' | 'only-on-failure' | 'on-first-failure
 class SnapshotRecorder {
   private _ordinal = 0;
   private _temporary: string[] = [];
-  private _snapshottedSymbol = Symbol('snapshotted');
 
   constructor(
     private _artifactsRecorder: ArtifactsRecorder,
@@ -590,9 +589,11 @@ class SnapshotRecorder {
   }
 
   private async _snapshotPage(page: Page, temporary: boolean) {
-    if ((page as any)[this._snapshottedSymbol])
+    // Make sure we do not snapshot the same page twice for a single TestInfo,
+    // which is reused between beforeAll(s), test and afterAll(s).
+    if ((page as any)[this.testInfo._uniqueSymbol])
       return;
-    (page as any)[this._snapshottedSymbol] = true;
+    (page as any)[this.testInfo._uniqueSymbol] = true;
     try {
       const path = temporary ? this._createTemporaryArtifact(createGuid() + this._extension) : this._createAttachmentPath();
       await this._doSnapshot(page, path);

packages/playwright/src/worker/testInfo.ts

@@ -61,6 +61,7 @@ export class TestInfoImpl implements TestInfo {
   readonly _startWallTime: number;
   readonly _tracing: TestTracing;
   readonly _floatingPromiseScope: FloatingPromiseScope = new FloatingPromiseScope();
+  readonly _uniqueSymbol;
 
   _wasInterrupted = false;
   _lastStepId = 0;
@@ -148,6 +149,7 @@ export class TestInfoImpl implements TestInfo {
     this._startTime = monotonicTime();
     this._startWallTime = Date.now();
     this._requireFile = test?._requireFile ?? '';
+    this._uniqueSymbol = Symbol('testInfoUniqueSymbol');
 
     this.repeatEachIndex = workerParams.repeatEachIndex;
     this.retry = retry;

tests/playwright-test/playwright.artifacts.spec.ts

@@ -151,10 +151,8 @@ test('should work with screenshot: on', async ({ runInlineTest }, testInfo) => {
     '  test-finished-1.png',
     'artifacts-shared-shared-failing',
     '  test-failed-1.png',
-    '  test-failed-2.png',
     'artifacts-shared-shared-passing',
     '  test-finished-1.png',
-    '  test-finished-2.png',
     'artifacts-two-contexts',
     '  test-finished-1.png',
     '  test-finished-2.png',
@@ -185,7 +183,6 @@ test('should work with screenshot: only-on-failure', async ({ runInlineTest }, t
     '  test-failed-1.png',
     'artifacts-shared-shared-failing',
     '  test-failed-1.png',
-    '  test-failed-2.png',
     'artifacts-two-contexts-failing',
     '  test-failed-1.png',
     '  test-failed-2.png',
@@ -248,6 +245,37 @@ test('should work with screenshot: only-on-failure & fullPage', async ({ runInli
   expect.soft(screenshotFailure).toMatchSnapshot('screenshot-grid-fullpage.png');
 });
 
+test('should capture a single screenshot on failure when afterAll fails', async ({ runInlineTest }, testInfo) => {
+  const result = await runInlineTest({
+    'a.spec.ts': `
+      import { test, expect } from '@playwright/test';
+      let page;
+      test.use({ screenshot: 'only-on-failure' });
+      test.beforeAll(async ({ browser }) => {
+        page = await browser.newPage();
+      });
+      test.afterAll(async () => {
+        await page.setContent('this is afterAll');
+        expect(1).toBe(2);
+        await page.close();
+      });
+      test('passes', async () => {
+        await page.setContent('this is test');
+      });
+    `,
+  }, { workers: 1 });
+
+  expect(result.exitCode).toBe(1);
+  expect(result.passed).toBe(0);
+  expect(result.failed).toBe(1);
+  expect(listFiles(testInfo.outputPath('test-results'))).toEqual([
+    '.last-run.json',
+    'a-passes',
+    '  test-failed-1.png',
+  ]);
+});
+
+
 test('should work with trace: on', async ({ runInlineTest }, testInfo) => {
   const result = await runInlineTest({
     ...testFiles,