[WIP] ReadableStream @@asyncIterator tests#13362
[WIP] ReadableStream @@asyncIterator tests#13362devsnek wants to merge 4 commits intoweb-platform-tests:masterfrom
Conversation
wpt-pr-bot
requested review from
calvaris,
domenic,
ricea,
wanderview,
youennf and
yutakahirano
|
If you run node generate-test-wrappers.js readable-streams/async-iterator.js in the parent directory it will generate the rest of the wrapper files for you. |
ricea
left a comment
ricea
left a comment
There was a problem hiding this comment.
Great overall. Just a few minor comments.
|
|
||
| try { | ||
| for await (const chunk of s) {} | ||
| assert(false); |
There was a problem hiding this comment.
You should use assert_unreached() here and in other places where you have assert(false).
|
|
||
| promise_test(async () => { | ||
| const s = new ReadableStream({ | ||
| start(s) { |
There was a problem hiding this comment.
Using the same name for the start() parameter and ReadableStream object is confusion. I suggest using c or controller for the paramter.
| } | ||
| }); | ||
|
|
||
| await (async () => { |
There was a problem hiding this comment.
I find this immediately-executed-async-function structure hard to understand. Is there a reason why a simple try ... catch wouldn't work?
There was a problem hiding this comment.
testing return. I suppose it can be reworked a little bit though.
There was a problem hiding this comment.
Probably if you just split the creation of the function from the execution of the function it will be enough to make it less confusing.
|
|
||
| promise_test(async () => { | ||
| { | ||
| const s = new ReadableStream({}); |
There was a problem hiding this comment.
Nitpick: you don't actually need the {} here.
| }, '@@asyncIterator() method is === to getIterator() method'); | ||
|
|
||
| promise_test(async () => { | ||
| const s = recordingReadableStream({ |
There was a problem hiding this comment.
This test uses recordingReadableStream() but never looks at `s.events'.
| }, | ||
| }); | ||
| const it = s.getIterator(); | ||
| assert_throws(() => { |
There was a problem hiding this comment.
The first argument to assert_throws() needs to match the exception. In this case new TypeError() should be the first argument.
| } | ||
| })().catch(() => 0); | ||
|
|
||
| assert_equals(s.locked, preventCancel); |
There was a problem hiding this comment.
It's easier to debug assert failures if they have a description. In this case, I'd use something like `s.locked should equal preventCancel when type = ${type} and preventCancel = ${preventCancel}`.
In this case, the assertion is wrong because s.locked is always false after the loop terminates. The easiest way to detect if preventCancel worked is to look at s.events. Something like this should work:
if (preventCancel) {
assert_array_equals(s.events, [], `cancel() should not be called when type = '${type}' and preventCancel is true`);
} else {
assert_array_equals(s.events, ['cancel', undefined], `cancel() should be called when type = '${type}' and preventCancel is false`);
}
|
I uploaded some brand-checks.js tests to this branch. I hope they don't cause any conflicts. |
|
interestingly, the |
ricea
left a comment
ricea
left a comment
There was a problem hiding this comment.
interestingly, the assert_equals(Object.getPrototypeOf(next), Object.prototype); test is failing with the reference implementation.
I think this is a result of the way the wpt-runner works. ReadableStream is compiled with a different global, so its Object is different from the Object of the page.
Unfortunately, this currently will mean including this check will prevent any further changes to the streams standard from landing. But maybe we should do that and then fix it over there.
| } | ||
| }); | ||
|
|
||
| await (async () => { |
There was a problem hiding this comment.
Probably if you just split the creation of the function from the execution of the function it will be enough to make it less confusing.
3 participants
Refs whatwg/streams#954