-
Notifications
You must be signed in to change notification settings - Fork 4.9k
feat: Warn on floating promises #34845
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
Conversation
…loating-promise-warning
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| 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> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Test results for "tests 1"10 flaky38623 passed, 794 skipped Merge workflow run. |
Prints a warning when a Playwright API promise that is expected to always be awaited is never attached to by the end of the test run. This warning is managed via a normal test annotation.
This could follow in a separate PR, but at the time of writing this PR includes changes to the various terminal reporters to display warnings (annotations with
type: 'warning'), grouping them by warning type. The HTML reporter doesn't have any special handling; it displays like a normal annotation right now, though we will likely change this in the future.