Re: [whatwg/streams] Add tests for TransformStream backpressure (#773)

domenic commented on this pull request.

I'd kind of like to see the result with a highWaterMark of, say, 3, tested: watch it go 3, 2, 1, 0, -1, etc. There are some tests like this for readable and writable streams. This batch has mostly the default values, plus a zero and an infinity.

On the other hand, maybe adding such tests is redundant with the fact that we already have them for readable/writable streams, and this is just a composition. I'm unsure. What do you think?

> +      if (extras.flush) {
+        return extras.flush(controller);
+      }
+
+      return undefined;
+    }
+  }, writableStrategy, readableStrategy);
+
+  stream.controller = controllerToCopyOver;
+  stream.events = [];
+
+  return stream;
+};
+
+self.recordingIdentityTransformStream = (writableStrategy, readableStrategy) => {
+  const stream = self.recordingTransformStream({

Nit: I'd just `return` this; no need to create a variable

> @@ -0,0 +1,112 @@
+'use strict';
+
+if (self.importScripts) {
+  self.importScripts('/resources/testharness.js', '../resources/recording-streams.js', '../resources/test-utils.js');

I'm not sure the wrapper-generator is smart enough to deal with the multi-arg form of importScripts. I think it might be better to be consistent and go with the single-arg form anyway.

> @@ -0,0 +1,112 @@
+'use strict';
+
+if (self.importScripts) {
+  self.importScripts('/resources/testharness.js', '../resources/recording-streams.js', '../resources/test-utils.js');
+}
+
+promise_test(() => {
+  const ts = recordingIdentityTransformStream();
+  const writer = ts.writable.getWriter();
+  writer.write('a');
+  writer.write('b');
+  return delay(0).then(() => {
+    assert_array_equals(ts.events, ['transform', 'a'], 'transform should be called once');
+  });
+}, 'transform() should be called once with default identity transform');

I don't think this test name is really reflecting what is going on. Basically this is testing that transform() is called synchronously once and the second chunk gets delayed async. Right?

> +if (self.importScripts) {
+  self.importScripts('/resources/testharness.js', '../resources/recording-streams.js', '../resources/test-utils.js');
+}
+
+promise_test(() => {
+  const ts = recordingIdentityTransformStream();
+  const writer = ts.writable.getWriter();
+  writer.write('a');
+  writer.write('b');
+  return delay(0).then(() => {
+    assert_array_equals(ts.events, ['transform', 'a'], 'transform should be called once');
+  });
+}, 'transform() should be called once with default identity transform');
+
+promise_test(() => {
+  // Without a transform() implementation, recordingTransformStream() never enqueues anything.

Let's expand this, as for a few seconds I thought "doesn't that mean the pipe will just get clogged, so there _will_ be backpressure?"

I'd go with

> That is, we're just throwing away chunks so the readable side is never full enough to exert backpressure and stop transform() from being called.

> +
+promise_test(() => {
+  const ts = new TransformStream();
+  const writer = ts.writable.getWriter();
+  const reader = ts.readable.getReader();
+  const events = [];
+  writer.write('a').then(() => events.push('a'));
+  writer.write('b').then(() => events.push('b'));
+  writer.close().then(() => events.push('closed'));
+  return flushAsyncEvents().then(() => {
+    assert_array_equals(events, [], 'no writes should have resolved yet');
+    return reader.read();
+  }).then(({ value, done }) => {
+    assert_false(done, 'done should not be true');
+    assert_equals('a', value, 'value should be "a"');
+    return delay(0);

I still don't know how to distinguish flushAsyncEvents() vs. delay(0) :-/. Could you remind me?

> +    assert_array_equals(events, ['a'], 'the first write should have resolved');
+    return reader.read();
+  }).then(({ value, done }) => {
+    assert_false(done, 'done should still not be true');
+    assert_equals('b', value, 'value should be "b"');
+    return delay(0);
+  }).then(() => {
+    assert_array_equals(events, ['a', 'b', 'closed'], 'the second write and close should be resolved');
+    return reader.read();
+  }).then(({ done }) => {
+    assert_true(done, 'done should be true');
+  });
+}, 'writes should not resolve until backpressure clears');
+
+promise_test(() => {
+  const ts = new TransformStream({}, {}, { highWaterMark: 0 });

empty object for the second arg is interesting. It might be worth pointing out this is basically testing that size() is never called (for either of them)

-- 
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/773#pullrequestreview-57970011