Add total test test of Streams API
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.
Created attachment 248617 [details] Patch Oops, I had uploaded a wrong changelog and I also added more info about how tests are organized.
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
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
(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.
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
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
Created attachment 248624 [details] Patch Avoid printing the stack so that tests don't fail because of different file paths.
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.
Created attachment 248736 [details] Patch Fixed requested test and reimplemented randomChunk.
Created attachment 249831 [details] Patch Added new spec tests
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/ ?
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.
(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
Created attachment 250262 [details] Patch Fixed things from the comments and updated to latest spec tests.
(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.
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.
(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.
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).
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.
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.
(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.
Comment on attachment 250366 [details] Patch I trust you on the coverage, rs=me.
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).
Created attachment 250449 [details] Patch for landing
Comment on attachment 250449 [details] Patch for landing Let's try again tomorrow
Comment on attachment 250449 [details] Patch for landing Let's try now.
Comment on attachment 250449 [details] Patch for landing Hold horses for a minute
Created attachment 250505 [details] Patch for landing
(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.
Comment on attachment 250505 [details] Patch for landing Clearing flags on attachment: 250505 Committed r182625: <http://trac.webkit.org/changeset/182625>
All reviewed patches have been landed. Closing bug.
Committed r182629: <http://trac.webkit.org/changeset/182629>