From b5082e10fd5e5ee6da36f8c168fe927a4552ab44 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Wed, 31 Jan 2024 15:33:38 -0800 Subject: [PATCH] fix: do not retry missing snapshot errors (#29272) When `updateSnapshots === 'missing'` we generate new expectations on the first attempt and don't retry the test afterwards instead of trying it retries-1 times and only writing new expectation on the last attempt. This logic infects all serial mode suites that contain the test with missing expectations, so they also will not be retried. Reference https://github.com/microsoft/playwright/issues/29073 --- packages/playwright/src/common/ipc.ts | 1 + .../src/matchers/toMatchSnapshot.ts | 4 +- packages/playwright/src/runner/dispatcher.ts | 15 +++++- packages/playwright/src/worker/testInfo.ts | 10 ++-- packages/playwright/src/worker/workerMain.ts | 3 +- .../to-have-screenshot.spec.ts | 50 +++++++++++++++++++ 6 files changed, 75 insertions(+), 8 deletions(-) diff --git a/packages/playwright/src/common/ipc.ts b/packages/playwright/src/common/ipc.ts index 17b1056a78..8bbd3319ad 100644 --- a/packages/playwright/src/common/ipc.ts +++ b/packages/playwright/src/common/ipc.ts @@ -84,6 +84,7 @@ export type TestEndPayload = { duration: number; status: TestStatus; errors: TestInfoError[]; + hasNonRetriableError: boolean; expectedStatus: TestStatus; annotations: { type: string, description?: string }[]; timeout: number; diff --git a/packages/playwright/src/matchers/toMatchSnapshot.ts b/packages/playwright/src/matchers/toMatchSnapshot.ts index 360d3f6d36..f65b324095 100644 --- a/packages/playwright/src/matchers/toMatchSnapshot.ts +++ b/packages/playwright/src/matchers/toMatchSnapshot.ts @@ -141,8 +141,6 @@ class SnapshotHelper { this.locator = locator; this.updateSnapshots = testInfo.config.updateSnapshots; - if (this.updateSnapshots === 'missing' && testInfo.retry < testInfo.project.retries) - this.updateSnapshots = 'none'; this.mimeType = mime.getType(path.basename(this.snapshotPath)) ?? 'application/octet-string'; this.comparator = getComparator(this.mimeType); @@ -206,7 +204,7 @@ class SnapshotHelper { return this.createMatcherResult(message, true); } if (this.updateSnapshots === 'missing') { - this.testInfo._failWithError(new Error(message), false /* isHardError */); + this.testInfo._failWithError(new Error(message), false /* isHardError */, false /* retriable */); return this.createMatcherResult('', true); } return this.createMatcherResult(message, false); diff --git a/packages/playwright/src/runner/dispatcher.ts b/packages/playwright/src/runner/dispatcher.ts index 6fb315114e..f922cf9555 100644 --- a/packages/playwright/src/runner/dispatcher.ts +++ b/packages/playwright/src/runner/dispatcher.ts @@ -243,6 +243,7 @@ class JobDispatcher { private _listeners: RegisteredListener[] = []; private _failedTests = new Set(); + private _failedWithNonRetriableError = new Set(); private _remainingByTestId = new Map(); private _dataByTestId = new Map }>(); private _parallelIndex = 0; @@ -293,10 +294,20 @@ class JobDispatcher { const isFailure = result.status !== 'skipped' && result.status !== test.expectedStatus; if (isFailure) this._failedTests.add(test); + if (params.hasNonRetriableError) + this._addNonretriableTestAndSerialModeParents(test); this._reportTestEnd(test, result); this._currentlyRunning = undefined; } + private _addNonretriableTestAndSerialModeParents(test: TestCase) { + this._failedWithNonRetriableError.add(test); + for (let parent: Suite | undefined = test.parent; parent; parent = parent.parent) { + if (parent._parallelMode === 'serial') + this._failedWithNonRetriableError.add(parent); + } + } + private _onStepBegin(params: StepBeginPayload) { const data = this._dataByTestId.get(params.testId); if (!data) { @@ -435,6 +446,8 @@ class JobDispatcher { const serialSuitesWithFailures = new Set(); for (const failedTest of this._failedTests) { + if (this._failedWithNonRetriableError.has(failedTest)) + continue; retryCandidates.add(failedTest); let outermostSerialSuite: Suite | undefined; @@ -442,7 +455,7 @@ class JobDispatcher { if (parent._parallelMode === 'serial') outermostSerialSuite = parent; } - if (outermostSerialSuite) + if (outermostSerialSuite && !this._failedWithNonRetriableError.has(outermostSerialSuite)) serialSuitesWithFailures.add(outermostSerialSuite); } diff --git a/packages/playwright/src/worker/testInfo.ts b/packages/playwright/src/worker/testInfo.ts index 5a940061cc..45fe34d9b0 100644 --- a/packages/playwright/src/worker/testInfo.ts +++ b/packages/playwright/src/worker/testInfo.ts @@ -69,6 +69,8 @@ export class TestInfoImpl implements TestInfo { _afterHooksStep: TestStepInternal | undefined; _onDidFinishTestFunction: (() => Promise) | undefined; + _hasNonRetriableError = false; + // ------------ TestInfo fields ------------ readonly testId: string; readonly repeatEachIndex: number; @@ -241,7 +243,7 @@ export class TestInfoImpl implements TestInfo { if (this.status === 'passed') this.status = 'skipped'; } else { - this._failWithError(error, true /* isHardError */); + this._failWithError(error, true /* isHardError */, true /* retriable */); return error; } } @@ -318,7 +320,7 @@ export class TestInfoImpl implements TestInfo { this._tracing.appendAfterActionForStep(stepId, errorForTrace, result.attachments); if (step.isSoft && result.error) - this._failWithError(result.error, false /* isHardError */); + this._failWithError(result.error, false /* isHardError */, true /* retriable */); } }; const parentStepList = parentStep ? parentStep.steps : this._steps; @@ -346,7 +348,9 @@ export class TestInfoImpl implements TestInfo { this.status = 'interrupted'; } - _failWithError(error: Error, isHardError: boolean) { + _failWithError(error: Error, isHardError: boolean, retriable: boolean) { + if (!retriable) + this._hasNonRetriableError = true; // Do not overwrite any previous hard errors. // Some (but not all) scenarios include: // - expect() that fails after uncaught exception. diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index 8c76f4091a..7311cb0bdf 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -169,7 +169,7 @@ export class WorkerMain extends ProcessRunner { // and unhandled errors - both lead to the test failing. This is good for regular tests, // so that you can, e.g. expect() from inside an event handler. The test fails, // and we restart the worker. - this._currentTest._failWithError(error, true /* isHardError */); + this._currentTest._failWithError(error, true /* isHardError */, true /* retriable */); // For tests marked with test.fail(), this might be a problem when unhandled error // is not coming from the user test code (legit failure), but from fixtures or test runner. @@ -624,6 +624,7 @@ function buildTestEndPayload(testInfo: TestInfoImpl): TestEndPayload { duration: testInfo.duration, status: testInfo.status!, errors: testInfo.errors, + hasNonRetriableError: testInfo._hasNonRetriableError, expectedStatus: testInfo.expectedStatus, annotations: testInfo.annotations, timeout: testInfo.timeout, diff --git a/tests/playwright-test/to-have-screenshot.spec.ts b/tests/playwright-test/to-have-screenshot.spec.ts index 3677231c4e..ef07b58090 100644 --- a/tests/playwright-test/to-have-screenshot.spec.ts +++ b/tests/playwright-test/to-have-screenshot.spec.ts @@ -74,6 +74,56 @@ test('should disable animations by default', async ({ runInlineTest }, testInfo) expect(result.exitCode).toBe(0); }); +test('should not retry missing expectation errors', async ({ runInlineTest }, testInfo) => { + const cssTransitionURL = pathToFileURL(path.join(__dirname, '../assets/css-transition.html')); + const result = await runInlineTest({ + ...playwrightConfig({ + retries: 2, + }), + 'a.spec.js': ` + const { test, expect } = require('@playwright/test'); + test('is a test', async ({ page }) => { + await page.goto('${cssTransitionURL}'); + await expect(page).toHaveScreenshot('foo.png', { timeout: 1000 }); + await expect(page).toHaveScreenshot('bar.png', { timeout: 1000 }); + }); + ` + }); + expect(result.output).not.toContain(`retry #`); + expect(result.output).toMatch(/A snapshot doesn't exist.*foo.*, writing actual./); + expect(result.output).toMatch(/A snapshot doesn't exist.*bar.*, writing actual./); + expect(result.exitCode).toBe(1); +}); + +test('should not retry serial mode suites with missing expectation errors', async ({ runInlineTest }, testInfo) => { + const cssTransitionURL = pathToFileURL(path.join(__dirname, '../assets/css-transition.html')); + const result = await runInlineTest({ + ...playwrightConfig({ + retries: 2, + }), + 'a.spec.js': ` + const { test, expect } = require('@playwright/test'); + test.describe.serial('outer', () => { + test('last', async ({ page }) => { + }); + test.describe('nested', () => { + test('is a test', async ({ page }) => { + await page.goto('${cssTransitionURL}'); + await expect(page).toHaveScreenshot({ timeout: 1000 }); + await expect(page).toHaveScreenshot({ timeout: 1000 }); + }); + test('last', async ({ page }) => { + }); + }); + }); + ` + }); + expect(result.output).not.toContain(`retry #`); + expect(result.output).toMatch(/A snapshot doesn't exist.*1.*, writing actual./); + expect(result.output).toMatch(/A snapshot doesn't exist.*2.*, writing actual./); + expect(result.exitCode).toBe(1); +}); + test.describe('expect config animations option', () => { test('disabled', async ({ runInlineTest }, testInfo) => { const cssTransitionURL = pathToFileURL(path.join(__dirname, '../assets/css-transition.html'));