diff --git a/packages/playwright/src/common/testType.ts b/packages/playwright/src/common/testType.ts index d9c828cc62..565ed58321 100644 --- a/packages/playwright/src/common/testType.ts +++ b/packages/playwright/src/common/testType.ts @@ -25,6 +25,7 @@ import { wrapFunctionWithLocation } from '../transform/transform'; import type { FixturesWithLocation } from './config'; import type { Fixtures, TestDetails, TestStepInfo, TestType } from '../../types/test'; import type { Location } from '../../types/testReporter'; +import type { TestInfoImpl } from '../worker/testInfo'; const testTypeSymbol = Symbol('testType'); @@ -261,10 +262,14 @@ export class TestTypeImpl { suite._use.push({ fixtures, location }); } - async _step(expectation: 'pass'|'skip', title: string, body: (step: TestStepInfo) => T | Promise, options: {box?: boolean, location?: Location, timeout?: number } = {}): Promise { + _step(expectation: 'pass'|'skip', title: string, body: (step: TestStepInfo) => T | Promise, options: {box?: boolean, location?: Location, timeout?: number } = {}): Promise { const testInfo = currentTestInfo(); if (!testInfo) throw new Error(`test.step() can only be called from a test`); + return testInfo._floatingPromiseScope.wrapPromiseAPIResult(this._stepInternal(expectation, testInfo, title, body, options)); + } + + private async _stepInternal(expectation: 'pass'|'skip', testInfo: TestInfoImpl, title: string, body: (step: TestStepInfo) => T | Promise, options: {box?: boolean, location?: Location, timeout?: number } = {}): Promise { const step = testInfo._addStep({ category: 'test.step', title, location: options.location, box: options.box }); return await currentZone().with('stepZone', step).run(async () => { try { diff --git a/packages/playwright/src/matchers/expect.ts b/packages/playwright/src/matchers/expect.ts index 58eb9be9fc..9789ca8766 100644 --- a/packages/playwright/src/matchers/expect.ts +++ b/packages/playwright/src/matchers/expect.ts @@ -381,8 +381,10 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler { setMatcherCallContext({ expectInfo: this._info, testInfo, step: step.info }); const callback = () => matcher.call(target, ...args); const result = currentZone().with('stepZone', step).run(callback); - if (result instanceof Promise) - return result.then(finalizer).catch(reportStepError); + if (result instanceof Promise) { + const promise = result.then(finalizer).catch(reportStepError); + return testInfo._floatingPromiseScope.wrapPromiseAPIResult(promise); + } finalizer(); return result; } catch (e) { diff --git a/packages/playwright/src/reporters/base.ts b/packages/playwright/src/reporters/base.ts index b64877e162..7affd1a016 100644 --- a/packages/playwright/src/reporters/base.ts +++ b/packages/playwright/src/reporters/base.ts @@ -270,6 +270,7 @@ export class TerminalReporter implements ReporterV2 { if (full && summary.failuresToPrint.length && !this._omitFailures) this._printFailures(summary.failuresToPrint); this._printSlowTests(); + this._printWarnings(); this._printSummary(summaryMessage); } @@ -289,6 +290,28 @@ export class TerminalReporter implements ReporterV2 { console.log(this.screen.colors.yellow(' Consider running tests from slow files in parallel, see https://playwright.dev/docs/test-parallel.')); } + private _printWarnings() { + const warningTests = this.suite.allTests().filter(test => test.annotations.some(a => a.type === 'warning')); + const encounteredWarnings = new Map>(); + for (const test of warningTests) { + for (const annotation of test.annotations) { + if (annotation.type !== 'warning' || annotation.description === undefined) + continue; + let tests = encounteredWarnings.get(annotation.description); + if (!tests) { + tests = []; + encounteredWarnings.set(annotation.description, tests); + } + tests.push(test); + } + } + for (const [description, tests] of encounteredWarnings) { + console.log(this.screen.colors.yellow(' Warning: ') + description); + for (const test of tests) + console.log(this.formatTestHeader(test, { indent: ' ', mode: 'default' })); + } + } + private _printSummary(summary: string) { if (summary.trim()) console.log(summary); diff --git a/packages/playwright/src/worker/floatingPromiseScope.ts b/packages/playwright/src/worker/floatingPromiseScope.ts new file mode 100644 index 0000000000..670e09eb84 --- /dev/null +++ b/packages/playwright/src/worker/floatingPromiseScope.ts @@ -0,0 +1,49 @@ +/** + * Copyright (c) Microsoft Corporation. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export class FloatingPromiseScope { + readonly _floatingCalls: Set> = new Set(); + + /** + * Enables a promise API call to be tracked by the test, alerting if unawaited. + * + * **NOTE:** Returning from an async function wraps the result in a promise, regardless of whether the return value is a promise. This will automatically mark the promise as awaited. Avoid this. + */ + wrapPromiseAPIResult(promise: Promise): Promise { + const promiseProxy = new Proxy(promise, { + get: (target, prop, receiver) => { + if (prop === 'then') { + return (...args: any[]) => { + this._floatingCalls.delete(promise); + + const originalThen = Reflect.get(target, prop, receiver) as Promise['then']; + return originalThen.call(target, ...args); + }; + } else { + return Reflect.get(target, prop, receiver); + } + } + }); + + this._floatingCalls.add(promise); + + return promiseProxy; + } + + hasFloatingPromises(): boolean { + return this._floatingCalls.size > 0; + } +} diff --git a/packages/playwright/src/worker/testInfo.ts b/packages/playwright/src/worker/testInfo.ts index 50fc494bd9..5bac56776d 100644 --- a/packages/playwright/src/worker/testInfo.ts +++ b/packages/playwright/src/worker/testInfo.ts @@ -23,6 +23,7 @@ import { TimeoutManager, TimeoutManagerError, kMaxDeadline } from './timeoutMana import { debugTest, filteredStackTrace, formatLocation, getContainedPath, normalizeAndSaveAttachment, trimLongString, windowsFilesystemFriendlyLength } from '../util'; import { TestTracing } from './testTracing'; import { testInfoError } from './util'; +import { FloatingPromiseScope } from './floatingPromiseScope'; import type { RunnableDescription } from './timeoutManager'; import type { FullProject, TestInfo, TestStatus, TestStepInfo } from '../../types/test'; @@ -67,6 +68,7 @@ export class TestInfoImpl implements TestInfo { readonly _startTime: number; readonly _startWallTime: number; readonly _tracing: TestTracing; + readonly _floatingPromiseScope: FloatingPromiseScope = new FloatingPromiseScope(); _wasInterrupted = false; _lastStepId = 0; diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index e09579d0e0..664409218d 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -369,6 +369,9 @@ export class WorkerMain extends ProcessRunner { // Now run the test itself. const fn = test.fn; // Extract a variable to get a better stack trace ("myTest" vs "TestCase.myTest [as fn]"). await fn(testFunctionParams, testInfo); + // Create warning if any of the async calls were not awaited. + if (testInfo._floatingPromiseScope.hasFloatingPromises()) + testInfo.annotations.push({ type: 'warning', description: 'Some async calls were not awaited by the end of the test. This can cause flakiness.' }); }); }).catch(() => {}); // Ignore the top-level error, it is already inside TestInfo.errors. diff --git a/tests/playwright-test/warnings.spec.ts b/tests/playwright-test/warnings.spec.ts new file mode 100644 index 0000000000..6c6c9b089c --- /dev/null +++ b/tests/playwright-test/warnings.spec.ts @@ -0,0 +1,176 @@ +/** + * Copyright (c) Microsoft Corporation. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { test, expect } from './playwright-test-fixtures'; + +const warningSnippet = 'Some async calls were not awaited'; + +test.describe.configure({ mode: 'parallel' }); + +test.describe('await', () => { + test('should not care about non-API promises', async ({ runInlineTest }) => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test } from '@playwright/test'; + test('test', () => { + new Promise(() => {}); + }); + ` + }); + expect(exitCode).toBe(0); + expect(stdout).not.toContain(warningSnippet); + }); + + test('should warn about missing await on expects when failing', async ({ runInlineTest }) => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('custom test name', async ({ page }) => { + expect(page.locator('div')).toHaveText('A', { timeout: 100 }); + }); + ` + }); + expect(exitCode).toBe(1); + expect(stdout).toContain(warningSnippet); + expect(stdout).toContain('custom test name'); + }); + + test('should warn about missing await on expects when passing', async ({ runInlineTest }) => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('test', async ({ page }) => { + await page.setContent('data:text/html,
A
'); + expect(page.locator('div')).toHaveText('A'); + }); + ` + }); + expect(exitCode).toBe(0); + expect(stdout).toContain(warningSnippet); + }); + + test('should not warn when not missing await on expects when failing', async ({ runInlineTest }) => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('test', async ({ page }) => { + await expect(page.locator('div')).toHaveText('A', { timeout: 100 }); + }); + ` + }); + expect(exitCode).toBe(1); + expect(stdout).not.toContain(warningSnippet); + }); + + test('should not warn when not missing await on expects when passing', async ({ runInlineTest }) => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('test', async ({ page }) => { + await page.setContent('data:text/html,
A
'); + await expect(page.locator('div')).toHaveText('A'); + }); + ` + }); + expect(exitCode).toBe(0); + expect(stdout).not.toContain(warningSnippet); + }); + + test('should warn about missing await on reject', async ({ runInlineTest }) => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('test', async ({ page }) => { + expect(Promise.reject(new Error('foo'))).rejects.toThrow('foo'); + }); + ` + }); + expect(exitCode).toBe(0); + expect(stdout).toContain(warningSnippet); + }); + + test('should warn about missing await on reject.not', async ({ runInlineTest }) => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('test', async ({ page }) => { + expect(Promise.reject(new Error('foo'))).rejects.not.toThrow('foo'); + }); + ` + }); + expect(exitCode).toBe(1); + expect(stdout).toContain(warningSnippet); + }); + + test('should warn about missing await on test.step', async ({ runInlineTest }) => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('test', async ({ page }) => { + await page.setContent('data:text/html,
A
'); + test.step('step', () => {}); + await expect(page.locator('div')).toHaveText('A'); + }); + ` + }); + expect(exitCode).toBe(0); + expect(stdout).toContain(warningSnippet); + }); + + test('should not warn when not missing await on test.step', async ({ runInlineTest }) => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('test', async ({ page }) => { + await page.setContent('data:text/html,
A
'); + await test.step('step', () => {}); + await expect(page.locator('div')).toHaveText('A'); + }); + ` + }); + expect(exitCode).toBe(0); + expect(stdout).not.toContain(warningSnippet); + }); + + test('should warn about missing await on test.step.skip', async ({ runInlineTest }) => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('test', async ({ page }) => { + await page.setContent('data:text/html,
A
'); + test.step.skip('step', () => {}); + await expect(page.locator('div')).toHaveText('A'); + }); + ` + }); + expect(exitCode).toBe(0); + expect(stdout).toContain(warningSnippet); + }); + + test('traced promise should be instanceof Promise', async ({ runInlineTest }) => { + const { exitCode } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('test', async ({ page }) => { + await page.setContent('data:text/html,
A
'); + const expectPromise = expect(page.locator('div')).toHaveText('A'); + expect(expectPromise instanceof Promise).toBeTruthy(); + }); + ` + }); + expect(exitCode).toBe(0); + }); +});