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

Conversation

osohyun0224
Copy link
Contributor

@osohyun0224 osohyun0224 commented Sep 8, 2024

Fixes #30160

Description:

This pull request introduces the ability to specify custom locations for test steps in Playwright. By enabling the provision of arbitrary locations to the test.step method, it resolves the limitation where helper methods obfuscate the original call site, providing more accurate and meaningful location data in test reports.

Motivation:

To enhance the utility and clarity of test reports in Playwright. Specifically, it addresses the need to trace test steps back to their precise location in the code, which is especially important when steps are abstracted in helper functions. This feature is crucial for maintaining accurate documentation and facilitating debugging processes.

Changes:

Added functionality to pass a custom location object to test.step.

Expected Outcome:

This PR is expected to significantly improve the precision and usefulness of diagnostic data in test reports by allowing specific locations within helper functions to be accurately documented. It facilitates better tracking of test executions and simplifies the debugging process, making it easier for developers to understand and address issues within complex tests.

References:

Closes #30160 - "[Feature]: allow to pass arbitrary location to test.step"

Code Check
I conducted tests on this new feature by integrating it into some existing test codes, and it worked well. I will attach the code used for testing and a screenshot showing the successful outcome.

�toggle dropdown
import type { Location } from '../../../packages/playwright/types/testReporter'
...
test('should respect the back button', async ({ page }) => {
    await page.locator('.todo-list li .toggle').nth(1).check();
    await checkNumberOfCompletedTodosInLocalStorage(page, 1);
...
    await test.step('Showing active items', async () => {
      await page.getByRole('link', { name: 'Active' }).click();
    }, {location});
image

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! We will need a test for this - take a look at test-step.spec.ts for inspiration.

examples/todomvc/tests/integration.spec.ts Outdated Show resolved Hide resolved
packages/playwright/src/common/fixtures.ts Outdated Show resolved Hide resolved
utils/generate_types/overrides-test.d.ts Outdated Show resolved Hide resolved
@@ -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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@osohyun0224
Copy link
Contributor Author

@dgozman hello:)

First of all, thank you so much for actively reviewing my PR before commenting! I think it was an opportunity for me to improve further thanks to your review.

I accepted your review and reverted the entire implementation, adding test code to the test-step.steps.ts file and class-test.md. I wrote a description of the features in.

Can I ask you to review again focusing on this part? Thanks again for reviewing my PR.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

The code looks good, but docs need some work, and tests do not pass. Thank you for working on this!

docs/src/test-api/class-test.md Outdated Show resolved Hide resolved

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

Looks great, just a single suggestion for the docs.

Comment on lines 1713 to 1718
### 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 :)

Copy link
Contributor

Test results for "tests 1"

1 failed
❌ [playwright-test] › babel.spec.ts:135:5 › should not transform external

2 flaky ⚠️ [webkit-library] › library/download.spec.ts:698:3 › should convert navigation to a resource with unsupported mime type into download
⚠️ [webkit-library] › library/video.spec.ts:581:5 › screencast › should capture static page in persistent context @smoke

35475 passed, 659 skipped
✔️✔️✔️

Merge workflow run.

@dgozman dgozman changed the title feat :: allow to pass arbitrary location to test.step feat(test runner): allow to pass arbitrary location to test.step Sep 17, 2024
Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, looks great!

@dgozman dgozman merged commit 8761daf into microsoft:main Sep 17, 2024
28 of 30 checks passed
@@ -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. 👍🏻👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: allow to pass arbitrary location to test.step
3 participants