-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update Browser/BrowserContext API:s to return Promise #44
Conversation
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.
Wow! Thanks a lot! I'm not sure how to review this, though (too many changes! 🙄). So, I trust you 👍
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.
So all the tests are ran concurrently now (at least the ones that work with promise), but within an async function which is awaited on.
Looks good 👍
How did you get that? |
@inancgumus Oh, I see. The reason for the amount of changes is that I wrapped all of the test-cases in an async function, because I had to do Otherwise it's just updating the I have run the tests and they all pass. 🙂 |
Got it 👍 Makes sense now. I'd suggest doing each step in a separate commit with a description so that we can easily see and review the changes. For example:
|
@ankur22 The function is actually never executed, it's just linted using the typescript type checker. So it doesn't matter if you await or not, unless you want to continue testing on some return value (e.g. |
Please fill in this template.
pnpm test <package to test>
.