From d84588b5652276f6362707a0f1604a6dbfa22fb5 Mon Sep 17 00:00:00 2001 From: Adam Gastineau Date: Wed, 19 Feb 2025 06:10:41 -0800 Subject: [PATCH] Moved scope logic to separate class --- packages/playwright/src/common/testType.ts | 4 +- packages/playwright/src/matchers/expect.ts | 2 +- .../src/worker/floatingPromiseScope.ts | 49 +++++++++++++++++++ packages/playwright/src/worker/testInfo.ts | 29 +---------- packages/playwright/src/worker/workerMain.ts | 2 +- tests/playwright-test/warnings.spec.ts | 3 +- 6 files changed, 57 insertions(+), 32 deletions(-) create mode 100644 packages/playwright/src/worker/floatingPromiseScope.ts diff --git a/packages/playwright/src/common/testType.ts b/packages/playwright/src/common/testType.ts index 60ac5a8400..565ed58321 100644 --- a/packages/playwright/src/common/testType.ts +++ b/packages/playwright/src/common/testType.ts @@ -266,10 +266,10 @@ export class TestTypeImpl { const testInfo = currentTestInfo(); if (!testInfo) throw new Error(`test.step() can only be called from a test`); - return testInfo._wrapPromiseAPIResult(this._stepInternal(expectation, testInfo, title, body, options)); + return testInfo._floatingPromiseScope.wrapPromiseAPIResult(this._stepInternal(expectation, testInfo, title, body, options)); } - async _stepInternal(expectation: 'pass'|'skip', testInfo: TestInfoImpl, title: string, body: (step: TestStepInfo) => T | Promise, options: {box?: boolean, location?: Location, timeout?: number } = {}): Promise { + 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 5f678b743f..9789ca8766 100644 --- a/packages/playwright/src/matchers/expect.ts +++ b/packages/playwright/src/matchers/expect.ts @@ -383,7 +383,7 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler { const result = currentZone().with('stepZone', step).run(callback); if (result instanceof Promise) { const promise = result.then(finalizer).catch(reportStepError); - return testInfo._wrapPromiseAPIResult(promise); + return testInfo._floatingPromiseScope.wrapPromiseAPIResult(promise); } finalizer(); return result; 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 279a00c250..28b6eb77bd 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; @@ -99,7 +101,6 @@ export class TestInfoImpl implements TestInfo { duration: number = 0; readonly annotations: Annotation[] = []; readonly attachments: TestInfo['attachments'] = []; - readonly unusedAsyncApiCalls: Set> = new Set(); status: TestStatus = 'passed'; snapshotSuffix: string = ''; readonly outputDir: string; @@ -411,32 +412,6 @@ export class TestInfoImpl implements TestInfo { this._timeoutManager.setIgnoreTimeouts(); } - /** - * 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, reciever) => { - if (prop === 'then') { - return (...args: any[]) => { - this.unusedAsyncApiCalls.delete(promise); - - const originalThen = Reflect.get(target, prop, reciever) as Promise['then']; - return originalThen.call(target, ...args); - }; - } else { - return Reflect.get(target, prop, reciever); - } - } - }); - - this.unusedAsyncApiCalls.add(promise); - - return promiseProxy; - } - // ------------ TestInfo methods ------------ async attach(name: string, options: { path?: string, body?: string | Buffer, contentType?: string } = {}) { diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index bbe366a5bb..664409218d 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -370,7 +370,7 @@ export class WorkerMain extends ProcessRunner { 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.unusedAsyncApiCalls.size > 0) + 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 index ce4796cfd6..6c6c9b089c 100644 --- a/tests/playwright-test/warnings.spec.ts +++ b/tests/playwright-test/warnings.spec.ts @@ -38,13 +38,14 @@ test.describe('await', () => { const { exitCode, stdout } = await runInlineTest({ 'a.test.ts': ` import { test, expect } from '@playwright/test'; - test('test', async ({ page }) => { + 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 }) => {