Skip to content

Commit

Permalink
chore: remove expectZone (#34312)
Browse files Browse the repository at this point in the history
  • Loading branch information
dgozman authored Jan 16, 2025
1 parent 08af3a2 commit be6caed
Show file tree
Hide file tree
Showing 6 changed files with 81 additions and 33 deletions.
5 changes: 0 additions & 5 deletions packages/playwright-core/src/utils/stackTrace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,3 @@ export function compressCallLog(log: string[]): string[] {
}
return lines;
}

export type ExpectZone = {
title: string;
stepId: string;
};
2 changes: 1 addition & 1 deletion packages/playwright-core/src/utils/zones.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import { AsyncLocalStorage } from 'async_hooks';

export type ZoneType = 'apiZone' | 'expectZone' | 'stepZone';
export type ZoneType = 'apiZone' | 'stepZone';

class ZoneManager {
private readonly _asyncLocalStorage = new AsyncLocalStorage<Zone | undefined>();
Expand Down
9 changes: 4 additions & 5 deletions packages/playwright/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import * as path from 'path';
import type { APIRequestContext, BrowserContext, Browser, BrowserContextOptions, LaunchOptions, Page, Tracing, Video } from 'playwright-core';
import * as playwrightLibrary from 'playwright-core';
import { createGuid, debugMode, addInternalStackPrefix, isString, asLocator, jsonStringifyForceASCII, zones } from 'playwright-core/lib/utils';
import type { ExpectZone } from 'playwright-core/lib/utils';
import type { Fixtures, PlaywrightTestArgs, PlaywrightTestOptions, PlaywrightWorkerArgs, PlaywrightWorkerOptions, ScreenshotMode, TestInfo, TestType, VideoMode } from '../types/test';
import type { TestInfoImpl, TestStepInternal } from './worker/testInfo';
import { rootTestType } from './common/testType';
Expand Down Expand Up @@ -264,12 +263,12 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
// Some special calls do not get into steps.
if (!testInfo || data.apiName.includes('setTestIdAttribute') || data.apiName === 'tracing.groupEnd')
return;
const expectZone = zones.zoneData<ExpectZone>('expectZone');
if (expectZone) {
const zone = zones.zoneData<TestStepInternal>('stepZone');
if (zone && zone.category === 'expect') {
// Display the internal locator._expect call under the name of the enclosing expect call,
// and connect it to the existing expect step.
data.apiName = expectZone.title;
data.stepId = expectZone.stepId;
data.apiName = zone.title;
data.stepId = zone.stepId;
return;
}
// In the general case, create a step for each api call and connect them through the stepId.
Expand Down
12 changes: 4 additions & 8 deletions packages/playwright/src/matchers/expect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {
createGuid,
isString,
pollAgainstDeadline } from 'playwright-core/lib/utils';
import type { ExpectZone } from 'playwright-core/lib/utils';
import {
toBeAttached,
toBeChecked,
Expand Down Expand Up @@ -315,9 +314,10 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler<any> {
// out all the frames that belong to the test runner from caught runtime errors.
const stackFrames = filteredStackTrace(captureRawStack());

// Enclose toPass in a step to maintain async stacks, toPass matcher is always async.
// toPass and poll matchers can contain other steps, expects and API calls,
// so they behave like a retriable step.
const stepInfo = {
category: 'expect',
category: (matcherName === 'toPass' || this._info.poll) ? 'step' : 'expect',
title: trimLongString(title, 1024),
params: args[0] ? { expected: args[0] } : undefined,
infectParentStepsWithError: this._info.isSoft,
Expand Down Expand Up @@ -345,11 +345,7 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler<any> {

try {
const callback = () => matcher.call(target, ...args);
// toPass and poll matchers can contain other steps, expects and API calls,
// so they behave like a retriable step.
const result = (matcherName === 'toPass' || this._info.poll) ?
zones.run('stepZone', step, callback) :
zones.run<ExpectZone, any>('expectZone', { title, stepId: step.stepId }, callback);
const result = zones.run('stepZone', step, callback);
if (result instanceof Promise)
return result.then(finalizer).catch(reportStepError);
finalizer();
Expand Down
9 changes: 2 additions & 7 deletions packages/playwright/src/worker/testInfo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import fs from 'fs';
import path from 'path';
import { captureRawStack, monotonicTime, zones, sanitizeForFilePath, stringifyStackFrames } from 'playwright-core/lib/utils';
import type { ExpectZone } from 'playwright-core/lib/utils';
import type { TestInfo, TestStatus, FullProject } from '../../types/test';
import type { AttachmentPayload, StepBeginPayload, StepEndPayload, TestInfoErrorImpl, WorkerInitParams } from '../common/ipc';
import type { TestCase } from '../common/test';
Expand All @@ -35,7 +34,7 @@ export interface TestStepInternal {
attachmentIndices: number[];
stepId: string;
title: string;
category: 'hook' | 'fixture' | 'test.step' | 'test.step.skip' | 'expect' | 'attach' | string;
category: string;
location?: Location;
boxedStack?: StackFrame[];
steps: TestStepInternal[];
Expand Down Expand Up @@ -195,7 +194,7 @@ export class TestInfoImpl implements TestInfo {
this._attachmentsPush = this.attachments.push.bind(this.attachments);
this.attachments.push = (...attachments: TestInfo['attachments']) => {
for (const a of attachments)
this._attach(a, this._expectStepId() ?? this._parentStep()?.stepId);
this._attach(a, this._parentStep()?.stepId);
return this.attachments.length;
};

Expand Down Expand Up @@ -245,10 +244,6 @@ export class TestInfoImpl implements TestInfo {
?? this._findLastStageStep(this._steps); // If no parent step on stack, assume the current stage as parent.
}

private _expectStepId() {
return zones.zoneData<ExpectZone>('expectZone')?.stepId;
}

_addStep(data: Omit<TestStepInternal, 'complete' | 'stepId' | 'steps' | 'attachmentIndices'>, parentStep?: TestStepInternal): TestStepInternal {
const stepId = `${data.category}@${++this._lastStepId}`;

Expand Down
77 changes: 70 additions & 7 deletions tests/playwright-test/test-step.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ test('should not propagate errors from within toPass', async ({ runInlineTest })
expect(result.exitCode).toBe(0);
expect(result.output).toBe(`
hook |Before Hooks
expect |expect.toPass @ a.test.ts:7
step |expect.toPass @ a.test.ts:7
expect | expect.toBe @ a.test.ts:6
expect | ↪ error: Error: expect(received).toBe(expected) // Object.is equality
expect | expect.toBe @ a.test.ts:6
Expand All @@ -643,8 +643,8 @@ test('should show final toPass error', async ({ runInlineTest }) => {
expect(result.exitCode).toBe(1);
expect(stripAnsi(result.output)).toBe(`
hook |Before Hooks
expect |expect.toPass @ a.test.ts:6
expect |↪ error: Error: expect(received).toBe(expected) // Object.is equality
step |expect.toPass @ a.test.ts:6
step |↪ error: Error: expect(received).toBe(expected) // Object.is equality
expect | expect.toBe @ a.test.ts:5
expect | ↪ error: Error: expect(received).toBe(expected) // Object.is equality
hook |After Hooks
Expand Down Expand Up @@ -909,7 +909,7 @@ test('step inside expect.toPass', async ({ runInlineTest }) => {
expect(stripAnsi(result.output)).toBe(`
hook |Before Hooks
test.step |step 1 @ a.test.ts:4
expect | expect.toPass @ a.test.ts:11
step | expect.toPass @ a.test.ts:11
test.step | step 2, attempt: 0 @ a.test.ts:7
test.step | ↪ error: Error: expect(received).toBe(expected) // Object.is equality
expect | expect.toBe @ a.test.ts:9
Expand Down Expand Up @@ -956,7 +956,7 @@ fixture | fixture: context
pw:api | browser.newContext
fixture | fixture: page
pw:api | browserContext.newPage
expect |expect.toPass @ a.test.ts:11
step |expect.toPass @ a.test.ts:11
pw:api | page.goto(about:blank) @ a.test.ts:6
test.step | inner step attempt: 0 @ a.test.ts:7
test.step | ↪ error: Error: expect(received).toBe(expected) // Object.is equality
Expand Down Expand Up @@ -1007,7 +1007,7 @@ fixture | fixture: context
pw:api | browser.newContext
fixture | fixture: page
pw:api | browserContext.newPage
expect |expect.poll.toHaveLength @ a.test.ts:14
step |expect.poll.toHaveLength @ a.test.ts:14
pw:api | page.goto(about:blank) @ a.test.ts:7
test.step | inner step attempt: 0 @ a.test.ts:8
expect | expect.toBe @ a.test.ts:10
Expand Down Expand Up @@ -1059,7 +1059,7 @@ pw:api | browser.newContext
fixture | fixture: page
pw:api | browserContext.newPage
pw:api |page.setContent @ a.test.ts:4
expect |expect.poll.toBe @ a.test.ts:13
step |expect.poll.toBe @ a.test.ts:13
expect | expect.toHaveText @ a.test.ts:7
test.step | iteration 1 @ a.test.ts:9
expect | expect.toBeVisible @ a.test.ts:10
Expand Down Expand Up @@ -1565,3 +1565,66 @@ expect |expect.toBe @ a.test.ts:10
hook |After Hooks
`);
});

test('show api calls inside expects', async ({ runInlineTest }) => {
const result = await runInlineTest({
'reporter.ts': stepIndentReporter,
'playwright.config.ts': `module.exports = { reporter: './reporter' };`,
'a.test.ts': `
import { test, expect as baseExpect } from '@playwright/test';
const expect = baseExpect.extend({
async toBeInvisible(locator: Locator) {
try {
await expect.poll(() => locator.isVisible()).toBe(false);
return { name: 'toBeInvisible', pass: true, message: '' };
} catch (e) {
return { name: 'toBeInvisible', pass: false, message: () => 'Expected to be invisible, got visible!' };
}
},
});
test('test', async ({ page }) => {
await page.setContent('<div>hello</div>');
const promise = expect(page.locator('div')).toBeInvisible();
await page.waitForTimeout(1100);
await page.setContent('<div style="display:none">hello</div>');
await promise;
});
`
}, { reporter: '' });

expect(result.exitCode).toBe(0);
expect(result.report.stats.expected).toBe(1);
expect(stripAnsi(result.output)).toBe(`
hook |Before Hooks
fixture | fixture: browser
pw:api | browserType.launch
fixture | fixture: context
pw:api | browser.newContext
fixture | fixture: page
pw:api | browserContext.newPage
pw:api |page.setContent @ a.test.ts:16
expect |expect.toBeInvisible @ a.test.ts:17
step | expect.poll.toBe @ a.test.ts:7
pw:api | locator.isVisible(div) @ a.test.ts:7
expect | expect.toBe @ a.test.ts:7
expect | ↪ error: Error: expect(received).toBe(expected) // Object.is equality
pw:api | locator.isVisible(div) @ a.test.ts:7
expect | expect.toBe @ a.test.ts:7
expect | ↪ error: Error: expect(received).toBe(expected) // Object.is equality
pw:api | locator.isVisible(div) @ a.test.ts:7
expect | expect.toBe @ a.test.ts:7
expect | ↪ error: Error: expect(received).toBe(expected) // Object.is equality
pw:api | locator.isVisible(div) @ a.test.ts:7
expect | expect.toBe @ a.test.ts:7
expect | ↪ error: Error: expect(received).toBe(expected) // Object.is equality
pw:api | locator.isVisible(div) @ a.test.ts:7
expect | expect.toBe @ a.test.ts:7
pw:api |page.waitForTimeout @ a.test.ts:18
pw:api |page.setContent @ a.test.ts:19
hook |After Hooks
fixture | fixture: page
fixture | fixture: context
`);
});

0 comments on commit be6caed

Please sign in to comment.