diff --git a/packages/playwright/src/common/ipc.ts b/packages/playwright/src/common/ipc.ts index 17b1056a784c9..8bbd3319ada2c 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 360d3f6d36970..f65b32409583d 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 6fb315114e6fc..f922cf95554ee 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 5a940061cc241..45fe34d9b063f 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 8c76f4091a09b..7311cb0bdf994 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 3677231c4ee12..ef07b58090391 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'));