- From: Domenic Denicola <notifications@github.com>
- Date: Thu, 13 Oct 2016 13:36:22 -0700
- To: whatwg/streams <streams@noreply.github.com>
- Message-ID: <whatwg/streams/pull/531/review/4157459@github.com>
domenic requested changes on this pull request.
Not sure if you rebased this and I missed it, or you just happened to port only tests that are already compatible with the new setup, or what, but in actuality there's no conflict here :). Left some review comments, and happy to take these conversions once we get them tidied up!
> @@ -0,0 +1,12 @@
+<!DOCTYPE html>
Let's merge the constructor tests into the general tests
> @@ -0,0 +1,15 @@
+'use strict';
+
+if (self.importScripts) {
+ self.importScripts('/resources/testharness.js');
+}
+
+test(() => {
+ const ts = new TransformStream({ transform() { } });
+}, 'TransformStream can be constructed with a transform function');
+
+test(() => {
+ const error = new TypeError('something bad');
+ assert_throws(error, () => new TransformStream(), 'TransformStream cannot be constructed with no arguments');
Nit: Use `assert_throws(new TypeError(), ...)` here instead of constructing an error with an arbitrary string.
> +
+ return ts.writable.getWriter().close().then(() => {
+ return assert_true(flushCalled, 'closing the writable triggers the transform flush immediately');
+ });
+}, 'TransformStream flush is called immediately when the writable is closed, if no writes are queued');
+
+// TODO
+test(() => {
+ let flushCalled = false;
+ const ts = new TransformStream({
+ transform() {
+ return new Promise(resolve => setTimeout(resolve, 10));
+ },
+ flush() {
+ flushCalled = true;
+ return new Promise(); // never resolves
This is not valid promise usage; it should be `new Promise(() => {})`. A preexisting problem I guess!
> + }
+ });
+
+ const writer = ts.writable.getWriter();
+ writer.write('a');
+ writer.close();
+ assert_false(flushCalled, 'closing the writable does not immediately call flush if writes are not finished');
+
+ let rsClosed = false;
+ ts.readable.getReader().closed.then(() => {
+ rsClosed = true;
+ });
+
+ setTimeout(() => {
+ assert_true(flushCalled, 'flush is eventually called');
+ assert_equals(rsClosed, false, 'if flushPromise does not resolve, the readable does not become closed');
You can use `assert_false` here.
> + c = controller;
+ },
+ transform() {
+ },
+ flush() {
+ c.enqueue('x');
+ c.enqueue('y');
+ }
+ });
+
+ const reader = ts.readable.getReader();
+
+ const writer = ts.writable.getWriter();
+ writer.write('a');
+ writer.close();
+ reader.read().then(result1 => {
return the promise here, and remove the `.catch(e => t.error(e))` (`t` is not defined)
> + flushCalled = true;
+ return new Promise(); // never resolves
+ }
+ });
+
+ const writer = ts.writable.getWriter();
+ writer.write('a');
+ writer.close();
+ assert_false(flushCalled, 'closing the writable does not immediately call flush if writes are not finished');
+
+ let rsClosed = false;
+ ts.readable.getReader().closed.then(() => {
+ rsClosed = true;
+ });
+
+ setTimeout(() => {
Instead of using setTimeout, do `return delay(50).then(...)`. This will require importing test-utils for these tests, in both the HTML and JS files. See other tests for examples.
> + transform() {
+ },
+ flush() {
+ c.enqueue('x');
+ c.enqueue('y');
+ return new Promise(resolve => setTimeout(resolve, 10));
+ }
+ });
+
+ const reader = ts.readable.getReader();
+
+ const writer = ts.writable.getWriter();
+ writer.write('a');
+ writer.close();
+
+ reader.read().then(result1 => {
Do `return Promise.all([...])` here on the two promises, with no `.catch()`s.
> + })
+ .catch(e => t.error(e));
+}, 'TransformStream flush gets a chance to enqueue more into the readable');
+
+promise_test(() => {
+ let c;
+ const ts = new TransformStream({
+ start(controller) {
+ c = controller;
+ },
+ transform() {
+ },
+ flush() {
+ c.enqueue('x');
+ c.enqueue('y');
+ return new Promise(resolve => setTimeout(resolve, 10));
Once you've imported `delay()`, you can do `return delay(10)` here.
> + reader.read().then(result1 => {
+ assert_equals(result1.value, 'x', 'the first chunk read is the first one enqueued in flush');
+ assert_equals(result1.done, false, 'the first chunk read is the first one enqueued in flush');
+
+ return reader.read().then(result2 => {
+ assert_equals(result2.value, 'y', 'the second chunk read is the second one enqueued in flush');
+ assert_equals(result2.done, false, 'the second chunk read is the second one enqueued in flush');
+ });
+ })
+ .catch(e => t.error(e));
+
+ reader.closed.then(() => {
+ assert_true(true, 'readable reader becomes closed');
+ })
+ .catch(e => { throw e; });
+}, 'TransformStream flush gets a chance to enqueue more into the readable, and can then async close');
Newline at EOF
> @@ -0,0 +1,22 @@
+'use strict';
+
+if (self.importScripts) {
+ self.importScripts('/resources/testharness.js');
+}
+
+test(() => {
+ const ts = new TransformStream({ transform() { } });
+
+ assert_true(Object.getOwnPropertyDescriptor(Object.getPrototypeOf(ts), 'writeable'), 'it has a writable property');
This existing test of the property descriptor is kind of bad. Maybe replace it with something like https://github.com/w3c/web-platform-tests/blob/a838bae2db982f96ddb6760ca88e699d4514884e/streams/readable-streams/general.js#L56-L61 instead.
> +test(() => {
+ const ts = new TransformStream({ transform() { } });
+
+ assert_true(Object.getOwnPropertyDescriptor(Object.getPrototypeOf(ts), 'writeable'), 'it has a writable property');
+ assert_true(ts.writable instanceof WritableStream, 'writable is an instance of WritableStream');
+
+ assert_true(Object.getOwnPropertyDescriptor(Object.getPrototypeOf(ts), 'readable'), 'it has a readable property');
+ assert_true(ts.readable instanceof ReadableStream, 'readable is an instance of ReadableStream');
+}, 'TransformStream instances must have writable and readable properties of the correct types');
+
+test(() => {
+ const ts = new TransformStream({ transform() { } });
+
+ const writer = ts.writable.getWriter();
+ assert_equals(writer.desiredSize, 1, 'writer.desiredSize should be 1');
+}, 'TransformStream writable starts in the writable state');
Newline at EOF
--
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/531#pullrequestreview-4157459
Received on Thursday, 13 October 2016 20:36:51 UTC