Reject writer.ready promise on errors, instead of fulfilling

In the process of testing this, ported several tests to web-platform-tests format, and introduced the recording stream helpers that I am using extensively in #512.

Author: domenic

index.bs

@@ -2737,8 +2737,9 @@ visible through the {{WritableStream}}'s public API.
   1. If _writer_ is not undefined,
     1. Reject _writer_.[[closedPromise]] with _e_.
     1. If _state_ is `"writable"` and !
-    WritableStreamDefaultControllerGetBackpressure(_stream_.[[writableStreamController]]) is *true*, resolve
-    _writer_.[[readyPromise]] with *undefined*.
+    WritableStreamDefaultControllerGetBackpressure(_stream_.[[writableStreamController]]) is *true*, reject
+    _writer_.[[readyPromise]] with _e_.
+    1. Otherwise, set _writer_.[[readyPromise]] to a new promise rejected with _e_.
   1. Set _stream_.[[state]] to `"errored"`.
   1. Set _stream_.[[storedError]] to _e_.
 </emu-alg>

reference-implementation/lib/writable-stream.js

@@ -139,7 +139,9 @@ function WritableStreamError(stream, e) {
 
     if (state === 'writable' &&
         WritableStreamDefaultControllerGetBackpressure(stream._writableStreamController) === true) {
-      defaultWriterReadyPromiseResolve(writer);
+      defaultWriterReadyPromiseReject(writer, e);
+    } else {
+      defaultWriterReadyPromiseResetToRejected(writer, e);
     }
   }
 

reference-implementation/test/writable-stream-abort.js

@@ -194,25 +194,6 @@ test('Aborting a WritableStream puts it in an errored state, with stored error e
   );
 });
 
-test('Aborting a WritableStream causes any outstanding ready promises to be fulfilled immediately', t => {
-  const ws = new WritableStream({
-    write() {
-      return new Promise(() => { }); // forever-pending, so normally .ready would not fulfill.
-    }
-  });
-
-  const writer = ws.getWriter();
-
-  writer.write('a');
-
-  writer.ready.then(() => {
-    t.end();
-  });
-
-  const passedReason = new Error('Sorry, it just wasn\'t meant to be.');
-  writer.abort(passedReason);
-});
-
 test('Aborting a WritableStream causes any outstanding write() promises to be rejected with the abort reason', t => {
   t.plan(1);
 

reference-implementation/test/writable-stream.js

@@ -104,154 +104,6 @@ test('If close is called on a WritableStream in waiting state, ready will be ful
   }, 0);
 });
 
-test('If sink rejects on a WritableStream in writable state, ready will return a fulfilled promise', t => {
-  let rejectSinkWritePromise;
-  const ws = new WritableStream({
-    write() {
-      return new Promise((r, reject) => {
-        rejectSinkWritePromise = reject;
-      });
-    }
-  });
-
-  setTimeout(() => {
-    const writer = ws.getWriter();
-
-    const writePromise = writer.write('a');
-
-    const passedError = new Error('pass me');
-    rejectSinkWritePromise(passedError);
-
-    writePromise.then(
-      () => {
-        t.fail('write promise was unexpectedly fulfilled');
-        t.end();
-      },
-      r => {
-        t.equal(r, passedError, 'write() should be rejected with the passed error');
-
-        writer.ready.then(v => {
-          t.equal(v, undefined, 'ready promise was fulfilled with undefined');
-          t.end();
-        });
-      }
-    );
-  }, 0);
-});
-
-test('WritableStream if sink\'s close throws', t => {
-  const passedError = new Error('pass me');
-  const ws = new WritableStream({
-    write() {
-      t.fail('Unexpected write call');
-      t.end();
-    },
-    close() {
-      throw passedError;
-    },
-    abort() {
-      t.fail('Unexpected abort call');
-      t.end();
-    }
-  });
-
-  // Wait for ws to start.
-  setTimeout(() => {
-    const writer = ws.getWriter();
-
-    const closedPromise = writer.close();
-
-    closedPromise.then(
-      () => {
-        t.fail('closedPromise is fulfilled unexpectedly');
-        t.end();
-      },
-      r => {
-        t.equal(r, passedError, 'close() should be rejected with the passed error');
-
-        writer.ready.then(v => {
-          t.equal(v, undefined, 'ready promise was fulfilled with undefined');
-          t.end();
-        });
-      }
-    );
-  }, 0);
-});
-
-test('WritableStream if the promise returned by sink\'s close rejects', t => {
-  const passedError = new Error('pass me');
-  const ws = new WritableStream({
-    write() {
-      t.fail('write of sink called');
-      t.end();
-    },
-    close() {
-      return Promise.reject(passedError);
-    },
-    abort() {
-      t.fail('abort of sink called');
-      t.end();
-    }
-  });
-
-  // Wait for ws to start.
-  setTimeout(() => {
-    const writer = ws.getWriter();
-
-    const closedPromise = writer.close();
-
-    closedPromise.then(
-      () => {
-        t.fail('closedPromise is fulfilled');
-        t.end();
-      },
-      r => {
-        t.equal(r, passedError, 'close() should be rejected with the passed error');
-
-        writer.ready.then(v => {
-          t.equal(v, undefined, 'ready promise was fulfilled with undefined');
-          t.end();
-        });
-      }
-    );
-  }, 0);
-});
-
-test('If sink\'s write rejects on a WritableStream in waiting state, ready will return a rejected promise', t => {
-  const passedError = new Error('pass me');
-  const ws = new WritableStream({
-    write(chunk) {
-      if (chunk === 'first chunk succeeds') {
-        return new Promise(resolve => setTimeout(resolve, 10));
-      }
-      return Promise.reject(passedError);
-    }
-  });
-
-  setTimeout(() => {
-    const writer = ws.getWriter();
-
-    writer.write('first chunk succeeds');
-
-    const secondWritePromise = writer.write('all other chunks fail');
-
-    secondWritePromise.then(
-      () => {
-        t.fail('write promise was unexpectedly fulfilled');
-        t.end();
-      },
-      r => {
-        t.equal(r, passedError, 'write() should be rejected with the passed error');
-
-        writer.ready.then(v => {
-          t.equal(v, undefined, 'ready promise was fulfilled with undefined');
-          t.end();
-        });
-      }
-    );
-  }, 0);
-});
-
 test('WritableStream should call underlying sink methods as methods', t => {
   t.plan(5);
 

reference-implementation/to-upstream-wpts/resources/recording-streams.js

@@ -0,0 +1,74 @@
+"use strict";
+
+self.recordingReadableStream = (extras = {}, strategy) => {
+  let controllerToCopyOver;
+  const stream = new ReadableStream({
+    start(controller) {
+      controllerToCopyOver = controller;
+
+      if (extras.start) {
+        return extras.start(controller);
+      }
+    },
+    pull(controller) {
+      stream.events.push('pull');
+
+      if (extras.pull) {
+        return extras.pull(controller);
+      }
+    },
+    cancel(reason) {
+      stream.events.push('cancel', reason);
+      stream.eventsWithoutPulls.push('cancel', reason);
+
+      if (extras.cancel) {
+        return extras.cancel(reason);
+      }
+    }
+  }, strategy);
+
+  stream.controller = controllerToCopyOver;
+  stream.events = [];
+  stream.eventsWithoutPulls = [];
+
+  return stream;
+};
+
+self.recordingWritableStream = (extras = {}, strategy) => {
+  let controllerToCopyOver;
+  const stream = new WritableStream({
+    start(controller) {
+      controllerToCopyOver = controller;
+
+      if (extras.start) {
+        return extras.start(controller);
+      }
+    },
+    write(chunk) {
+      stream.events.push('write', chunk);
+
+      if (extras.write) {
+        return extras.write(chunk);
+      }
+    },
+    close() {
+      stream.events.push('close');
+
+      if (extras.close) {
+        return extras.close();
+      }
+    },
+    abort(e) {
+      stream.events.push('abort', e);
+
+      if (extras.abort) {
+        return extras.abort(e);
+      }
+    }
+  }, strategy);
+
+  stream.controller = controllerToCopyOver;
+  stream.events = [];
+
+  return stream;
+};

reference-implementation/to-upstream-wpts/resources/test-utils.js

@@ -0,0 +1,44 @@
+'use strict';
+// This file already exists upstream. We are duplicating it here due to tooling limitations.
+
+self.getterRejects = (t, obj, getterName, target) => {
+  const getter = Object.getOwnPropertyDescriptor(obj, getterName).get;
+
+  return promise_rejects(t, new TypeError(), getter.call(target));
+};
+
+self.methodRejects = (t, obj, methodName, target) => {
+  const method = obj[methodName];
+
+  return promise_rejects(t, new TypeError(), method.call(target));
+};
+
+self.getterThrows = (obj, getterName, target) => {
+  const getter = Object.getOwnPropertyDescriptor(obj, getterName).get;
+
+  assert_throws(new TypeError(), () => getter.call(target), getterName + ' should throw a TypeError');
+};
+
+self.methodThrows = (obj, methodName, target, args) => {
+  const method = obj[methodName];
+
+  assert_throws(new TypeError(), () => method.apply(target, args), methodName + ' should throw a TypeError');
+};
+
+self.garbageCollect = () => {
+  if (self.gc) {
+    // Use --expose_gc for V8 (and Node.js)
+    // Exposed in SpiderMonkey shell as well
+    self.gc();
+  } else if (self.GCController) {
+    // Present in some WebKit development environments
+    GCController.collect();
+  } else {
+    /* eslint-disable no-console */
+    console.warn('Tests are running without the ability to do manual garbage collection. They will still work, but ' +
+      'coverage will be suboptimal.');
+    /* eslint-enable no-console */
+  }
+};
+
+self.delay = ms => new Promise(resolve => step_timeout(resolve, ms));

reference-implementation/to-upstream-wpts/writable-streams/aborting.https.html

@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/service-workers/service-worker/resources/test-helpers.sub.js"></script>
+<script src="../resources/test-initializer.js"></script>
+
+<script src="aborting.js"></script>
+<script>
+'use strict';
+worker_test('aborting.js');
+</script>

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

@@ -0,0 +1,47 @@
+'use strict';
+
+if (self.importScripts) {
+  self.importScripts('/resources/testharness.js');
+}
+
+const error1 = new Error('error1');
+error1.name = 'error1';
+
+promise_test(t => {
+
+  const ws = new WritableStream({
+    write() {
+      return new Promise(() => { }); // forever-pending, so normally .ready would not fulfill.
+    }
+  });
+
+  const writer = ws.getWriter();
+  writer.write('a');
+
+  const readyPromise = writer.ready;
+
+  writer.abort(error1);
+
+  assert_equals(writer.ready, readyPromise, 'the ready promise property should not change');
+
+  return promise_rejects(t, new TypeError(), readyPromise, 'the ready promise should reject with a TypeError');
+
+}, 'Aborting a WritableStream should cause the writer\'s unsettled ready promise to reject');
+
+promise_test(t => {
+
+  const ws = new WritableStream();
+
+  const writer = ws.getWriter();
+  writer.write('a');
+
+  const readyPromise = writer.ready;
+
+  return readyPromise.then(() => {
+    writer.abort(error1);
+
+    assert_not_equals(writer.ready, readyPromise, 'the ready promise property should change');
+    return promise_rejects(t, new TypeError(), writer.ready, 'the ready promise should reject with a TypeError');
+  });
+
+}, 'Aborting a WritableStream should cause the writer\'s fulfilled ready promise to reset to a rejected one');

reference-implementation/to-upstream-wpts/writable-streams/bad-underlying-sinks.https.html

@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/service-workers/service-worker/resources/test-helpers.sub.js"></script>
+<script src="../resources/test-utils.js"></script>
+<script src="../resources/recording-streams.js"></script>
+<script src="../resources/test-initializer.js"></script>
+
+<script src="bad-underlying-sinks.js"></script>
+<script>
+'use strict';
+worker_test('bad-underlying-sinks.js');
+</script>