Re: [whatwg/streams] Convert readable byte stream tests to WPT (#648)

domenic commented on this pull request.

Overall looks good, but async_test has some subtleties and is best converted to promise_test when possible.

> +
+test(() => {
+  assert_throws(new TypeError(), () => new ReadableStream().getReader({ mode: 'byob' }));
+}, 'getReader({mode: "byob"}) throws on non-bytes streams');
+
+
+test(() => {
+  // Constructing ReadableStream with an empty underlying byte source object as parameter shouldn't throw.
+  new ReadableStream({ type: 'bytes' });
+}, 'ReadableStream with byte source can be constructed with no errors');
+
+async_test(t => {
+  let startCalled = false;
+  let controller;
+
+  new ReadableStream({

Because of how the async_test framework works, I believe you need to wrap any functions called async that could possibly throw, such as `pull`, in `t.step_func`

> +
+test(() => {
+  assert_throws(new TypeError(), () => new ReadableStream().getReader({ mode: 'byob' }));
+}, 'getReader({mode: "byob"}) throws on non-bytes streams');
+
+
+test(() => {
+  // Constructing ReadableStream with an empty underlying byte source object as parameter shouldn't throw.
+  new ReadableStream({ type: 'bytes' });
+}, 'ReadableStream with byte source can be constructed with no errors');
+
+async_test(t => {
+  let startCalled = false;
+  let controller;
+
+  new ReadableStream({

In general you may be able to replace some of these with uses of the recording stream helpers, modified to allow `type: 'bytes'`. But maybe not.

> +  });
+
+}, 'ReadableStream with byte source: No automatic pull call if start doesn\'t finish');
+
+async_test(t => {
+  new ReadableStream({
+    pull() {
+      assert_unreached('pull must not be called');
+      t.done();
+    },
+    type: 'bytes'
+  }, {
+    highWaterMark: 0
+  });
+
+  Promise.resolve().then(() => {

Looks like this could be a promise_test

> +  });
+
+  Promise.resolve().then(() => {
+    t.done();
+  });
+}, 'ReadableStream with byte source: Construct with highWaterMark of 0');
+
+async_test(t => {
+  const stream = new ReadableStream({
+    type: 'bytes'
+  });
+
+  const reader = stream.getReader();
+  reader.releaseLock();
+
+  reader.closed.then(() => {

Should be a promise_test with promise_rejects. Same for most of the async_tests that follow and end by checking reader.closed.

> +    },
+    pull() {
+      assert_not_equals(controller.byobRequest, undefined, 'byobRequest must not be undefined');
+      throw testError;
+    },
+    type: 'bytes'
+  });
+
+  const reader = stream.getReader({ mode: 'byob' });
+
+  const promise = promise_rejects(t, testError, reader.read(new Uint8Array(1)), 'read(view) must fail');
+  return promise_rejects(t, testError, promise.then(() => reader.closed));
+}, 'ReadableStream with byte source: Throwing in pull in response to read(view) function must error the stream');
+
+promise_test(t => {
+  const passedError = new TypeError('foo');

I've found it a nicer pattern, at the top of the file, create an `error1` and maybe `error2`, and then you can use assert_throws/promise_rejects more easily. See e.g. https://github.com/whatwg/streams/blob/b412a96bbb5905b1abcc25c8032439a6c081e96c/reference-implementation/to-upstream-wpts/writable-streams/aborting.js (Ctrl+F "error1")

I guess this is a preexisting issue so no big deal if you'd rather not do this cleanup.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/648#pullrequestreview-16677631