Re: [whatwg/streams] Reject writer.ready promise on errors, instead of fulfilling (#526)

ricea approved this pull request.

lgtm with nits, but please wait for @tyoshino's review.

> @@ -0,0 +1,78 @@
+'use strict';
+
+if (self.importScripts) {
+  self.importScripts('/resources/testharness.js');
+  self.importScripts('../resources/test-utils.js');
+  self.importScripts('../resources/recording-streams.js');
+}
+
+const error1 = new Error('error1');
+error1.name = 'error1';
+
+promise_test(t => {
+

Unnecessary blank line?

> @@ -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>

Doesn't appear to be used? Also in importScripts bad-underlying-sinks.js below.

> @@ -0,0 +1,43 @@
+'use strict';

This file doesn't seem to be used in this commit, so I would prefer for it to be landed in a separate commit.

> +
+  const ws = recordingWritableStream({
+    close() {
+      throw error1;
+    }
+  });
+
+  const writer = ws.getWriter();
+
+  return promise_rejects(t, error1, writer.close(), 'close() promise must reject with the thrown error')
+  .then(() => promise_rejects(t, error1, writer.ready, 'ready promise must reject with the thrown error'))
+  .then(() => {
+    assert_array_equals(ws.events, ['close']);
+  });
+
+}, 'Underlying sink close: throwing method');

I prefer "should" phrasing in test descriptions, eg. 'close() and ready promises should reject when sink close() throws'.

Also I would prefer it if the test descriptions didn't contain prefixes than can be inferred from the context.

-- 
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/526#pullrequestreview-3242717

Received on Friday, 7 October 2016 04:27:22 UTC