Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(test runner): allow to pass arbitrary location to test.step #32504

Merged
merged 18 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from 17 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
6 changes: 6 additions & 0 deletions docs/src/test-api/class-test.md
Original file line number Diff line number Diff line change
Expand Up @@ -1710,6 +1710,12 @@ Step body.

Whether to box the step in the report. Defaults to `false`. When the step is boxed, errors thrown from the step internals point to the step call site. See below for more details.

### option: Test.step.location
* since: v1.50
- `location` <[Object]>

Specifies a custom location for a test step as { file, line, column }, enabling precise error reporting within user code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### option: Test.step.location
* since: v1.50
- `location` <[Object]>
Specifies a custom location for a test step as { file, line, column }, enabling precise error reporting within user code.
### option: Test.step.location
* since: v1.48
- `location` <[Location]>
Specifies a custom location for the step to be shown in test reports. By default, location of the [`method: Test.step`] call is shown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgozman Thanks for the quick response to my modified commit! I will immediately reflect this part of your answer :)

## method: Test.use
* since: v1.10

Expand Down
4 changes: 2 additions & 2 deletions packages/playwright/src/common/testType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,11 +259,11 @@ export class TestTypeImpl {
suite._use.push({ fixtures, location });
}

async _step<T>(title: string, body: () => Promise<T>, options: { box?: boolean } = {}): Promise<T> {
async _step<T>(title: string, body: () => Promise<T>, options: {box?: boolean, location?: Location } = {}): Promise<T> {
const testInfo = currentTestInfo();
if (!testInfo)
throw new Error(`test.step() can only be called from a test`);
const step = testInfo._addStep({ category: 'test.step', title, box: options.box });
const step = testInfo._addStep({ category: 'test.step', title, location: options.location, box: options.box });
return await zones.run('stepZone', step, async () => {
try {
const result = await body();
Expand Down
2 changes: 1 addition & 1 deletion packages/playwright/types/test.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4703,7 +4703,7 @@ export interface TestType<TestArgs extends KeyValue, WorkerArgs extends KeyValue
* @param body Step body.
* @param options
*/
step<T>(title: string, body: () => T | Promise<T>, options?: { box?: boolean }): Promise<T>;
step<T>(title: string, body: () => T | Promise<T>, options?: { box?: boolean, location?: Location }): Promise<T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that Location here corresponds to the global lib-dom Location, as opposed to the Playwright's Location interface.

I'd suggest moving the definition of Playwright's Location from testReporter.d.ts to playwright/types/test.d.ts since it's now part of the public API.

@dgozman - what's your view?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at it along with the issues you mentioned, I think it's right to change like you said.
The maintainers fixed the bug according to your issue. Thank you for the review. 👍🏻👍🏻

/**
* `expect` function can be used to create test assertions. Read more about [test assertions](https://playwright.dev/docs/test-assertions).
*
Expand Down
39 changes: 39 additions & 0 deletions tests/playwright-test/test-step.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1239,3 +1239,42 @@ fixture | fixture: page
fixture | fixture: context
`);
});

test('test location to test.step', async ({ runInlineTest }) => {
const result = await runInlineTest({
'reporter.ts': stepIndentReporter,
'helper.ts': `
import { test } from '@playwright/test';

export async function dummyStep(test, title, action, location) {
return await test.step(title, action, { location });
}

export function getCustomLocation() {
return { file: 'dummy-file.ts', line: 123, column: 45 };
}
`,
'playwright.config.ts': `
module.exports = {
reporter: './reporter',
};
`,
'a.test.ts': `
import { test } from '@playwright/test';
import { dummyStep, getCustomLocation } from './helper';

test('custom location test', async () => {
const location = getCustomLocation();
await dummyStep(test, 'Perform a dummy step', async () => {
}, location);
});
`
}, { reporter: '', workers: 1 });

expect(result.exitCode).toBe(0);
expect(stripAnsi(result.output)).toBe(`
hook |Before Hooks
test.step |Perform a dummy step @ dummy-file.ts:123
hook |After Hooks
`);
});
2 changes: 1 addition & 1 deletion utils/generate_types/overrides-test.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ export interface TestType<TestArgs extends KeyValue, WorkerArgs extends KeyValue
afterAll(inner: (args: TestArgs & WorkerArgs, testInfo: TestInfo) => Promise<any> | any): void;
afterAll(title: string, inner: (args: TestArgs & WorkerArgs, testInfo: TestInfo) => Promise<any> | any): void;
use(fixtures: Fixtures<{}, {}, TestArgs, WorkerArgs>): void;
step<T>(title: string, body: () => T | Promise<T>, options?: { box?: boolean }): Promise<T>;
step<T>(title: string, body: () => T | Promise<T>, options?: { box?: boolean, location?: Location }): Promise<T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to this, you have to update documentation at docs/src/test-api/class-test.md.

expect: Expect<{}>;
extend<T extends KeyValue, W extends KeyValue = {}>(fixtures: Fixtures<T, W, TestArgs, WorkerArgs>): TestType<TestArgs & T, WorkerArgs & W>;
info(): TestInfo;
Expand Down
Loading