Re: [whatwg/streams] Port writable stream abort tests to wpt (#540)

domenic requested changes on this pull request.



> @@ -1,14 +1,16 @@
 'use strict';
+/* global delay, recordingWritableStream */

If you rebase this will not be necessary

>  
 if (self.importScripts) {
+  self.importScripts('../resources/test-utils.js');

Move this after testharness.js

> +  return promise_rejects(t, new TypeError(), writer.abort(), 'abort() should reject with a TypeError');
+}, 'abort() on a released writer rejects');
+
+promise_test(() => {
+  const ws = recordingWritableStream();
+
+  return delay(0)
+    .then(() => {
+      const writer = ws.getWriter();
+
+      writer.abort();
+      writer.write(1);
+      writer.write(2);
+    })
+    .then(() => {
+      assert_equals(ws.events.length, 2, 'the stream should have received 2 events');

You should use assert_array_equals here like the other recording stream tests. Same below.

The events[1] entry is the argument passed to abort, by the way, and is not related to writes.

> +  return abortPromise.then(value => {
+    assert_equals(value, undefined, 'fulfillment value must be undefined');
+  });
+}, 'Fulfillment value of ws.abort() call must be undefined even if the underlying sink returns a non-undefined value');
+
+promise_test(t => {
+  const errorInSinkAbort = new Error('Sorry, it just wasn\'t meant to be.');
+  const ws = new WritableStream({
+    abort() {
+      throw errorInSinkAbort;
+    }
+  });
+
+  const writer = ws.getWriter();
+
+  return promise_rejects(t, errorInSinkAbort, writer.abort(undefined),

Can you use the `error1` pattern used elsewhere? With a single `error1` at the top of the file? Here and below.

Also everywhere that creates a separate `passedReason` can use that `error1`.

> +}, 'WritableStream if sink\'s abort throws, the promise returned by ws.abort() rejects');
+
+test(() => {
+  let recordedReason;
+  const ws = new WritableStream({
+    abort(reason) {
+      recordedReason = reason;
+    }
+  });
+
+  const writer = ws.getWriter();
+
+  const passedReason = new Error('Sorry, it just wasn\'t meant to be.');
+  writer.abort(passedReason);
+
+  assert_equals(recordedReason, passedReason);

This test can use recordingWritableStream instead, with assert_array_equals on the contents.

> +}, 'Aborting a WritableStream passes through the given reason');
+
+promise_test(t => {
+  const ws = new WritableStream();
+  const writer = ws.getWriter();
+
+  const passedReason = new Error('Sorry, it just wasn\'t meant to be.');
+  writer.abort(passedReason);
+
+  return Promise.all([
+    promise_rejects(t, new TypeError(), writer.write(), 'writing should reject with the given reason'),
+    promise_rejects(t, new TypeError(), writer.close(), 'closing should reject with the given reason'),
+    promise_rejects(t, new TypeError(), writer.abort(), 'aborting should reject with the given reason'),
+    promise_rejects(t, new TypeError(), writer.closed, 'closed should reject with the given reason'),
+  ]);
+}, 'Aborting a WritableStream puts it in an errored state, with stored error equal to the abort reason');

This test description is outdated; change it to "with a TypeError as the stored error"

> +promise_test(t => {
+  const ws = new WritableStream();
+  const writer = ws.getWriter();
+
+  const passedReason = new Error('Sorry, it just wasn\'t meant to be.');
+  writer.abort(passedReason);
+
+  return Promise.all([
+    promise_rejects(t, new TypeError(), writer.write(), 'writing should reject with the given reason'),
+    promise_rejects(t, new TypeError(), writer.close(), 'closing should reject with the given reason'),
+    promise_rejects(t, new TypeError(), writer.abort(), 'aborting should reject with the given reason'),
+    promise_rejects(t, new TypeError(), writer.closed, 'closed should reject with the given reason'),
+  ]);
+}, 'Aborting a WritableStream puts it in an errored state, with stored error equal to the abort reason');
+
+// TODO return promise

Just store the promise with `const writePromise = promise_rejects(...)` and then return it as the last line

> +    promise_rejects(t, new TypeError(), writer.close(), 'closing should reject with the given reason'),
+    promise_rejects(t, new TypeError(), writer.abort(), 'aborting should reject with the given reason'),
+    promise_rejects(t, new TypeError(), writer.closed, 'closed should reject with the given reason'),
+  ]);
+}, 'Aborting a WritableStream puts it in an errored state, with stored error equal to the abort reason');
+
+// TODO return promise
+promise_test(t => {
+  const ws = new WritableStream();
+  const writer = ws.getWriter();
+
+  promise_rejects(t, new TypeError(), writer.write('a'), 'writing should reject with a TypeError');
+
+  const passedReason = new Error('Sorry, it just wasn\'t meant to be.');
+  writer.abort(passedReason);
+}, 'Aborting a WritableStream causes any outstanding write() promises to be rejected with the abort reason');

Test description is again outdated; "with a TypeError"

> +  promise_rejects(t, new TypeError(), writer.write('a'), 'writing should reject with a TypeError');
+
+  const passedReason = new Error('Sorry, it just wasn\'t meant to be.');
+  writer.abort(passedReason);
+}, 'Aborting a WritableStream causes any outstanding write() promises to be rejected with the abort reason');
+
+promise_test(t => {
+  const ws = new WritableStream();
+  const writer = ws.getWriter();
+
+  writer.close();
+
+  const passedReason = new Error('Sorry, it just wasn\'t meant to be.');
+  writer.abort(passedReason);
+
+  return promise_rejects(t, new TypeError('Aborted'), writer.closed, 'the stream should be errored with a TypeError');

Don't include `'Aborted'` here; the test framework doesn't look at messages. (Yeah, it's kind of wonky.)

> +  }, 20);
+
+  return promise_rejects(t, new TypeError(), writer.closed, 'the stream should be errored with a TypeError');
+}, 'Closing a WritableStream and aborting it while it closes causes the stream to error');
+
+promise_test(() => {
+  const ws = new WritableStream();
+  const writer = ws.getWriter();
+
+  writer.close();
+
+  return delay(0)
+    .then(() => writer.abort())
+    .then(
+      v => assert_equals(v, undefined, 'abort promise should fulfill with undefined'),
+      () => { throw new Error(); }

This `() => { throw new Error(); }` is not necessary. In fact the second `.then()` can be removed entirely since we already test the undefinedness earlier.

> +    .then(
+      v => assert_equals(v, undefined, 'abort promise should fulfill with undefined'),
+      () => { throw new Error(); }
+    );
+}, 'Aborting a WritableStream after it is closed is a no-op');
+
+test(() => {
+  const ws = new WritableStream({
+    close(...args) {
+      assert_equals(args.length, 0, 'close() was called (with no arguments)');
+    }
+  });
+
+  const writer = ws.getWriter();
+
+  writer.abort();

Again best to use recordingWritableStream. As-is, this test will pass even if close is never called.

It might be good to add something to recordingWritableStream's close() that asserts that `arguments.length === 0`.

-- 
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/540#pullrequestreview-5161439