From 33c0a1b0ca5ef907b5223541dddd9fda1ad2fcc0 Mon Sep 17 00:00:00 2001 From: Adam Gastineau Date: Thu, 20 Feb 2025 10:46:49 -0800 Subject: [PATCH] chore: add floating promise warning to hooks (#34861) --- .../src/worker/floatingPromiseScope.ts | 4 + packages/playwright/src/worker/workerMain.ts | 17 +++- tests/playwright-test/warnings.spec.ts | 98 +++++++++++++++++-- 3 files changed, 110 insertions(+), 9 deletions(-) diff --git a/packages/playwright/src/worker/floatingPromiseScope.ts b/packages/playwright/src/worker/floatingPromiseScope.ts index 670e09eb84..5c3997f5e6 100644 --- a/packages/playwright/src/worker/floatingPromiseScope.ts +++ b/packages/playwright/src/worker/floatingPromiseScope.ts @@ -43,6 +43,10 @@ export class FloatingPromiseScope { return promiseProxy; } + clear() { + this._floatingCalls.clear(); + } + hasFloatingPromises(): boolean { return this._floatingCalls.size > 0; } diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index 664409218d..8ff6d31fd0 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -321,6 +321,15 @@ export class WorkerMain extends ProcessRunner { let shouldRunAfterEachHooks = false; testInfo._allowSkips = true; + + // Create warning if any of the async calls were not awaited in various stages. + const checkForFloatingPromises = (functionDescription: string) => { + if (!testInfo._floatingPromiseScope.hasFloatingPromises()) + return; + testInfo.annotations.push({ type: 'warning', description: `Some async calls were not awaited by the end of ${functionDescription}. This can cause flakiness.` }); + testInfo._floatingPromiseScope.clear(); + }; + await testInfo._runAsStage({ title: 'setup and test' }, async () => { await testInfo._runAsStage({ title: 'start tracing', runnable: { type: 'test' } }, async () => { // Ideally, "trace" would be an config-level option belonging to the @@ -360,6 +369,8 @@ export class WorkerMain extends ProcessRunner { testFunctionParams = await this._fixtureRunner.resolveParametersForFunction(test.fn, testInfo, 'test', { type: 'test' }); }); + checkForFloatingPromises('beforeAll/beforeEach hooks'); + if (testFunctionParams === null) { // Fixture setup failed or was skipped, we should not run the test now. return; @@ -369,9 +380,7 @@ 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.' }); + checkForFloatingPromises('the test'); }); }).catch(() => {}); // Ignore the top-level error, it is already inside TestInfo.errors. @@ -429,6 +438,8 @@ export class WorkerMain extends ProcessRunner { throw firstAfterHooksError; }).catch(() => {}); // Ignore the top-level error, it is already inside TestInfo.errors. + checkForFloatingPromises('afterAll/afterEach hooks'); + if (testInfo._isFailure()) this._isStopped = true; diff --git a/tests/playwright-test/warnings.spec.ts b/tests/playwright-test/warnings.spec.ts index 6c6c9b089c..d9ed42eb5d 100644 --- a/tests/playwright-test/warnings.spec.ts +++ b/tests/playwright-test/warnings.spec.ts @@ -45,6 +45,7 @@ test.describe('await', () => { }); expect(exitCode).toBe(1); expect(stdout).toContain(warningSnippet); + expect(stdout).toContain('the test'); expect(stdout).toContain('custom test name'); }); @@ -53,7 +54,7 @@ test.describe('await', () => { 'a.test.ts': ` import { test, expect } from '@playwright/test'; test('test', async ({ page }) => { - await page.setContent('data:text/html,
A
'); + await page.setContent('
A
'); expect(page.locator('div')).toHaveText('A'); }); ` @@ -80,7 +81,7 @@ test.describe('await', () => { 'a.test.ts': ` import { test, expect } from '@playwright/test'; test('test', async ({ page }) => { - await page.setContent('data:text/html,
A
'); + await page.setContent('
A
'); await expect(page.locator('div')).toHaveText('A'); }); ` @@ -89,6 +90,20 @@ test.describe('await', () => { expect(stdout).not.toContain(warningSnippet); }); + test('should not warn when using then 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('
A
'); + expect(page.locator('div')).toHaveText('A').then(() => {}); + }); + ` + }); + 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': ` @@ -120,7 +135,7 @@ test.describe('await', () => { 'a.test.ts': ` import { test, expect } from '@playwright/test'; test('test', async ({ page }) => { - await page.setContent('data:text/html,
A
'); + await page.setContent('
A
'); test.step('step', () => {}); await expect(page.locator('div')).toHaveText('A'); }); @@ -135,7 +150,7 @@ test.describe('await', () => { 'a.test.ts': ` import { test, expect } from '@playwright/test'; test('test', async ({ page }) => { - await page.setContent('data:text/html,
A
'); + await page.setContent('
A
'); await test.step('step', () => {}); await expect(page.locator('div')).toHaveText('A'); }); @@ -150,7 +165,7 @@ test.describe('await', () => { 'a.test.ts': ` import { test, expect } from '@playwright/test'; test('test', async ({ page }) => { - await page.setContent('data:text/html,
A
'); + await page.setContent('
A
'); test.step.skip('step', () => {}); await expect(page.locator('div')).toHaveText('A'); }); @@ -165,7 +180,7 @@ test.describe('await', () => { 'a.test.ts': ` import { test, expect } from '@playwright/test'; test('test', async ({ page }) => { - await page.setContent('data:text/html,
A
'); + await page.setContent('
A
'); const expectPromise = expect(page.locator('div')).toHaveText('A'); expect(expectPromise instanceof Promise).toBeTruthy(); }); @@ -173,4 +188,75 @@ test.describe('await', () => { }); expect(exitCode).toBe(0); }); + + test('should warn about missing await in before hooks', async ({ runInlineTest }) => { + const group = ['beforeAll', 'beforeEach']; + for (const hook of group) { + await test.step(hook, async () => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + let page; + test.${hook}(async ({ browser }) => { + page = await browser.newPage(); + await page.setContent('
A
'); + expect(page.locator('div')).toHaveText('A'); + }); + test('test ${hook}', async () => { + await expect(page.locator('div')).toBeVisible(); + }); + ` + }); + + expect(exitCode).toBe(0); + expect(stdout).toContain(warningSnippet); + expect(stdout).toContain(`${group[0]}/${group[1]} hooks`); + }); + } + }); + + test.describe('should warn about missing await in after hooks', () => { + const group = ['afterAll', 'afterEach']; + for (const hook of group) { + test(hook, async ({ runInlineTest }) => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + let page; + test('test ${hook}', async ({ browser }) => { + await expect(Promise.resolve()).resolves.toBe(undefined); + }); + test.${hook}(async () => { + expect(Promise.resolve()).resolves.toBe(undefined); + }); + ` + }); + + expect(exitCode).toBe(0); + expect(stdout).toContain(warningSnippet); + expect(stdout).toContain(`${group[0]}/${group[1]} hooks`); + }); + } + }); + + test('should warn about missing await across hooks and test', async ({ runInlineTest }) => { + const { exitCode, stdout } = await runInlineTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test.beforeAll(async () => { + expect(Promise.resolve()).resolves.toBe(undefined); + }); + test('test', async () => { + expect(Promise.resolve()).resolves.toBe(undefined); + }); + test.afterEach(async () => { + expect(Promise.resolve()).resolves.toBe(undefined); + }); + ` + }); + expect(exitCode).toBe(0); + expect(stdout).toContain(`${warningSnippet} by the end of beforeAll/beforeEach hooks.`); + expect(stdout).toContain(`${warningSnippet} by the end of the test.`); + expect(stdout).toContain(`${warningSnippet} by the end of afterAll/afterEach hooks.`); + }); });