Re: [whatwg/streams] Eliminate usages of async_test (#534)

domenic commented on this pull request.



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

Hah, I forgot you could do that. Maybe a separate line though for consistency with the rest of the imports?

> @@ -15,17 +15,11 @@ async_test(t => {
       });
     },
     write(chunk) {
-      t.step(() => {
-        assert_true(expectWriteCall, 'write should not be called until start promise resolves');
-        assert_equals(chunk, 'a', 'chunk should be the value passed to write');
-        t.done();
-      });
+      assert_true(expectWriteCall, 'write should not be called until start promise resolves');
+      assert_equals(chunk, 'a', 'chunk should be the value passed to write');

I think this should be a promise_test, but you can use the recordingWritableStream to test that it gets passed the right arguments. Example at https://github.com/whatwg/streams/blob/ba1e784ec2761fa7e992c80e1c3ef0f10b991ce1/reference-implementation/to-upstream-wpts/writable-streams/bad-underlying-sinks.js#L74-L76

That will also take care of the asserting that close is not called.

In general I think many of the tests in this file that do asserts that certain underlying sink methods are called would benefit from this.

>  
-    writer.ready.then(t.step_func(() => {
-      const writePromise = writer.write('a');
-      let writePromiseResolved = false;
-      assert_not_equals(resolveSinkWritePromise, undefined, 'resolveSinkWritePromise should not be undefined');
+  writer.ready.then(() => {

Missing `return`, I think?

-- 
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/534#pullrequestreview-3951593