Skip to content

Commit

Permalink
chore: box step w/o modifying runtime errors
Browse files Browse the repository at this point in the history
  • Loading branch information
@pavelfeldman
pavelfeldman committed Dec 22, 2023
1 parent 5d9e08a commit c417c52
Show file tree
Hide file tree
Showing 9 changed files with 161 additions and 91 deletions.
55 changes: 38 additions & 17 deletions packages/playwright/src/matchers/expect.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,45 +248,66 @@ class ExpectMetaInfoProxyHandler implements ProxyHandler<any> {
if (!testInfo)
return matcher.call(target, ...args);

const rawStack = captureRawStack();
const stackFrames = filteredStackTrace(rawStack);
const customMessage = this._info.message || '';
const argsSuffix = computeArgsSuffix(matcherName, args);

const defaultTitle = `expect${this._info.isPoll ? '.poll' : ''}${this._info.isSoft ? '.soft' : ''}${this._info.isNot ? '.not' : ''}.${matcherName}${argsSuffix}`;
const title = customMessage || defaultTitle;
const wallTime = Date.now();
const step = matcherName !== 'toPass' ? testInfo._addStep({
location: stackFrames[0],

// This looks like it is unnecessary, but it isn't - we need to filter
// out all the frames that belong to the test runner from caught runtime errors.
const rawStack = captureRawStack();
const stackFrames = filteredStackTrace(rawStack);

// Enclose toPass in a step to maintain async stacks, toPass matcher is always async.
if (matcherName === 'toPass') {
return testInfo._runAsStep({
category: 'expect',
title: trimLongString(title, 1024),
wallTime,
infectParentStepsWithError: this._info.isSoft,
laxParent: true,
isSoft: this._info.isSoft,
}, async step => {
try {
const result = await matcher.call(target, ...args);
step.complete({});
return result;
} catch (jestError) {
const error = new ExpectError(jestError, customMessage, stackFrames);
step.complete({ error });
if (!this._info.isSoft)
throw error;
}
});
}

const step = testInfo._addStep({
category: 'expect',
title: trimLongString(title, 1024),
params: args[0] ? { expected: args[0] } : undefined,
wallTime,
infectParentStepsWithError: this._info.isSoft,
laxParent: true,
}) : undefined;
isSoft: this._info.isSoft,
});

const reportStepError = (jestError: ExpectError) => {
const error = new ExpectError(jestError, customMessage, stackFrames);
const serializedError = {
message: error.message,
stack: error.stack,
};
step?.complete({ error: serializedError });
if (this._info.isSoft)
testInfo._failWithError(serializedError, false /* isHardError */);
else
step.complete({ error });
if (!this._info.isSoft)
throw error;
};

const finalizer = () => {
step?.complete({});
step.complete({});
};

const expectZone: ExpectZone = { title, wallTime };

try {
const result = zones.run<ExpectZone, any>('expectZone', expectZone, () => matcher.call(target, ...args));
const expectZone: ExpectZone | null = matcherName !== 'toPass' ? { title, wallTime } : null;
const callback = () => matcher.call(target, ...args);
const result = expectZone ? zones.run<ExpectZone, any>('expectZone', expectZone, callback) : zones.preserve(callback);
if (result instanceof Promise)
return result.then(finalizer).catch(reportStepError);
finalizer();
Expand Down
1 change: 1 addition & 0 deletions packages/playwright/src/matchers/matcherHint.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export class ExpectError extends Error {
log?: string[];
timeout?: number;
};

constructor(jestError: ExpectError, customMessage: string, stackFrames: StackFrame[]) {
super('');
// Copy to erase the JestMatcherError constructor name from the console.log(error).
Expand Down
64 changes: 24 additions & 40 deletions packages/playwright/src/matchers/matchers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@
import type { Locator, Page, APIResponse } from 'playwright-core';
import type { FrameExpectOptions } from 'playwright-core/lib/client/types';
import { colors } from 'playwright-core/lib/utilsBundle';
import { expectTypes, callLogText, filteredStackTrace } from '../util';
import { expectTypes, callLogText } from '../util';
import { toBeTruthy } from './toBeTruthy';
import { toEqual } from './toEqual';
import { toExpectedTextValues, toMatchText } from './toMatchText';
import { captureRawStack, constructURLBasedOnBaseURL, isRegExp, isTextualMimeType, pollAgainstDeadline } from 'playwright-core/lib/utils';
import { constructURLBasedOnBaseURL, isRegExp, isTextualMimeType, pollAgainstDeadline } from 'playwright-core/lib/utils';
import { currentTestInfo } from '../common/globals';
import { TestInfoImpl, type TestStepInternal } from '../worker/testInfo';
import { TestInfoImpl } from '../worker/testInfo';
import type { ExpectMatcherContext } from './expect';

interface LocatorEx extends Locator {
Expand Down Expand Up @@ -369,42 +369,26 @@ export async function toPass(
const testInfo = currentTestInfo();
const timeout = options.timeout !== undefined ? options.timeout : 0;

const rawStack = captureRawStack();
const stackFrames = filteredStackTrace(rawStack);

const runWithOrWithoutStep = async (callback: (step: TestStepInternal | undefined) => Promise<{ pass: boolean; message: () => string; }>) => {
if (!testInfo)
return await callback(undefined);
return await testInfo._runAsStep({
title: 'expect.toPass',
category: 'expect',
location: stackFrames[0],
}, callback);
};

return await runWithOrWithoutStep(async (step: TestStepInternal | undefined) => {
const { deadline, timeoutMessage } = testInfo ? testInfo._deadlineForMatcher(timeout) : TestInfoImpl._defaultDeadlineForMatcher(timeout);
const result = await pollAgainstDeadline<Error|undefined>(async () => {
if (testInfo && currentTestInfo() !== testInfo)
return { continuePolling: false, result: undefined };
try {
await callback();
return { continuePolling: !!this.isNot, result: undefined };
} catch (e) {
return { continuePolling: !this.isNot, result: e };
}
}, deadline, options.intervals || [100, 250, 500, 1000]);

if (result.timedOut) {
const message = result.result ? [
result.result.message,
'',
`Call Log:`,
`- ${timeoutMessage}`,
].join('\n') : timeoutMessage;
step?.complete({ error: { message } });
return { message: () => message, pass: !!this.isNot };
const { deadline, timeoutMessage } = testInfo ? testInfo._deadlineForMatcher(timeout) : TestInfoImpl._defaultDeadlineForMatcher(timeout);
const result = await pollAgainstDeadline<Error|undefined>(async () => {
if (testInfo && currentTestInfo() !== testInfo)
return { continuePolling: false, result: undefined };
try {
await callback();
return { continuePolling: !!this.isNot, result: undefined };
} catch (e) {
return { continuePolling: !this.isNot, result: e };
}
return { pass: !this.isNot, message: () => '' };
});
}, deadline, options.intervals || [100, 250, 500, 1000]);

if (result.timedOut) {
const message = result.result ? [
result.result.message,
'',
`Call Log:`,
`- ${timeoutMessage}`,
].join('\n') : timeoutMessage;
return { message: () => message, pass: !!this.isNot };
}
return { pass: !this.isNot, message: () => '' };
}
4 changes: 2 additions & 2 deletions packages/playwright/src/matchers/toMatchSnapshot.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import type { ImageComparatorOptions, Comparator } from 'playwright-core/lib/uti
import { getComparator, sanitizeForFilePath, zones } from 'playwright-core/lib/utils';
import type { PageScreenshotOptions } from 'playwright-core/types/types';
import {
addSuffixToFilePath, serializeError,
addSuffixToFilePath,
trimLongString, callLogText,
expectTypes } from '../util';
import { colors } from 'playwright-core/lib/utilsBundle';
Expand Down Expand Up @@ -206,7 +206,7 @@ class SnapshotHelper<T extends ImageComparatorOptions> {
return this.createMatcherResult(message, true);
}
if (this.updateSnapshots === 'missing') {
this.testInfo._failWithError(serializeError(new Error(message)), false /* isHardError */);
this.testInfo._failWithError(new Error(message), false /* isHardError */);
return this.createMatcherResult('', true);
}
return this.createMatcherResult(message, false);
Expand Down
4 changes: 2 additions & 2 deletions packages/playwright/src/worker/fixtureRunner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { formatLocation, debugTest, filterStackFile, serializeError } from '../util';
import { formatLocation, debugTest, filterStackFile } from '../util';
import { ManualPromise, zones } from 'playwright-core/lib/utils';
import type { TestInfoImpl, TestStepInternal } from './testInfo';
import type { FixtureDescription, TimeoutManager } from './timeoutManager';
Expand Down Expand Up @@ -143,7 +143,7 @@ class Fixture {
this._selfTeardownComplete?.then(() => {
afterStep?.complete({});
}).catch(e => {
afterStep?.complete({ error: serializeError(e) });
afterStep?.complete({ error: e });
});
}
testInfo._timeoutManager.setCurrentFixture(undefined);
Expand Down
50 changes: 26 additions & 24 deletions packages/playwright/src/worker/testInfo.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import type { Attachment } from './testTracing';
import type { StackFrame } from '@protocol/channels';

export interface TestStepInternal {
complete(result: { error?: Error | TestInfoError, attachments?: Attachment[] }): void;
complete(result: { error?: Error, attachments?: Attachment[] }): void;
stepId: string;
title: string;
category: string;
Expand All @@ -45,6 +45,7 @@ export interface TestStepInternal {
error?: TestInfoError;
infectParentStepsWithError?: boolean;
box?: boolean;
isSoft?: boolean;
}

export class TestInfoImpl implements TestInfo {
Expand Down Expand Up @@ -231,17 +232,16 @@ export class TestInfoImpl implements TestInfo {
this.duration = this._timeoutManager.defaultSlotTimings().elapsed | 0;
}

async _runAndFailOnError(fn: () => Promise<void>, skips?: 'allowSkips'): Promise<TestInfoError | undefined> {
async _runAndFailOnError(fn: () => Promise<void>, skips?: 'allowSkips'): Promise<Error | undefined> {
try {
await fn();
} catch (error) {
if (skips === 'allowSkips' && error instanceof SkipError) {
if (this.status === 'passed')
this.status = 'skipped';
} else {
const serialized = serializeError(error);
this._failWithError(serialized, true /* isHardError */);
return serialized;
this._failWithError(error, true /* isHardError */);
return error;
}
}
}
Expand Down Expand Up @@ -283,24 +283,18 @@ export class TestInfoImpl implements TestInfo {
complete: result => {
if (step.endWallTime)
return;

step.endWallTime = Date.now();
let error: TestInfoError | undefined;
if (result.error instanceof Error) {
// Step function threw an error.
if (data.boxedStack) {
const errorTitle = `${result.error.name}: ${result.error.message}`;
result.error.stack = `${errorTitle}\n${stringifyStackFrames(data.boxedStack).join('\n')}`;
}
error = serializeError(result.error);
} else if (result.error) {
// Internal API step reported an error.
if (result.error) {
if (!(result.error as any)[stepSymbol])
(result.error as any)[stepSymbol] = step;
const error = serializeError(result.error);
if (data.boxedStack)
result.error.stack = `${result.error.message}\n${stringifyStackFrames(data.boxedStack).join('\n')}`;
error = result.error;
error.stack = `${error.message}\n${stringifyStackFrames(data.boxedStack).join('\n')}`;
step.error = error;
}
step.error = error;

if (!error) {
if (!step.error) {
// Soft errors inside try/catch will make the test fail.
// In order to locate the failing step, we are marking all the parent
// steps as failing unconditionally.
Expand All @@ -311,18 +305,20 @@ export class TestInfoImpl implements TestInfo {
break;
}
}
error = step.error;
}

const payload: StepEndPayload = {
testId: this._test.id,
stepId,
wallTime: step.endWallTime,
error,
error: step.error,
};
this._onStepEnd(payload);
const errorForTrace = error ? { name: '', message: error.message || '', stack: error.stack } : undefined;
const errorForTrace = step.error ? { name: '', message: step.error.message || '', stack: step.error.stack } : undefined;
this._tracing.appendAfterActionForStep(stepId, errorForTrace, result.attachments);

if (step.isSoft && result.error)
this._failWithError(result.error, false /* isHardError */);
}
};
const parentStepList = parentStep ? parentStep.steps : this._steps;
Expand Down Expand Up @@ -350,7 +346,7 @@ export class TestInfoImpl implements TestInfo {
this.status = 'interrupted';
}

_failWithError(error: TestInfoError, isHardError: boolean) {
_failWithError(error: Error, isHardError: boolean) {
// Do not overwrite any previous hard errors.
// Some (but not all) scenarios include:
// - expect() that fails after uncaught exception.
Expand All @@ -361,7 +357,11 @@ export class TestInfoImpl implements TestInfo {
this._hasHardError = true;
if (this.status === 'passed' || this.status === 'skipped')
this.status = 'failed';
this.errors.push(error);
const serialized = serializeError(error);
const step = (error as any)[stepSymbol] as TestStepInternal | undefined;
if (step && step.boxedStack)
serialized.stack = `${error.name}: ${error.message}\n${stringifyStackFrames(step.boxedStack).join('\n')}`;
this.errors.push(serialized);
}

async _runAsStepWithRunnable<T>(
Expand Down Expand Up @@ -486,3 +486,5 @@ export class TestInfoImpl implements TestInfo {

class SkipError extends Error {
}

const stepSymbol = Symbol('step');
8 changes: 4 additions & 4 deletions packages/playwright/src/worker/workerMain.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ export class WorkerMain extends ProcessRunner {
// and unhandled errors - both lead to the test failing. This is good for regular tests,
// so that you can, e.g. expect() from inside an event handler. The test fails,
// and we restart the worker.
this._currentTest._failWithError(serializeError(error), true /* isHardError */);
this._currentTest._failWithError(error, true /* isHardError */);

// For tests marked with test.fail(), this might be a problem when unhandled error
// is not coming from the user test code (legit failure), but from fixtures or test runner.
Expand Down Expand Up @@ -395,7 +395,7 @@ export class WorkerMain extends ProcessRunner {
const afterHooksSlot = testInfo._didTimeout ? { timeout: this._project.project.timeout, elapsed: 0 } : undefined;
await testInfo._runAsStepWithRunnable({ category: 'hook', title: 'After Hooks', runnableType: 'afterHooks', runnableSlot: afterHooksSlot }, async step => {
testInfo._afterHooksStep = step;
let firstAfterHooksError: TestInfoError | undefined;
let firstAfterHooksError: Error | undefined;
await testInfo._runWithTimeout(async () => {
// Note: do not wrap all teardown steps together, because failure in any of them
// does not prevent further teardown steps from running.
Expand Down Expand Up @@ -541,11 +541,11 @@ export class WorkerMain extends ProcessRunner {
throw beforeAllError;
}

private async _runAfterAllHooksForSuite(suite: Suite, testInfo: TestInfoImpl) {
private async _runAfterAllHooksForSuite(suite: Suite, testInfo: TestInfoImpl): Promise<Error | undefined> {
if (!this._activeSuites.has(suite))
return;
this._activeSuites.delete(suite);
let firstError: TestInfoError | undefined;
let firstError: Error | undefined;
for (const hook of suite._hooks) {
if (hook.type !== 'afterAll')
continue;
Expand Down
2 changes: 1 addition & 1 deletion tests/playwright-test/reporter.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ for (const useIntermediateMergeReport of [false, true] as const) {
`begin {\"title\":\"expect.toBeTruthy\",\"category\":\"expect\"}`,
`end {\"title\":\"expect.toBeTruthy\",\"category\":\"expect\"}`,
`begin {\"title\":\"expect.toBeTruthy\",\"category\":\"expect\"}`,
`end {\"title\":\"expect.toBeTruthy\",\"category\":\"expect\",\"error\":{\"message\":\"\\u001b[2mexpect(\\u001b[22m\\u001b[31mreceived\\u001b[39m\\u001b[2m).\\u001b[22mtoBeTruthy\\u001b[2m()\\u001b[22m\\n\\nReceived: \\u001b[31mfalse\\u001b[39m\",\"stack\":\"<stack>\",\"location\":\"<location>\",\"snippet\":\"<snippet>\"}}`,
`end {\"title\":\"expect.toBeTruthy\",\"category\":\"expect\",\"error\":{\"message\":\"Error: \\u001b[2mexpect(\\u001b[22m\\u001b[31mreceived\\u001b[39m\\u001b[2m).\\u001b[22mtoBeTruthy\\u001b[2m()\\u001b[22m\\n\\nReceived: \\u001b[31mfalse\\u001b[39m\",\"stack\":\"<stack>\",\"location\":\"<location>\",\"snippet\":\"<snippet>\"}}`,
`begin {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
`end {\"title\":\"After Hooks\",\"category\":\"hook\"}`,
`begin {\"title\":\"Before Hooks\",\"category\":\"hook\"}`,
Expand Down

0 comments on commit c417c52

Please sign in to comment.