fix(artifacts): only attach screenshot when it succeeds (#24406)

Fixes #24378.

packages/playwright-test/src/index.ts

@@ -532,6 +532,7 @@ class ArtifactsRecorder {
   private _temporaryScreenshots: string[] = [];
   private _reusedContexts = new Set<BrowserContext>();
   private _traceOrdinal = 0;
+  private _screenshotOrdinal = 0;
   private _screenshottedSymbol: symbol;
   private _startedCollectingArtifacts: symbol;
 
@@ -552,6 +553,10 @@ class ArtifactsRecorder {
     testInfo._onDidFinishTestFunction = () => this.didFinishTestFunction();
     this._captureTrace = shouldCaptureTrace(this._traceMode, testInfo) && !process.env.PW_TEST_DISABLE_TRACING;
 
+    // Since beforeAll(s), test and afterAll(s) reuse the same TestInfo, make sure we do not
+    // overwrite previous screenshots.
+    this._screenshotOrdinal = testInfo.attachments.filter(a => a.name === 'screenshot').length;
+
     // Process existing contexts.
     for (const browserType of [this._playwright.chromium, this._playwright.firefox, this._playwright.webkit]) {
       const promises: (Promise<void> | undefined)[] = [];
@@ -617,9 +622,15 @@ class ArtifactsRecorder {
         await Promise.all(context.pages().map(async page => {
           if ((page as any)[this._screenshottedSymbol])
             return;
+          try {
+            const screenshotPath = this._createScreenshotAttachmentPath();
             // Pass caret=initial to avoid any evaluations that might slow down the screenshot
             // and let the page modify itself from the problematic state it had at the moment of failure.
-          await page.screenshot({ ...this._screenshotOptions, timeout: 5000, path: this._addScreenshotAttachment(), caret: 'initial' }).catch(() => {});
+            await page.screenshot({ ...this._screenshotOptions, timeout: 5000, path: screenshotPath, caret: 'initial' });
+            this._testInfo.attachments.push({ name: 'screenshot', path: screenshotPath, contentType: 'image/png' });
+          } catch {
+            // Screenshot may fail, just ignore.
+          }
         }));
       }
     }).concat(leftoverApiRequests.map(async context => {
@@ -630,10 +641,16 @@ class ArtifactsRecorder {
     // Either remove or attach temporary screenshots for contexts closed before
     // collecting the test trace.
     await Promise.all(this._temporaryScreenshots.map(async file => {
-      if (captureScreenshots)
-        await fs.promises.rename(file, this._addScreenshotAttachment()).catch(() => {});
-      else
+      if (!captureScreenshots) {
         await fs.promises.unlink(file).catch(() => {});
+        return;
+      }
+      try {
+        const screenshotPath = this._createScreenshotAttachmentPath();
+        await fs.promises.rename(file, screenshotPath);
+        this._testInfo.attachments.push({ name: 'screenshot', path: screenshotPath, contentType: 'image/png' });
+      } catch {
+      }
     }));
 
     // Collect test trace.
@@ -669,11 +686,11 @@ class ArtifactsRecorder {
     }
   }
 
-  private _addScreenshotAttachment() {
+  private _createScreenshotAttachmentPath() {
     const testFailed = this._testInfo.status !== this._testInfo.expectedStatus;
-    const index = this._testInfo.attachments.filter(a => a.name === 'screenshot').length + 1;
+    const index = this._screenshotOrdinal + 1;
+    ++this._screenshotOrdinal;
     const screenshotPath = this._testInfo.outputPath(`test-${testFailed ? 'failed' : 'finished'}-${index}.png`);
-    this._testInfo.attachments.push({ name: 'screenshot', path: screenshotPath, contentType: 'image/png' });
     return screenshotPath;
   }
 
@@ -681,11 +698,15 @@ class ArtifactsRecorder {
     if ((page as any)[this._screenshottedSymbol])
       return;
     (page as any)[this._screenshottedSymbol] = true;
+    try {
       const screenshotPath = path.join(this._artifactsDir, createGuid() + '.png');
-    this._temporaryScreenshots.push(screenshotPath);
       // Pass caret=initial to avoid any evaluations that might slow down the screenshot
       // and let the page modify itself from the problematic state it had at the moment of failure.
       await page.screenshot({ ...this._screenshotOptions, timeout: 5000, path: screenshotPath, caret: 'initial' }).catch(() => {});
+      this._temporaryScreenshots.push(screenshotPath);
+    } catch {
+      // Screenshot may fail, just ignore.
+    }
   }
 
   private async _screenshotOnTestFailure() {

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

@@ -705,3 +705,29 @@ test('should not throw when attachment is missing', async ({ runInlineTest }, te
   const trace = await parseTrace(testInfo.outputPath('test-results', 'a-passes', 'trace.zip'));
   expect(trace.actionTree).toContain('attach "screenshot"');
 });
+
+test('should not throw when screenshot on failure fails', async ({ runInlineTest, server }, testInfo) => {
+  const result = await runInlineTest({
+    'playwright.config.ts': `
+      module.exports = { use: { trace: 'on', screenshot: 'on' } };
+    `,
+    'a.spec.ts': `
+      import { test, expect } from '@playwright/test';
+      test('has pdf page', async ({ page }) => {
+        await page.goto("${server.EMPTY_PAGE}");
+        await page.setContent('<a href="/empty.pdf" target="blank">open me!</a>');
+        const downloadPromise = page.waitForEvent('download');
+        await page.click('a');
+        const download = await downloadPromise;
+        expect(download.suggestedFilename()).toBe('empty.pdf');
+      });
+    `,
+  }, { workers: 1 });
+
+  expect(result.exitCode).toBe(0);
+  expect(result.passed).toBe(1);
+  const trace = await parseTrace(testInfo.outputPath('test-results', 'a-has-pdf-page', 'trace.zip'));
+  const attachedScreenshots = trace.actionTree.filter(s => s === `  attach "screenshot"`);
+  // One screenshot for the page, no screenshot for pdf page since it should have failed.
+  expect(attachedScreenshots.length).toBe(1);
+});