From a411dfc39bd0ff992a0531458430315832b4637e Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Mon, 30 Jan 2023 23:34:12 +0400 Subject: [PATCH 1/4] fix: swallow expect.soft errors inside successful toPass matcher Fixes #20437 --- .../playwright-test/src/common/testInfo.ts | 12 +++++++ .../playwright-test/src/matchers/matchers.ts | 17 +++++++++- tests/playwright-test/expect-to-pass.spec.ts | 31 ++++++++++++++++++- 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/packages/playwright-test/src/common/testInfo.ts b/packages/playwright-test/src/common/testInfo.ts index 5fb6bd9cd3bc5..a40aeebe74bfe 100644 --- a/packages/playwright-test/src/common/testInfo.ts +++ b/packages/playwright-test/src/common/testInfo.ts @@ -246,6 +246,18 @@ export class TestInfoImpl implements TestInfo { this.errors.push(error); } + _saveTestInfo() { + return { + status: this.status, + errors: this.errors.slice(), + } + } + + _restoreTestInfo(state: { status: TestStatus, errors: TestInfoError[] }) { + this.status = state.status; + this.errors = state.errors.slice(); + } + async _runAsStep(cb: () => Promise, stepInfo: Omit): Promise { const step = this._addStep(stepInfo); try { diff --git a/packages/playwright-test/src/matchers/matchers.ts b/packages/playwright-test/src/matchers/matchers.ts index 6204dd339f180..bc81b5ce94bbc 100644 --- a/packages/playwright-test/src/matchers/matchers.ts +++ b/packages/playwright-test/src/matchers/matchers.ts @@ -19,6 +19,7 @@ import type { FrameExpectOptions } from 'playwright-core/lib/client/types'; import { colors } from 'playwright-core/lib/utilsBundle'; import type { Expect } from '../common/types'; import { expectTypes, callLogText } from '../util'; +import { currentTestInfo } from '../common/globals'; import { toBeTruthy } from './toBeTruthy'; import { toEqual } from './toEqual'; import { toExpectedTextValues, toMatchText } from './toMatchText'; @@ -317,11 +318,22 @@ export async function toPass( timeout?: number, } = {}, ) { + const testInfo = currentTestInfo(); + if (!testInfo) + throw new Error(`toPass() must be called during the test`); + const timeout = options.timeout !== undefined ? options.timeout : 0; + // Soft expects might mark test as failing. + // We want to revert this later if the matcher is actually passing. + // See https://github.com/microsoft/playwright/issues/20437 + const testStateBeforeToPassMatcher = testInfo._saveTestInfo(); const result = await pollAgainstTimeout(async () => { try { + const errorCount = testInfo.errors.length; await callback(); + if (testInfo.errors.length !== errorCount) + return { continuePolling: !this.isNot, result: testInfo.errors[testInfo.errors.length - 1] }; return { continuePolling: this.isNot, result: undefined }; } catch (e) { return { continuePolling: !this.isNot, result: e }; @@ -339,5 +351,8 @@ export async function toPass( return { message, pass: this.isNot }; } - return { pass: !this.isNot, message: () => '' }; + const pass = !this.isNot; + if (pass) + testInfo._restoreTestInfo(testStateBeforeToPassMatcher); + return { pass, message: () => '' }; } diff --git a/tests/playwright-test/expect-to-pass.spec.ts b/tests/playwright-test/expect-to-pass.spec.ts index 1a92e0b81ddac..c99191500d414 100644 --- a/tests/playwright-test/expect-to-pass.spec.ts +++ b/tests/playwright-test/expect-to-pass.spec.ts @@ -35,10 +35,17 @@ test('should retry predicate', async ({ runInlineTest }) => { }).toPass(); expect(i).toBe(3); }); + test('should retry expect.soft assertions', async () => { + let i = 0; + await test.expect(() => { + expect.soft(++i).toBe(3); + }).toPass(); + expect(i).toBe(3); + }); ` }); expect(result.exitCode).toBe(0); - expect(result.passed).toBe(2); + expect(result.passed).toBe(3); }); test('should respect timeout', async ({ runInlineTest }) => { @@ -152,6 +159,28 @@ test('should use custom message', async ({ runInlineTest }) => { expect(result.failed).toBe(1); }); +test('should swallow all soft errors inside toPass matcher, if successful', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.spec.ts': ` + const { test } = pwt; + test('should respect soft', async () => { + expect.soft('before-toPass').toBe('zzzz'); + let i = 0; + await test.expect(() => { + ++i; + expect.soft('inside-toPass-' + i).toBe('inside-toPass-2'); + }).toPass({ timeout: 1000 }); + expect.soft('after-toPass').toBe('zzzz'); + }); + ` + }); + expect(stripAnsi(result.output)).toContain('Received: "before-toPass"'); + expect(stripAnsi(result.output)).toContain('Received: "after-toPass"'); + expect(stripAnsi(result.output)).not.toContain('Received: "inside-toPass"'); + expect(result.exitCode).toBe(1); + expect(result.failed).toBe(1); +}); + test('should work with soft', async ({ runInlineTest }) => { const result = await runInlineTest({ 'a.spec.ts': ` From e52e22622fc024f189c22c9446fb62fac8bd6e55 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Mon, 30 Jan 2023 23:45:55 +0400 Subject: [PATCH 2/4] fix test --- tests/playwright-test/expect-to-pass.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/playwright-test/expect-to-pass.spec.ts b/tests/playwright-test/expect-to-pass.spec.ts index c99191500d414..3af3e9a88738d 100644 --- a/tests/playwright-test/expect-to-pass.spec.ts +++ b/tests/playwright-test/expect-to-pass.spec.ts @@ -176,7 +176,7 @@ test('should swallow all soft errors inside toPass matcher, if successful', asyn }); expect(stripAnsi(result.output)).toContain('Received: "before-toPass"'); expect(stripAnsi(result.output)).toContain('Received: "after-toPass"'); - expect(stripAnsi(result.output)).not.toContain('Received: "inside-toPass"'); + expect(stripAnsi(result.output)).not.toContain('Received: "inside-toPass-1"'); expect(result.exitCode).toBe(1); expect(result.failed).toBe(1); }); From e1d5145909008408d7491fde8ae0a5cbb71f9109 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Tue, 31 Jan 2023 13:29:27 +0400 Subject: [PATCH 3/4] address offline comments --- .../playwright-test/src/common/testInfo.ts | 14 ++++++++++--- .../playwright-test/src/matchers/matchers.ts | 11 +++++----- tests/playwright-test/expect-to-pass.spec.ts | 20 +++++++++++++++++++ 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/packages/playwright-test/src/common/testInfo.ts b/packages/playwright-test/src/common/testInfo.ts index a40aeebe74bfe..ed703185f1260 100644 --- a/packages/playwright-test/src/common/testInfo.ts +++ b/packages/playwright-test/src/common/testInfo.ts @@ -24,6 +24,12 @@ import { TimeoutManager } from './timeoutManager'; import type { Annotation, FullConfigInternal, FullProjectInternal, TestStepInternal } from './types'; import { getContainedPath, normalizeAndSaveAttachment, sanitizeForFilePath, serializeError, trimLongString } from '../util'; +export type TestInfoState = { + status: TestStatus, + errors: TestInfoError[], + hasHardError: boolean, +}; + export class TestInfoImpl implements TestInfo { private _onStepBegin: (payload: StepBeginPayload) => void; private _onStepEnd: (payload: StepEndPayload) => void; @@ -246,16 +252,18 @@ export class TestInfoImpl implements TestInfo { this.errors.push(error); } - _saveTestInfo() { + _saveTestState(): TestInfoState { return { + hasHardError: this._hasHardError, status: this.status, errors: this.errors.slice(), - } + }; } - _restoreTestInfo(state: { status: TestStatus, errors: TestInfoError[] }) { + _restoreTestState(state: TestInfoState) { this.status = state.status; this.errors = state.errors.slice(); + this._hasHardError = state.hasHardError; } async _runAsStep(cb: () => Promise, stepInfo: Omit): Promise { diff --git a/packages/playwright-test/src/matchers/matchers.ts b/packages/playwright-test/src/matchers/matchers.ts index bc81b5ce94bbc..9f28d184f2553 100644 --- a/packages/playwright-test/src/matchers/matchers.ts +++ b/packages/playwright-test/src/matchers/matchers.ts @@ -20,6 +20,7 @@ import { colors } from 'playwright-core/lib/utilsBundle'; import type { Expect } from '../common/types'; import { expectTypes, callLogText } from '../util'; import { currentTestInfo } from '../common/globals'; +import type { TestInfoState } from '../common/testInfo'; import { toBeTruthy } from './toBeTruthy'; import { toEqual } from './toEqual'; import { toExpectedTextValues, toMatchText } from './toMatchText'; @@ -327,12 +328,14 @@ export async function toPass( // Soft expects might mark test as failing. // We want to revert this later if the matcher is actually passing. // See https://github.com/microsoft/playwright/issues/20437 - const testStateBeforeToPassMatcher = testInfo._saveTestInfo(); + let testStateBeforeToPassMatcher: undefined|TestInfoState; const result = await pollAgainstTimeout(async () => { try { - const errorCount = testInfo.errors.length; + if (testStateBeforeToPassMatcher) + testInfo._restoreTestState(testStateBeforeToPassMatcher); + testStateBeforeToPassMatcher = testInfo._saveTestState(); await callback(); - if (testInfo.errors.length !== errorCount) + if (testInfo.errors.length !== testStateBeforeToPassMatcher.errors.length) return { continuePolling: !this.isNot, result: testInfo.errors[testInfo.errors.length - 1] }; return { continuePolling: this.isNot, result: undefined }; } catch (e) { @@ -352,7 +355,5 @@ export async function toPass( return { message, pass: this.isNot }; } const pass = !this.isNot; - if (pass) - testInfo._restoreTestInfo(testStateBeforeToPassMatcher); return { pass, message: () => '' }; } diff --git a/tests/playwright-test/expect-to-pass.spec.ts b/tests/playwright-test/expect-to-pass.spec.ts index 3af3e9a88738d..f200ad5043670 100644 --- a/tests/playwright-test/expect-to-pass.spec.ts +++ b/tests/playwright-test/expect-to-pass.spec.ts @@ -181,6 +181,26 @@ test('should swallow all soft errors inside toPass matcher, if successful', asyn expect(result.failed).toBe(1); }); +test('should show only soft errors on last toPass pass', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.spec.ts': ` + const { test } = pwt; + test('should respect soft', async () => { + let i = 0; + await test.expect(() => { + ++i; + expect.soft('inside-toPass-' + i).toBe('0'); + }).toPass({ timeout: 1000, intervals: [100, 100, 100000] }); + }); + ` + }); + expect(stripAnsi(result.output)).not.toContain('Received: "inside-toPass-1"'); + expect(stripAnsi(result.output)).not.toContain('Received: "inside-toPass-2"'); + expect(stripAnsi(result.output)).toContain('Received: "inside-toPass-3"'); + expect(result.exitCode).toBe(1); + expect(result.failed).toBe(1); +}); + test('should work with soft', async ({ runInlineTest }) => { const result = await runInlineTest({ 'a.spec.ts': ` From de3374955f550d3b6522cbb017d8900d2afd38da Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Thu, 2 Feb 2023 22:12:33 +0400 Subject: [PATCH 4/4] address comments --- packages/playwright-test/src/common/testInfo.ts | 6 +++--- .../playwright-test/src/matchers/matchers.ts | 17 +++++++---------- tests/playwright-test/expect-to-pass.spec.ts | 2 ++ 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/packages/playwright-test/src/common/testInfo.ts b/packages/playwright-test/src/common/testInfo.ts index ed703185f1260..28518dd66f249 100644 --- a/packages/playwright-test/src/common/testInfo.ts +++ b/packages/playwright-test/src/common/testInfo.ts @@ -24,7 +24,7 @@ import { TimeoutManager } from './timeoutManager'; import type { Annotation, FullConfigInternal, FullProjectInternal, TestStepInternal } from './types'; import { getContainedPath, normalizeAndSaveAttachment, sanitizeForFilePath, serializeError, trimLongString } from '../util'; -export type TestInfoState = { +export type TestInfoErrorState = { status: TestStatus, errors: TestInfoError[], hasHardError: boolean, @@ -252,7 +252,7 @@ export class TestInfoImpl implements TestInfo { this.errors.push(error); } - _saveTestState(): TestInfoState { + _saveErrorState(): TestInfoErrorState { return { hasHardError: this._hasHardError, status: this.status, @@ -260,7 +260,7 @@ export class TestInfoImpl implements TestInfo { }; } - _restoreTestState(state: TestInfoState) { + _restoreErrorState(state: TestInfoErrorState) { this.status = state.status; this.errors = state.errors.slice(); this._hasHardError = state.hasHardError; diff --git a/packages/playwright-test/src/matchers/matchers.ts b/packages/playwright-test/src/matchers/matchers.ts index 9f28d184f2553..6dd5c658ba718 100644 --- a/packages/playwright-test/src/matchers/matchers.ts +++ b/packages/playwright-test/src/matchers/matchers.ts @@ -20,7 +20,7 @@ import { colors } from 'playwright-core/lib/utilsBundle'; import type { Expect } from '../common/types'; import { expectTypes, callLogText } from '../util'; import { currentTestInfo } from '../common/globals'; -import type { TestInfoState } from '../common/testInfo'; +import type { TestInfoErrorState } from '../common/testInfo'; import { toBeTruthy } from './toBeTruthy'; import { toEqual } from './toEqual'; import { toExpectedTextValues, toMatchText } from './toMatchText'; @@ -320,22 +320,20 @@ export async function toPass( } = {}, ) { const testInfo = currentTestInfo(); - if (!testInfo) - throw new Error(`toPass() must be called during the test`); const timeout = options.timeout !== undefined ? options.timeout : 0; // Soft expects might mark test as failing. // We want to revert this later if the matcher is actually passing. // See https://github.com/microsoft/playwright/issues/20437 - let testStateBeforeToPassMatcher: undefined|TestInfoState; + let testStateBeforeToPassMatcher: undefined|TestInfoErrorState; const result = await pollAgainstTimeout(async () => { try { - if (testStateBeforeToPassMatcher) - testInfo._restoreTestState(testStateBeforeToPassMatcher); - testStateBeforeToPassMatcher = testInfo._saveTestState(); + if (testStateBeforeToPassMatcher && testInfo) + testInfo._restoreErrorState(testStateBeforeToPassMatcher); + testStateBeforeToPassMatcher = testInfo?._saveErrorState(); await callback(); - if (testInfo.errors.length !== testStateBeforeToPassMatcher.errors.length) + if (testInfo && testStateBeforeToPassMatcher && testInfo.errors.length > testStateBeforeToPassMatcher.errors.length) return { continuePolling: !this.isNot, result: testInfo.errors[testInfo.errors.length - 1] }; return { continuePolling: this.isNot, result: undefined }; } catch (e) { @@ -354,6 +352,5 @@ export async function toPass( return { message, pass: this.isNot }; } - const pass = !this.isNot; - return { pass, message: () => '' }; + return { pass: !this.isNot, message: () => '' }; } diff --git a/tests/playwright-test/expect-to-pass.spec.ts b/tests/playwright-test/expect-to-pass.spec.ts index f200ad5043670..d5d9937e57f1d 100644 --- a/tests/playwright-test/expect-to-pass.spec.ts +++ b/tests/playwright-test/expect-to-pass.spec.ts @@ -160,6 +160,8 @@ test('should use custom message', async ({ runInlineTest }) => { }); test('should swallow all soft errors inside toPass matcher, if successful', async ({ runInlineTest }) => { + test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/20437' }); + const result = await runInlineTest({ 'a.spec.ts': ` const { test } = pwt;