Port writable stream abort tests to wpt #540

marvinhagemeister · 2016-10-14T17:13:21Z

Just ported another test file to wpt. There is one thing where I'm unsure if this is the correct way for assertion. In some tests the constructor of the error object is compared. I couldn't get it to work without casting it to a string first.

For example:

const ws = new WritableStream();
const writer = ws.getWriter();
writer.write().then(
   () => { throw new Error('something failed'); },
   r => assert_equals(r.constructor.toString(), TypeError.toString(), 'err message')
);

domenic · 2016-10-14T17:26:44Z

The pattern we've been using for that is to put

const error1 = new Error('error1');
error1.name = 'error1';

at the top of the file (plus maybe error2 if you have multiple errors and want to distinguish), then later doing

return promise_rejects(t, error1, writer.write(), 'err message');

marvinhagemeister · 2016-10-14T18:05:38Z

Ah I see. I'll adapt my code to use promise_rejects 👍

domenic

In general very nice, and the porting work is super-appreciated. The main things are using promise_rejects and recordingWritableStream to clean up the tests.

domenic · 2016-10-14T22:07:29Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

 }, 'Aborting a WritableStream should cause the writer\'s fulfilled ready promise to reset to a rejected one');
+
+function promise_fulfills(expectedValue, promise, msg) {


This is not necessary. Inside your promise_test, just do

return promise.then(value => { assert_equals(value, expectedValue, msg); });

the test will fail if it returns a rejected promise.

domenic · 2016-10-14T22:09:17Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+promise_test(() => {
+  const ws = new WritableStream({
+    write() {
+      throw new Error('Unexpected write() call');


It's a decent bit of work, but I think these sorts of tests would be improved by using the recordingWritableStream, and then checking the recorded events in a final .then block. An example:

streams/reference-implementation/to-upstream-wpts/writable-streams/close.js

Lines 65 to 81 in ff1d3c9

promise_test(() => {

const ws = recordingWritableStream();

const writer = ws.getWriter();

return writer.ready.then(() => {

assert_equals(writer.desiredSize, 1, 'desiredSize should be 1');

writer.close();

assert_equals(writer.desiredSize, 1, 'desiredSize should be still 1');

return writer.ready.then(v => {

assert_equals(v, undefined, 'ready promise should be fulfilled with undefined');

assert_array_equals(ws.events, ['close'], 'write and abort should not be called');

});

});

}, 'when close is called on a WritableStream in writable state, ready should return a fulfilled promise');

Thanks for the tip. recordingWritableStream is much cleaner 👍

domenic · 2016-10-14T22:09:58Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+promise_test(() => {
+  let writeCount = 0;
+
+  const ws = new WritableStream({


Another good example of where recordingWritableStream would help you. You wouldn't need writeCount or any asserts, just check the events in a final .then block.

domenic · 2016-10-14T22:11:14Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+  ]);
+}, 'Aborting a WritableStream puts it in an errored state, with stored error equal to the abort reason');
+
+test(() => {


This will need to be a promise_test when it moves to use assert_promise_rejects

I'm unsure where to import assert_promise_rejects from. This function seems to be duplicated inside the web-platform-tests repo. I tried including one of the existing functions, but get an URL error by testharness

I meant promise_rejects, sorry.

domenic · 2016-10-14T22:11:33Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+
+  writer.close();
+
+  setTimeout(() => {


Use promise_test and delay()

marvinhagemeister · 2016-10-19T07:22:09Z

Sorry for the late response, I got ill for a few days. Thanks, for the review. I'll make these changes over the day 👍

domenic · 2016-10-19T20:04:53Z

It looks like you just accidentally created a duplicate of promise_rejects named assert_promise_rejects. I'm sorry for misleading you by saying the wrong name! But yeah, that can be deleted and we can just use promise_rejects.

marvinhagemeister · 2016-10-19T21:06:40Z

Thanks for clearing that up. Yeah, I got confused by the existing tests in inside the web-platform-tests folder. Adjusted the PR.

domenic · 2016-10-20T21:22:03Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

@@ -1,14 +1,16 @@
 'use strict';
+/* global delay, recordingWritableStream */


If you rebase this will not be necessary

domenic · 2016-10-20T21:22:24Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js


 if (self.importScripts) {
+  self.importScripts('../resources/test-utils.js');


Move this after testharness.js

domenic · 2016-10-20T21:23:37Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+      writer.write(2);
+    })
+    .then(() => {
+      assert_equals(ws.events.length, 2, 'the stream should have received 2 events');


You should use assert_array_equals here like the other recording stream tests. Same below.

The events[1] entry is the argument passed to abort, by the way, and is not related to writes.

domenic · 2016-10-20T21:24:19Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+
+  const writer = ws.getWriter();
+
+  return promise_rejects(t, errorInSinkAbort, writer.abort(undefined),


Can you use the error1 pattern used elsewhere? With a single error1 at the top of the file? Here and below.

Also everywhere that creates a separate passedReason can use that error1.

domenic · 2016-10-20T21:25:22Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+  const passedReason = new Error('Sorry, it just wasn\'t meant to be.');
+  writer.abort(passedReason);
+
+  assert_equals(recordedReason, passedReason);


This test can use recordingWritableStream instead, with assert_array_equals on the contents.

domenic · 2016-10-20T21:26:44Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+  ]);
+}, 'Aborting a WritableStream puts it in an errored state, with stored error equal to the abort reason');
+
+// TODO return promise


Just store the promise with const writePromise = promise_rejects(...) and then return it as the last line

domenic · 2016-10-20T21:26:58Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+
+  const passedReason = new Error('Sorry, it just wasn\'t meant to be.');
+  writer.abort(passedReason);
+}, 'Aborting a WritableStream causes any outstanding write() promises to be rejected with the abort reason');


Test description is again outdated; "with a TypeError"

domenic · 2016-10-20T21:27:22Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+  const passedReason = new Error('Sorry, it just wasn\'t meant to be.');
+  writer.abort(passedReason);
+
+  return promise_rejects(t, new TypeError('Aborted'), writer.closed, 'the stream should be errored with a TypeError');


Don't include 'Aborted' here; the test framework doesn't look at messages. (Yeah, it's kind of wonky.)

domenic · 2016-10-20T21:28:25Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+    .then(() => writer.abort())
+    .then(
+      v => assert_equals(v, undefined, 'abort promise should fulfill with undefined'),
+      () => { throw new Error(); }


This () => { throw new Error(); } is not necessary. In fact the second .then() can be removed entirely since we already test the undefinedness earlier.

domenic · 2016-10-20T21:29:36Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+
+  const writer = ws.getWriter();
+
+  writer.abort();


Again best to use recordingWritableStream. As-is, this test will pass even if close is never called.

It might be good to add something to recordingWritableStream's close() that asserts that arguments.length === 0.

domenic · 2016-10-25T19:24:19Z

@marvinhagemeister I'll take care of the rest of the issues I noted. Thanks so much for your help getting this file moved over :).

marvinhagemeister force-pushed the writable_abort_tests branch from 5a52ea9 to 2c1618e Compare October 14, 2016 17:14

marvinhagemeister changed the title ~~Prot writable stream abort tests to wpt~~ Port writable stream abort tests to wpt Oct 14, 2016

domenic requested changes Oct 14, 2016

View reviewed changes

marvinhagemeister force-pushed the writable_abort_tests branch from 48d730e to edaab4b Compare October 19, 2016 21:05

domenic requested changes Oct 20, 2016

View reviewed changes

marvinhagemeister and others added 3 commits October 25, 2016 15:11

Port writable stream abort tests to wpt

65423ff

use promise_rejects in tests

711b2fb

Take care of code review issues

995382f

domenic force-pushed the writable_abort_tests branch from edaab4b to 995382f Compare October 25, 2016 19:23

domenic mentioned this pull request Oct 25, 2016

Convert writable stream tests to web platform tests format #492

Closed

5 tasks

domenic merged commit 5dd1862 into whatwg:master Oct 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port writable stream abort tests to wpt #540

Port writable stream abort tests to wpt #540

marvinhagemeister commented Oct 14, 2016

domenic commented Oct 14, 2016

marvinhagemeister commented Oct 14, 2016

domenic left a comment

domenic Oct 14, 2016

marvinhagemeister Oct 19, 2016

domenic Oct 14, 2016

marvinhagemeister Oct 19, 2016

domenic Oct 14, 2016

domenic Oct 14, 2016

marvinhagemeister Oct 19, 2016

domenic Oct 19, 2016

domenic Oct 14, 2016

marvinhagemeister commented Oct 19, 2016

domenic commented Oct 19, 2016

marvinhagemeister commented Oct 19, 2016

domenic Oct 20, 2016

domenic Oct 20, 2016

domenic Oct 20, 2016

domenic Oct 20, 2016

domenic Oct 20, 2016

domenic Oct 20, 2016

domenic Oct 20, 2016

domenic Oct 20, 2016

domenic Oct 20, 2016

domenic Oct 20, 2016

domenic commented Oct 25, 2016

		}, 'Aborting a WritableStream should cause the writer\'s fulfilled ready promise to reset to a rejected one');

		function promise_fulfills(expectedValue, promise, msg) {

	promise_test(() => {
	const ws = recordingWritableStream();

	const writer = ws.getWriter();

	return writer.ready.then(() => {
	assert_equals(writer.desiredSize, 1, 'desiredSize should be 1');

	writer.close();
	assert_equals(writer.desiredSize, 1, 'desiredSize should be still 1');

	return writer.ready.then(v => {
	assert_equals(v, undefined, 'ready promise should be fulfilled with undefined');
	assert_array_equals(ws.events, ['close'], 'write and abort should not be called');
	});
	});
	}, 'when close is called on a WritableStream in writable state, ready should return a fulfilled promise');

		@@ -1,14 +1,16 @@
		'use strict';
		/* global delay, recordingWritableStream */


		if (self.importScripts) {
		self.importScripts('../resources/test-utils.js');


		const writer = ws.getWriter();

		return promise_rejects(t, errorInSinkAbort, writer.abort(undefined),

Port writable stream abort tests to wpt #540

Port writable stream abort tests to wpt #540

Conversation

marvinhagemeister commented Oct 14, 2016

domenic commented Oct 14, 2016

marvinhagemeister commented Oct 14, 2016

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marvinhagemeister commented Oct 19, 2016

domenic commented Oct 19, 2016

marvinhagemeister commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Oct 25, 2016