Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/playwright/src/common/testType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -261,10 +262,14 @@ export class TestTypeImpl {
suite._use.push({ fixtures, location });
}

async _step<T>(expectation: 'pass'|'skip', title: string, body: (step: TestStepInfo) => T | Promise<T>, options: {box?: boolean, location?: Location, timeout?: number } = {}): Promise<T> {
_step<T>(expectation: 'pass'|'skip', title: string, body: (step: TestStepInfo) => T | Promise<T>, options: {box?: boolean, location?: Location, timeout?: number } = {}): Promise<T> {
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));
}

async _stepInternal<T>(expectation: 'pass'|'skip', testInfo: TestInfoImpl, title: string, body: (step: TestStepInfo) => T | Promise<T>, options: {box?: boolean, location?: Location, timeout?: number } = {}): Promise<T> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code in /common/* should not accept TestInfoImpl that only exists in /worker/ process.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this statement. We're already directly referencing TestInfoImpl a few lines above: https://github.com/microsoft/playwright/blob/main/packages/playwright/src/common/testType.ts#L251. That doesn't have an explicit type, but the original _step method consumes the type as an argument to body.

const step = testInfo._addStep({ category: 'test.step', title, location: options.location, box: options.box });
return await currentZone().with('stepZone', step).run(async () => {
try {
Expand Down
6 changes: 4 additions & 2 deletions packages/playwright/src/matchers/expect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,10 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler<any> {
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._wrapPromiseAPIResult(promise);
}
finalizer();
return result;
} catch (e) {
Expand Down
23 changes: 23 additions & 0 deletions packages/playwright/src/reporters/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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<string, Array<TestCase>>();
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);
Expand Down
27 changes: 27 additions & 0 deletions packages/playwright/src/worker/testInfo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ export class TestInfoImpl implements TestInfo {
duration: number = 0;
readonly annotations: Annotation[] = [];
readonly attachments: TestInfo['attachments'] = [];
readonly unusedAsyncApiCalls: Set<Promise<any>> = new Set();
status: TestStatus = 'passed';
snapshotSuffix: string = '';
readonly outputDir: string;
Expand Down Expand Up @@ -410,6 +411,32 @@ 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<T>(promise: Promise<T>): Promise<T> {
Comment thread
agg23 marked this conversation as resolved.
Outdated
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<T>['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 } = {}) {
Expand Down
3 changes: 3 additions & 0 deletions packages/playwright/src/worker/workerMain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.unusedAsyncApiCalls.size > 0)
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.

Expand Down
175 changes: 175 additions & 0 deletions tests/playwright-test/warnings.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
/**
* 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('test', async ({ page }) => {
expect(page.locator('div')).toHaveText('A', { timeout: 100 });
});
`
});
expect(exitCode).toBe(1);
expect(stdout).toContain(warningSnippet);
Comment thread
agg23 marked this conversation as resolved.
});

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,<div>A</div>');
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,<div>A</div>');
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,<div>A</div>');
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,<div>A</div>');
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,<div>A</div>');
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,<div>A</div>');
const expectPromise = expect(page.locator('div')).toHaveText('A');
expect(expectPromise instanceof Promise).toBeTruthy();
});
`
});
expect(exitCode).toBe(0);
});
});