wpt
wpt copied to clipboard
web-platform-tests
Use promise_test in idlharness
In #24363, we ran into a bug where a rejected promise was not correctly caught by the IDL harness. We fixed it by adding a .step_func() call, but it still feels weird having to manually add these.
This PR refactors all usages of async_test to promise_test in the IDL harness, leveraging async/await to easily chain promises within the tests. We no longer need any manual calls to a_test.step() or a_test.done(), since promise_test handles all of that for us. 😁
Right, so apparently this violates the policy set out in #6266, in that we should avoid using modern JavaScript features in the harness. This resulted in a failure on the harness tests.
It's 2020: async/await has been in the language since ES2017 and is supported by all browsers... Do we still need the harness to work without async/await support, or even without Promise support (#11150)? 😕
Hi Mattias, sorry for the delay in this being reviewed >_<.
I fully agree that using async/await in 2020 is totally reasonable, and have argued as such in https://github.com/web-platform-tests/wpt/pull/6333 just recently. So I'm hopeful that won't turn out to be an issue.
@jgraham - WDYT on using async/await in idlharness.js?
I have some concerns about the potential performance implications of moving from concurrent async_tests to sequential promise_tests, but that doesn't seem borne out by the wpt.fyi results. (They seem pretty much equivalent for the bigger IDL cases, e.g. html/).
Looks like I forgot about this PR. Sorry! 😅
Seeing that the policy around testharness.js has been updated in #26204 and async/await is now officially allowed, I think this PR is still worth considering. I've rebased it, should be ready for review. 🙂
Hmm, it seems the idlharness.js tests are failing because we run many of them both with and without a global Promise.
So... idlharness.js is not allowed to use promise_test, and we can't use async/await after all? 😞
Or has this comment become outdated, and should we as of now require a global Promise? After all, it references #6266, which is now superseded by #26204, right?
@MattiasBuelens in https://github.com/web-platform-tests/rfcs/blob/master/rfcs/promise.md we decided to no longer support browsers/runtimes that don't have Promise. However, it seems like the tests were never cleaned up...
If you'd like to delete those tests, feel free!
I've gone ahead and sent https://github.com/web-platform-tests/wpt/pull/33507 to clean that up.
Thanks! I didn't know we had RFCs for web-platform-tests, TIL. 😁
I'll rebase this PR after yours lands. 👍
I'm amazed this apparently doesn't have any merge conflicts, but also not totally surprised.
This feels potentially still worthwhile to land, though maybe we should check this doesn't change any results now.
All that leaves is finding someone happy to review idlharness changes (though this is maybe simpler than most!).
