diff --git a/packages/playwright-test/src/index.ts b/packages/playwright-test/src/index.ts index 125f9fb799..301ae79cfa 100644 --- a/packages/playwright-test/src/index.ts +++ b/packages/playwright-test/src/index.ts @@ -532,6 +532,7 @@ class ArtifactsRecorder { private _temporaryScreenshots: string[] = []; private _reusedContexts = new Set(); 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 | undefined)[] = []; @@ -617,9 +622,15 @@ class ArtifactsRecorder { await Promise.all(context.pages().map(async page => { if ((page as any)[this._screenshottedSymbol]) return; - // 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(() => {}); + 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: 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; - 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(() => {}); + try { + const screenshotPath = path.join(this._artifactsDir, createGuid() + '.png'); + // 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() { diff --git a/tests/playwright-test/playwright.trace.spec.ts b/tests/playwright-test/playwright.trace.spec.ts index 74962f2cfe..bd44e027dd 100644 --- a/tests/playwright-test/playwright.trace.spec.ts +++ b/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('open me!'); + 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); +});