Bug 142682 – Add total test test of Streams API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xabier Rodríguez Calvar]

Add total test test of Streams API

[Xabier Rodríguez Calvar]

Created attachment 248616 [details]
Patch

These are all tests that we have with their expectations adapted to the code that is now on trunk. We think it is a good idea to have them already in the repo and update the expectations with the new patches.

[Xabier Rodríguez Calvar]

Created attachment 248617 [details]
Patch

Oops, I had uploaded a wrong changelog and I also added more info about how tests are organized.

[Build Bot]

Comment on attachment 248617 [details]
Patch

Attachment 248617 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5388872036909056

New failing tests:
streams/reference-implementation/readable-stream-cancel.html
streams/readablestream-pull.html
streams/reference-implementation/count-queuing-strategy.html
streams/readablestream-cancel.html
streams/readablestream-constructor.html
streams/readablestream-start.html
streams/reference-implementation/readable-stream.html

[Build Bot]

Created attachment 248620 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> Comment on attachment 248617 [details]
> Patch
> 
> Attachment 248617 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/5388872036909056
> 
> New failing tests:
> streams/reference-implementation/readable-stream-cancel.html
> streams/readablestream-pull.html
> streams/reference-implementation/count-queuing-strategy.html
> streams/readablestream-cancel.html
> streams/readablestream-constructor.html
> streams/readablestream-start.html
> streams/reference-implementation/readable-stream.html

One way of fixing this problems without flagging the tests would be commenting the tests and uncommenting them when they pass.

[Build Bot]

Comment on attachment 248617 [details]
Patch

Attachment 248617 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6213453144391680

New failing tests:
streams/readablestream-cancel.html
streams/readablestream-pull.html
streams/reference-implementation/count-queuing-strategy.html
streams/reference-implementation/readable-stream-cancel.html
streams/readablestream-constructor.html
streams/readablestream-start.html
streams/reference-implementation/readable-stream.html

[Build Bot]

Created attachment 248622 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Xabier Rodríguez Calvar]

Created attachment 248624 [details]
Patch

Avoid printing the stack so that tests don't fail because of different file paths.

[Benjamin Poulain]

Comment on attachment 248624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248624&action=review

I just had a quick look, I am sure we'll iterate and improve testing.

r+ if you do not remove tests.

> LayoutTests/streams/readablestream-constructor.html:-18
> -
> -    assert_true(Object.getOwnPropertyDescriptor(rs, 'closed').enumerable);
> -    assert_false(Object.getOwnPropertyDescriptor(rs, 'closed').configurable);

Why are you removing those tests? They look valuable.

> LayoutTests/streams/reference-implementation/resources/streams-utils.js:65
> +// http://stackoverflow.com/questions/1349404/generate-a-string-of-5-random-characters-in-javascript

Can you please just re-implement this?

Dealing with the licenses is too much trouble for something this trivial.

[Xabier Rodríguez Calvar]

Created attachment 248736 [details]
Patch

Fixed requested test and reimplemented randomChunk.

[Xabier Rodríguez Calvar]

Created attachment 249831 [details]
Patch

Added new spec tests

[youenn fablet]

Comment on attachment 249831 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249831&action=review

> LayoutTests/ChangeLog:46
> +        * streams/reference-implementation/resources/streams-utils.js: Added.

That is really great to have these tests!

> LayoutTests/resources/testharness.js:590
> +

I would try to not touch testharness.js as we should sync it regularly with https://github.com/w3c/testharness.js/blob/master/testharness.js

Can we move the changes to some other files and/or upstream the changes to https://github.com/w3c/testharness.js?

> LayoutTests/streams/readablestream-cancel.html:1
> +<!DOCTYPE html>

If these tests cover different parts than reference-implementation tests, can we upstream those to the reference-implementation repo?

> LayoutTests/streams/readablestream-wrong-arguments.html:15
> +});

No need to make this test async.
Ditto for tests below.

> LayoutTests/streams/reference-implementation/byte-length-queuing-strategy-expected.txt:3
> +

I do not think we should pass this test yet.

> LayoutTests/streams/reference-implementation/byte-length-queuing-strategy.html:4
> +<script src='resources/streams-utils.js'></script>

Maybe we should remove <script src='resources/streams-utils.js'></script> from this test.
Or move CountQueueingStrategy and ByteLengthQueuingStrategy definitions to specific files and include them sparingly?

> LayoutTests/streams/reference-implementation/count-queuing-strategy.html:4
> +<script src='resources/streams-utils.js'></script>

For this one, it may be good to keep including CountQueueingStrategy so that we can use the tests when upstreaming enqueue.
Maybe add a FIXME here to remove CountQueueingStrategy declaration when being implemented?

> LayoutTests/streams/reference-implementation/readable-stream-reader-expected.txt:3
> +PASS Constructing an ReadableStreamReader directly should fail if the stream is already locked (via direct construction) 

s/an ReadableStreamReader/a ReadableStreamReader/ ?

[Benjamin Poulain]

Comment on attachment 249831 [details]
Patch

Rubber-stamp by me. I did not read everything, I'll check the behaviors as I review the implementation patches.

Youenn already did a full review, please fix the patch before landing.

[Xabier Rodríguez Calvar]

(In reply to comment #12)
> > LayoutTests/resources/testharness.js:590
> > +
> 
> I would try to not touch testharness.js as we should sync it regularly with
> https://github.com/w3c/testharness.js/blob/master/testharness.js
> 
> Can we move the changes to some other files and/or upstream the changes to
> https://github.com/w3c/testharness.js?

Yes, I will try to create merge requests with this.

If it is not accepted, I could create a testharness-extended.js with the assertion and touching the report to suppress or fix the stack.

> > LayoutTests/streams/readablestream-cancel.html:1
> > +<!DOCTYPE html>
> 
> If these tests cover different parts than reference-implementation tests,
> can we upstream those to the reference-implementation repo?

Maybe not all of them, but most, yes.

> > LayoutTests/streams/readablestream-wrong-arguments.html:15
> > +});
> 
> No need to make this test async.
> Ditto for tests below.

Done.

> > LayoutTests/streams/reference-implementation/byte-length-queuing-strategy-expected.txt:3
> > +
> 
> I do not think we should pass this test yet.

It is just a dumb test to ensure that the strategy can be created. The rest of the tests in this file were related to WritableStream and other things that didn't make sense at this point, so the only one remaining was this.

Considering that this is not testing anything related to RS or RSR, I could remove it.

> > LayoutTests/streams/reference-implementation/byte-length-queuing-strategy.html:4
> > +<script src='resources/streams-utils.js'></script>
> 
> Maybe we should remove <script src='resources/streams-utils.js'></script>
> from this test.
> Or move CountQueueingStrategy and ByteLengthQueuingStrategy definitions to
> specific files and include them sparingly?
> 
> > LayoutTests/streams/reference-implementation/count-queuing-strategy.html:4
> > +<script src='resources/streams-utils.js'></script>
> 
> For this one, it may be good to keep including CountQueueingStrategy so that
> we can use the tests when upstreaming enqueue.
> Maybe add a FIXME here to remove CountQueueingStrategy declaration when
> being implemented?

I'll rework all this.

> > LayoutTests/streams/reference-implementation/readable-stream-reader-expected.txt:3
> > +PASS Constructing an ReadableStreamReader directly should fail if the stream is already locked (via direct construction) 
> 
> s/an ReadableStreamReader/a ReadableStreamReader/ ?

I will change and fix it upstream too

[Xabier Rodríguez Calvar]

Created attachment 250262 [details]
Patch

Fixed things from the comments and updated to latest spec tests.

[Xabier Rodríguez Calvar]

(In reply to comment #12)
> I would try to not touch testharness.js as we should sync it regularly with
> https://github.com/w3c/testharness.js/blob/master/testharness.js
> 
> Can we move the changes to some other files and/or upstream the changes to
> https://github.com/w3c/testharness.js?

Didn't touch testharness in the end, did it with the report.

> > LayoutTests/streams/readablestream-cancel.html:1
> > +<!DOCTYPE html>
> 
> If these tests cover different parts than reference-implementation tests,
> can we upstream those to the reference-implementation repo?

Next task

> > LayoutTests/streams/readablestream-wrong-arguments.html:15
> > +});
> 
> No need to make this test async.
> Ditto for tests below.

Done.

> > LayoutTests/streams/reference-implementation/byte-length-queuing-strategy-expected.txt:3
> > +
> 
> I do not think we should pass this test yet.

Removed.

> > LayoutTests/streams/reference-implementation/byte-length-queuing-strategy.html:4
> > +<script src='resources/streams-utils.js'></script>
> 
> Maybe we should remove <script src='resources/streams-utils.js'></script>
> from this test.
> Or move CountQueueingStrategy and ByteLengthQueuingStrategy definitions to
> specific files and include them sparingly?
> 
> > LayoutTests/streams/reference-implementation/count-queuing-strategy.html:4
> > +<script src='resources/streams-utils.js'></script>
> 
> For this one, it may be good to keep including CountQueueingStrategy so that
> we can use the tests when upstreaming enqueue.
> Maybe add a FIXME here to remove CountQueueingStrategy declaration when
> being implemented?

Moved to their own file.

> > LayoutTests/streams/reference-implementation/readable-stream-reader-expected.txt:3
> > +PASS Constructing an ReadableStreamReader directly should fail if the stream is already locked (via direct construction) 
> 
> s/an ReadableStreamReader/a ReadableStreamReader/ ?

Fixed here and upstream.

[Xabier Rodríguez Calvar]

Created attachment 250266 [details]
Patch

Made stack dumping optional in testharness. It is off by default but you can activate it by adding setting the attribute dumpStack to true for a specific test.

[youenn fablet]

(In reply to comment #17)
> Created attachment 250266 [details]
> Patch
> 
> Made stack dumping optional in testharness. It is off by default but you can
> activate it by adding setting the attribute dumpStack to true for a specific
> test.

The patch sounds good to me this way.
Let's ship it!

A couple of nits that we may want to further improve in the future:
- We should filter the stack trace and not remove it completely. Then we could do that by default as this would benefit to all testharness based run through the file system
- The tests do not yet use the controller. We will update them as part of ongoing streams API patches.

[Xabier Rodríguez Calvar]

Comment on attachment 250266 [details]
Patch

I translated some of our own tests to the official spec tests and it seems that we have some dups and some of them are wrong (I consulted with the spec editor).

[Benjamin Poulain]

Comment on attachment 250266 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250266&action=review

> LayoutTests/ChangeLog:27
> +        * streams/readablestream-constructor.html: Removed outdated tests
> +        and updated expectations.

What do you mean by outdated tests? They seem useful.

If this is already covered elsewhere, please mention where in the changelog.

[Xabier Rodríguez Calvar]

Created attachment 250366 [details]
Patch

All our custom tests are in the process of being merged into the spec so I moved them to the place they will be in as soon as https://github.com/whatwg/streams/pull/315 is merged. Therefore we do not have custom tests at the moment.

[Xabier Rodríguez Calvar]

(In reply to comment #20)
> > LayoutTests/ChangeLog:27
> > +        * streams/readablestream-constructor.html: Removed outdated tests
> > +        and updated expectations.
> 
> What do you mean by outdated tests? They seem useful.
> 
> If this is already covered elsewhere, please mention where in the changelog.

I refer to comment 21, but other than that, the only test that was removed was done for a reason that is explained in the changelog.

[Benjamin Poulain]

Comment on attachment 250366 [details]
Patch

I trust you on the coverage, rs=me.

[Xabier Rodríguez Calvar]

Created attachment 250446 [details]
Patch

Rebased against last trunk, updated the expectations and moved tests according to their new upstream place (pull request pending https://github.com/whatwg/streams/pull/315).

[Xabier Rodríguez Calvar]

Created attachment 250449 [details]
Patch for landing

[Xabier Rodríguez Calvar]

Comment on attachment 250449 [details]
Patch for landing

Let's try again tomorrow

[Xabier Rodríguez Calvar]

Comment on attachment 250449 [details]
Patch for landing

Let's try now.

[Xabier Rodríguez Calvar]

Comment on attachment 250449 [details]
Patch for landing

Hold horses for a minute

[Xabier Rodríguez Calvar]

Created attachment 250505 [details]
Patch for landing

[Xabier Rodríguez Calvar]

(In reply to comment #29)
> Created attachment 250505 [details]
> Patch for landing

After comment with Benjamin and Youenn I changed the RS construction test to ensure that we follow the spec and that a parameter nuance depending on the engine also behaves as expected by the JS spec.

[WebKit Commit Bot]

Comment on attachment 250505 [details]
Patch for landing

Clearing flags on attachment: 250505

Committed r182625: <http://trac.webkit.org/changeset/182625>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xabier Rodríguez Calvar]

Committed r182629: <http://trac.webkit.org/changeset/182629>