Streams: Add/update WPTs with AbortSignal's abort reason #31400
Conversation
wpt-pr-bot
requested review from
domenic,
ricea,
wanderview,
youennf and
yutakahirano
streams/piping/abort.any.js
Outdated
| assert_equals(rs.events[0], 'cancel', 'first event should be cancel'); | ||
| assert_equals(rs.events[1].name, reason, 'the argument to cancel should be reason'); | ||
| }, 'an aborted signal should cause the writable stream to reject with the signal\'s abort reason'); | ||
|
|
There was a problem hiding this comment.
Maybe we want to have a test for the case where the pipeTo operation is aborted after it started.
There was a problem hiding this comment.
Other tests in this file already handle such cases. However, they always call abortController.abort(). Should we parameterize some of these tests, so you can run them with abort() or with abort(reason)? 🤔
There was a problem hiding this comment.
That's a fair point. A lot of the tests are either of the form "prevent... should prevent ....", "abort signal takes priority over ...", or "abort should do nothing after ..." which IIUC doesn't really make sense to test for the abort reason specifically. I've looked through the rest and tried parameterizing a few that seemed useful to me, but if you think there's any others I should do as well, please let me know :)
streams/piping/abort.any.js
Outdated
| ws.getWriter().closed.catch(e => { | ||
| assert_equals(e, error, 'the writable should be errored with the same object'); | ||
| for (const reason of [null, undefined, error1]) { | ||
| promise_test(() => { |
There was a problem hiding this comment.
How about making this an async function?
There was a problem hiding this comment.
I kept the tests using Promise chains to stay consistent because all of the other tests in the file also used them. Although I agree using async/await is slightly more readable, would it be weird if some tests use async/await, while the rest still have Promise chains? (just wondering because I'm not sure what the norm is)
There was a problem hiding this comment.
Nevermind, I've made it an async function now, but if you think I should change it back, please let me know.
There was a problem hiding this comment.
would it be weird if some tests use async/await, while the rest still have Promise chains?
I don't think that's a problem. @ricea do you have any opinions?
There was a problem hiding this comment.
I actually just talked to Adam offline, and he mentioned preferring async/await in newer tests now.
|
Thank you for reviewing! |
See whatwg/streams#1182 for the accompanying spec change.
This PR updates the existing aborting test cases to check the
ctrl.signal.reasonagain, which was initially removed in #31293 whenabortReasonwas removed from the spec.I've also parameterized a few test cases to check the AbortSignal's abort reason is properly used in
rs.pipeTo()calls.See whatwg/streams#1182 for the accompanying spec change.
/cc @yutakahirano, @MattiasBuelens