Initial Merge of WritableStreams Tests w/Upstream W3C

[Brandr0id]

Initial merge of writablestream tests into upstream W3C version.
Contains some formatting and minor functionality fixes (timing issues in
a small subset of tests).

[sideshowbarker]

w3c-test:mirror

[Brandr0id]

@annevk , @domenic could you please take a look @ the test updates? There shouldn't be too much functionally different but I'd like to make sure the style updates work well for this repro as well so we can better keep these tests in-sync as they are updated.

[domenic]

Please don't change the formatting of the HTML wrapper files. They are auto-generated, and the changes introduced are unnecessary.

Happy to re-review once those are reverted.

[domenic]

Actually, please do not change any formatting at all (such as whitespace, which I see has changed dramatically in the .js file). That just makes Git history useless and doesn't help anything.

[Brandr0id]

Happy to revert the wrapper files for this change, would you be receptive to a separate PR that updates generate-test-wrappers.js and re-generates the wrapper files?

For the other formatting changes this was a direct merge of our forked version that contains updated formatting (consistent spacing/line breaks etc). Since the functional updates are fairly small I should be able to pull them out pretty easy (It'll make the PR a lot smaller 👍 ). While I don't want to pollute the commit history with formatting changes I would like to get a feel for what, if any, updates you'd be receptive to (e.g. 2sp => 4 sp, function calls either consistently on one line, or all params on a new line, etc).

[domenic]

I'd rather not change the wrapper files; they are nice and minimal right now, and I don't see the motivation.

Similarly, I don't understand the motivation for any of the formatting changes. Are you updating other web platform test directories with 4 spaces as well? Without a better understanding of what value they bring, I can't really say if any of them would make sense.

[annevk]

Yeah, I think two spaces in JavaScript (and one or two spaces in markup) is what we somewhat consistently use. It seems fine for new resources to deviate from that, but I'd rather not reformat just for the sake of it. If we do that we should start with some kind of style guide (though that has its own problems, you might be testing the indentation doesn't change meaning or some such).

[Brandr0id]

Alright thanks for the feedback, I've pushed a new version w/o the majority of the formatting diffs (whitespace/wrappers).

[tyoshino]

We had 120 col limit on the size of each line when we were developing the tests in the Streams Standard repo. That's another point @domenic may want to say something though as we've moved to wpt, it might be acceptable.

[domenic]

I made it through one file, but there are still too many formatting changes for me to usefully review this.

It may be best to start from scratch, instead of trying to upstream from your copy.

[domenic]

Can you undo all these formatting changes?

[domenic]

What is the motivation for this change?

[Brandr0id]

It looks like this test has been working fine for Chrome's impl due to the requirement of async events needing to be manually processed during the test (e.g. calls to flushAsyncEvents()). Since flushAsyncEvents() isn't called in this test, the stream is constructed but never actually finishes calling start so start is still pending due to this requirement. However it doesn't look like there's a spec'd reason why start couldn't/shouldn't start immediately when constructed.

I found in our implementation that this was happening and so the test wasn't behaving as intended. By ensuring that start doesn't resolve until the test intends (regardless of async event processing) this test now passes in both impls.

[ricea]

I'm leaving the main review to @domenic. I'm just adding a couple of drive-by comments to reduce latency before he can get back.

This is a little bit subtle, but the step "Upon fulfillment of startPromise," makes the following steps be executed asynchronously, even if start() itself returned synchronously.

Aside: almost all WritableStream tests were written against the reference implementation, not Chrome's implementation. There's a few that I initially wrote against Chrome's implementation, but I always verified against the reference implementation before upstreaming.

[domenic]

Can you undo all these formatting changes?

[domenic]

Can you leave this blank line here?

[Brandr0id]

sure, should be back in the latest version :)

[domenic]

Can you undo this change? It makes the test less clear. Same below.

[domenic]

Can you undo this formatting change?

[domenic]

Can you undo this formatting change?

[domenic]

Can you undo this formatting change?

[domenic]

Can you undo this formatting change?

[domenic]

It looks like this is a different test. The old one was correct; can you add a new one that tests the new code path you're working through, instead of modifying the existing one?

[Brandr0id]

same issue as the other tests, start is just returning a promise to complete in a deterministic manner.

[domenic]

(This still has too many formatting changes for me to review usefully, by the way. I won't bother commenting on each individual one again.)

[domenic]

It seems so far in fact all these changes are invalid:

• They are working around bugs in Edge's event loop implementation where it does not resolve promises in the same order they were enqueued (i.e. Edge apparently processes promises nondeterministically).
• One stems from misunderstanding write() vs. write(undefined) in JavaScript.
• The rest are formatting changes.

At least, in what I've reviewed so far. As such, I'm inclined to close this without merging... What do you think, @Brandr0id?

[ricea]

Supplemental note: The strictness about Promise resolution order in the tests is intentional, since it is an observable side-effect of the order of steps in the standard.

Ideally I would like all user-visible behaviour to be the same in every implementation, so that developers never get any nasty surprises. I don't want to accept changes that make the tests less strict. But I don't think failing some of the strictest expectations should block shipping.

[Brandr0id]

Thanks @ricea and @domenic , I could squash the two commits to make it easier to see the lack of formatting but I think it probably isn't needed at this point. I tested some changes locally that ensure we always resolve a promise for the UnderlyingSink methods, along with updates due to a spec change (adding the erroring state) and I think the rest of the aborting test changes likely aren't needed now.

Could you please advise on two further items though:

1. It looks like all the tests are validating that the closed promise is a new TypeError() when rejected, but it looks like potentially it should be the storedError?

2. In the constructor set of tests the typeof 'WritableStreamDefaultWriter' and 'WritableStreamDefaultController' are tested to be undefined. Even though the constructors are not exposed, when defined in WebIdl, these are exposed as functions. Are there any issues with updating this to validate the function type?

[domenic]

I could squash the two commits to make it easier to see the lack of formatting but I think it probably isn't needed at this point

I'm actaully reviewing via https://github.com/w3c/web-platform-tests/pull/6499/files, so I can see them. This does seem very much cleaned up now, so thanks for that. It looks like there are still some good things to test here, e.g. removing the CountQueuingStrategy dependency in start.js, and adding the this tests in general.js. So I take back my desire to close this without merging.

But yeah, let's get things straightened out first. Great to hear that always-resolving fixed most issues; I look forward to a push without those changes.

It looks like all the tests are validating that the closed promise is a new TypeError() when rejected, but it looks like potentially it should be the storedError?

This did change somewhat recently, and is still kind of confusing, so I'm not sure I can give the right answer off the top of my head. (Maybe @ricea can.) Can you give a small example we can walk through together?

In the constructor set of tests the typeof 'WritableStreamDefaultWriter' and 'WritableStreamDefaultController' are tested to be undefined. Even though the constructors are not exposed, when defined in WebIdl, these are exposed as functions. Are there any issues with updating this to validate the function type?

Per https://streams.spec.whatwg.org/#globals, these are not exposed. This is the second bit of feedback we've gotten about this section of the spec being too hidden though. You may enjoy:

• Should ReadableStreamDefaultReader be exposed? whatwg/streams#586
• Exact normative requirements for interface definitions are unclear whatwg/streams#732
• JSIDL vs WebIDL whatwg/streams#45

as you can see this issue of Web IDL vs. ES-style specification has been a bit problematic for us. It's good to get feedback that Edge is implementing with Web IDL; that helps shape the decision there.

For now though, we should probably not expose those classes, and keep testing that we don't expose them.

[Brandr0id]

One of the existing test cases is a good example for the closed promise:

promise_test(t => {
  const ws = new WritableStream({
    write() {
      return Promise.reject(error1);
    }
  });
  const writer = ws.getWriter();
  return writer.ready.then(() => {
    const writePromise = writer.write('a');
    const abortPromise = writer.abort(error2);
    let closedRejected = false;
    return Promise.all([
      promise_rejects(t, error1, writePromise, 'write() should reject')
          .then(() => assert_false(closedRejected, '.closed should not resolve before write()')),
      promise_rejects(t, new TypeError(), writer.closed, '.closed should reject')
          .then(() => {
            closedRejected = true;
          }),
      abortPromise
    ]);
  });
}, '.closed should not resolve before rejected write(); write() error should not overwrite abort() error');

It seems like when closeandclosed promises are rejected with the storedError, it should be populated by the rejection of the write callback and be rejected with error1 not a new TypeError. FWIW I do sort of prefer it rejecting with a new TypeError in this case, but would like to understand why it's not using error1.

[domenic]

So in that test case, the relevant happenings go something like this:

• writer.write('a')
  • WritableStreamDefaultWriterWrite
    • WritableStreamDefaultControllerWrite
      • WritableStreamDefaultControllerAdvanceQueueIfNeeded
        • WritableStreamDefaultControllerProcessWrite
          • Let sinkWritePromise = PromiseInvokeOrNoop(underlyingSink, "write", « 'a', controller »)
          • Further processing happens in response to sinkWritePromise
• writer.abort(error2)
  • WritableStreamDefaultWriterAbort
    • WritableStreamAbort
      • State is "writable"; the above writer.write() did not get to the point where it changes the state.
      • WritableStreamStartErroring(stream, new TypeError)
        • Sets [[storedError]] to the TypeError

In other words, even though the write() call "happened first", the rejection with error1 isn't seen until after the abort() call already transitioned the state to errored with the aborting TypeError.

Make sense?

[ricea]

asserts don't work properly in sink methods because the exceptions they throw are caught by the WritableStream. This is a serious trap: we used to have lots of tests that looked fine but weren't actually testing anything.

I mostly fixed these by moving the asserts out of the methods, but another way that works is to replace start() { assert_something(); } with start: t.step_func(() => { assert_something(); }).

[Brandr0id]

Thanks @domenic that makes sense. I've pushed a change with just the 2 test cases modified (removing the strategy dependency in start.js and updating the asserts in general.js). While testing this I didn't notice the asserts being caught by the UnderlyingSink methods as @ricea was concerned about; however, after updating our implementation to always resolve a promise regardless of potentially having an immediate answer this test seems fine with or without the change.

[ghost]

View the complete job log.

Firefox (nightly)

[ghost]

View the complete job log.

Sauce (safari)

[ghost]

View the complete job log.

Chrome (unstable)

[ghost]

View the complete job log.

Sauce (MicrosoftEdge)

[domenic]

Sorry for the continued delay on this @Brandr0id! I finally got around to it and suggest the following diff:

diff --git a/streams/writable-streams/general.js b/streams/writable-streams/general.js
index 0f4dae9f1a..74929ea745 100644
--- a/streams/writable-streams/general.js
+++ b/streams/writable-streams/general.js
@@ -111,26 +111,34 @@ promise_test(() => {
   );
 }, 'closed and ready on a released writer');
 
-promise_test(() => {
-  var thisObject = null;
+promise_test(t => {
+  let thisObject = null;
   // Calls to Sink methods after the first are implicitly ignored. Only the first value that is passed to the resolver
   // is used.
   class Sink {
     start() {
       // Called twice
-      assert_equals(thisObject, this, 'start should be called as a method');
+      t.step(() => {
+        assert_equals(this, thisObject, 'start should be called as a method');
+      });
     }
 
     write() {
-      assert_equals(thisObject, this, 'write should be called as a method');
+      t.step(() => {
+        assert_equals(this, thisObject, 'write should be called as a method');
+      });
     }
 
     close() {
-      assert_equals(thisObject, this, 'close should be called as a method');
+      t.step(() => {
+        assert_equals(this, thisObject, 'close should be called as a method');
+      });
     }
 
     abort() {
-      assert_equals(thisObject, this, 'abort should be called as a method');
+      t.step(() => {
+        assert_equals(this, thisObject, 'abort should be called as a method');
+      });
     }
   }
 
@@ -141,11 +149,11 @@ promise_test(() => {
   const writer = ws.getWriter();
 
   writer.write('a');
-  var closePromise = writer.close();
+  const closePromise = writer.close();
 
   const ws2 = new WritableStream(theSink);
   const writer2 = ws2.getWriter();
-  var abortPromise = writer2.abort();
+  const abortPromise = writer2.abort();
 
   return Promise.all([
     closePromise,

Then we can merge. Alternately you can click "allow edits from maintainers" and I can merge this myself.

@ricea, if you have time an extra review (with my above patch) would be appreciated.

[ricea]

With @domenic's change, lgtm.

[ricea]

@domenic I merged your patch and tested against the reference implementation. lgtm.