Replace streams tests with automatically-imported ones

Replace streams tests with automatically-imported ones

This replaces the manually-imported and maintained testharness.js tests for streams, with the automatically-imported web platform tests. This allows us to benefit from a couple upstream fixes, including the one contributing to bug 662172.

The failing expectations here are related to the fact that the upstream tests test for the presence of the pipeTo and pipeThrough methods, which we do not support yet.

BUG=662172

[domenic, 2016-11-14 21:41:03]

Description was changed from

==========
Replace streams tests with automatically-imported ones

BUG=662172
==========

to

==========
Replace streams tests with automatically-imported ones

This replaces the manually-imported and maintained testharness.js tests for
streams, with the automatically-imported web platform tests. This allows us to
benefit from a couple upstream fixes, including the one contributing to bug
662172.

The failing expectations here are related to the fact that the upstream tests
test for the presence of the pipeTo and pipeThrough methods, which we do not
support yet.

BUG=662172
==========

[domenic, 2016-11-14 21:41:03]

domenic@chromium.org changed reviewers:
+ ricea@chromium.org, tyoshino@chromium.org

[Adam Rice, 2016-11-15 06:55:42]

lgtm

https://codereview.chromium.org/2500133002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/imported/wpt/streams/readable-streams/general.https-expected.txt
(right):

https://codereview.chromium.org/2500133002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/wpt/streams/readable-streams/general.https-expected.txt:11:
FAIL ReadableStream start should be called with the proper parameters
assert_equals: constructor should have 4 parameter expected 4 but got 5
What's a good way to fix this?

https://codereview.chromium.org/2500133002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/imported/wpt/streams/readable-streams/pipe-through.https-expected.txt
(right):

https://codereview.chromium.org/2500133002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/wpt/streams/readable-streams/pipe-through.https-expected.txt:3:
PASS ReadableStream.prototype.pipeThrough should throw when its first argument
is not convertible to an object
This should not pass. We should modify methodThrows() to assert that the method
is not undefined. Is it possible to do this directly upstream?

Edit: Another point of view is that since another test already verifies that
pipeThrough exists, it would be over-testing to also assert it here.

[domenic, 2016-11-15 16:34:01]

On 2016/11/15 at 06:55:42, ricea wrote:
> lgtm
> 
>
https://codereview.chromium.org/2500133002/diff/1/third_party/WebKit/LayoutTe...
> File
third_party/WebKit/LayoutTests/imported/wpt/streams/readable-streams/general.https-expected.txt
(right):
> 
>
https://codereview.chromium.org/2500133002/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/imported/wpt/streams/readable-streams/general.https-expected.txt:11:
FAIL ReadableStream start should be called with the proper parameters
assert_equals: constructor should have 4 parameter expected 4 but got 5
> What's a good way to fix this?

We can change from

    constructor(stream, underlyingSource, size, highWaterMark,
isExternallyControlled) {

to

    constructor(stream, underlyingSource, size, highWaterMark,
isExternallyControlled = false) {

> 
>
https://codereview.chromium.org/2500133002/diff/1/third_party/WebKit/LayoutTe...
> File
third_party/WebKit/LayoutTests/imported/wpt/streams/readable-streams/pipe-through.https-expected.txt
(right):
> 
>
https://codereview.chromium.org/2500133002/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/imported/wpt/streams/readable-streams/pipe-through.https-expected.txt:3:
PASS ReadableStream.prototype.pipeThrough should throw when its first argument
is not convertible to an object
> This should not pass. We should modify methodThrows() to assert that the
method is not undefined. Is it possible to do this directly upstream?
> 
> Edit: Another point of view is that since another test already verifies that
pipeThrough exists, it would be over-testing to also assert it here.

I'd be OK either way, as long as at least one test fails when pipeThrough is not
present. We can easily fix it upstream via pull requests to
https://github.com/w3c/web-platform-tests/blob/master/streams/resources/test-...

[tyoshino (SeeGerritForStatus), 2016-11-16 04:17:12]

lgtm

[Adam Rice, 2016-11-16 05:01:03]

On 2016/11/15 16:34:01, domenic wrote:
> We can change from
> 
>     constructor(stream, underlyingSource, size, highWaterMark,
> isExternallyControlled) {
> 
> to
> 
>     constructor(stream, underlyingSource, size, highWaterMark,
> isExternallyControlled = false) {

Thanks. I will wait for this CL to land and then do it.

> I'd be OK either way, as long as at least one test fails when pipeThrough is
not
> present. We can easily fix it upstream via pull requests to
>
https://github.com/w3c/web-platform-tests/blob/master/streams/resources/test-...

Maybe the right way to think about it is that the behaviour of methodThrows() is
unexpected and should be fixed?

[domenic, 2016-11-16 19:27:45]

The CQ bit was checked by domenic@chromium.org

[commit-bot: I haz the power, 2016-11-16 19:28:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 20:46:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 20:46:04]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[domenic, 2016-11-16 21:23:38]

On 2016/11/16 at 20:46:04, commit-bot wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

I'm not sure what to make of these failures. A lot of them are streams-related
but not related to the ones in this patch. Once of them is not streams-related
at all (fast/overflow/overflow-height-float-not-removed-crash3.html). The only
one in this patch that failed is
imported/wpt/streams/readable-streams/templated.https.html, for which the logs
show the uninformative:

12:42:12.965 2529 "/usr/bin/wdiff --start-delete=##WDIFF_DEL##
--end-delete=##WDIFF_END## --start-insert=##WDIFF_ADD##
--end-insert=##WDIFF_END##
/b/rr/tmp_JFN_d/w/layout-test-results/retry_2/imported/wpt/streams/readable-streams/templated.https-expected.txt
/b/rr/tmp_JFN_d/w/layout-test-results/retry_2/imported/wpt/streams/readable-streams/templated.https-actual.txt"
took 0.02s
12:42:12.989 2529 "ruby --version" took 0.02s
12:42:13.054 2529 "ruby -I
/b/c/b/linux/src/third_party/WebKit/Tools/Scripts/webkitruby/PrettyPatch
/b/c/b/linux/src/third_party/WebKit/Tools/Scripts/webkitruby/PrettyPatch/prettify.rb
/b/rr/tmp_JFN_d/w/layout-test-results/retry_2/imported/wpt/streams/readable-streams/templated.https-diff.txt"
took 0.06s
12:42:13.056 22988 [13/28]
imported/wpt/streams/readable-streams/templated.https.html failed unexpectedly
(text diff)
12:42:13.055 2529 worker/3
imported/wpt/streams/readable-streams/templated.https.html failed:
12:42:13.055 2529 worker/3  text diff

In fact for none of the failing tests do I see anything useful. Just things
like:

12:42:10.194 2524 worker/0
http/tests/fetch/serviceworker/body-mixin-base-https-other-https.html failed:
12:42:10.194 2524 worker/0  asserts failed


I also am confused why they would pass on most of the bots but fail on the
non-x64 release ones.

Not sure how to debug this. Any help would be appreciated.

[Adam Rice, 2016-11-17 03:20:51]

On 2016/11/16 21:23:38, domenic wrote:
> I also am confused why they would pass on most of the bots but fail on the
> non-x64 release ones.

I think it's just that the webkit tests only run on a few bots.

> Not sure how to debug this. Any help would be appreciated.

There's a top trick for this. Completely ignore the useless red 
"webkit_tests (with patch)" entry and instead look at the green
"archive_webkit_tests_results" immediately below. That has a link
"layout_test_results" which does exactly what you would hope.

In summary: you should probably put back
LayoutTests/http/tests/streams/resources/rs-utils.js
while we fix the fetch tests not to depend on it.

Line number changes in ReadableStream.js have broken the expectations for
readable-streams/templated.https.html. Resetting and re-uploading
the -expectated.txt file should fix it.

[domenic, 2016-11-18 19:53:11]

On 2016/11/17 at 03:20:51, ricea wrote:
> On 2016/11/16 21:23:38, domenic wrote:
> > I also am confused why they would pass on most of the bots but fail on the
> > non-x64 release ones.
> 
> I think it's just that the webkit tests only run on a few bots.
> 
> > Not sure how to debug this. Any help would be appreciated.
> 
> There's a top trick for this. Completely ignore the useless red 
> "webkit_tests (with patch)" entry and instead look at the green
> "archive_webkit_tests_results" immediately below. That has a link
> "layout_test_results" which does exactly what you would hope.
> 
> In summary: you should probably put back
> LayoutTests/http/tests/streams/resources/rs-utils.js
> while we fix the fetch tests not to depend on it.
> 
> Line number changes in ReadableStream.js have broken the expectations for
> readable-streams/templated.https.html. Resetting and re-uploading
> the -expectated.txt file should fix it.

Thanks so much for your help. Unfortunately it seems like templated.https.html
is still failing despite the new -expected.txt I uploaded, and CQ runs are no
longer producing archive_webkit_tests_results due to

> Uncaught Exception: ModuleInjectionError("RecipeApi has no dependency 'build'.
(Add it to DEPS?)",)

Maybe the easiest thing to do here is to wait until
https://bugs.chromium.org/p/chromium/issues/detail?id=654701 is fixed :-/

[Adam Rice, 2016-11-30 10:07:21]

The CQ bit was checked by ricea@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 10:07:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 10:10:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 10:10:33]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[domenic, 2016-12-21 22:13:13]

On 2016/11/18 at 19:53:11, domenic wrote:
> On 2016/11/17 at 03:20:51, ricea wrote:
> > On 2016/11/16 21:23:38, domenic wrote:
> > > I also am confused why they would pass on most of the bots but fail on the
> > > non-x64 release ones.
> > 
> > I think it's just that the webkit tests only run on a few bots.
> > 
> > > Not sure how to debug this. Any help would be appreciated.
> > 
> > There's a top trick for this. Completely ignore the useless red 
> > "webkit_tests (with patch)" entry and instead look at the green
> > "archive_webkit_tests_results" immediately below. That has a link
> > "layout_test_results" which does exactly what you would hope.
> > 
> > In summary: you should probably put back
> > LayoutTests/http/tests/streams/resources/rs-utils.js
> > while we fix the fetch tests not to depend on it.
> > 
> > Line number changes in ReadableStream.js have broken the expectations for
> > readable-streams/templated.https.html. Resetting and re-uploading
> > the -expectated.txt file should fix it.
> 
> Thanks so much for your help. Unfortunately it seems like templated.https.html
is still failing despite the new -expected.txt I uploaded, and CQ runs are no
longer producing archive_webkit_tests_results due to
> 
> > Uncaught Exception: ModuleInjectionError("RecipeApi has no dependency
'build'. (Add it to DEPS?)",)
> 
> Maybe the easiest thing to do here is to wait until
https://bugs.chromium.org/p/chromium/issues/detail?id=654701 is fixed :-/

So the unhandled rejections bug is now fixed, so we can have stable
-expected.txt files, which I uploaded. However wptserve is on the fritz
apparently, so this is currently blocked on
https://bugs.chromium.org/p/chromium/issues/detail?id=669802. The tests all pass
locally with --enable-wptserve.

[qyearsley, 2017-01-18 00:35:47]

On 2016/12/21 at 22:13:13, domenic wrote:
> On 2016/11/18 at 19:53:11, domenic wrote:
> > On 2016/11/17 at 03:20:51, ricea wrote:
> > > On 2016/11/16 21:23:38, domenic wrote:
> > > > I also am confused why they would pass on most of the bots but fail on
the
> > > > non-x64 release ones.
> > > 
> > > I think it's just that the webkit tests only run on a few bots.
> > > 
> > > > Not sure how to debug this. Any help would be appreciated.
> > > 
> > > There's a top trick for this. Completely ignore the useless red 
> > > "webkit_tests (with patch)" entry and instead look at the green
> > > "archive_webkit_tests_results" immediately below. That has a link
> > > "layout_test_results" which does exactly what you would hope.
> > > 
> > > In summary: you should probably put back
> > > LayoutTests/http/tests/streams/resources/rs-utils.js
> > > while we fix the fetch tests not to depend on it.
> > > 
> > > Line number changes in ReadableStream.js have broken the expectations for
> > > readable-streams/templated.https.html. Resetting and re-uploading
> > > the -expectated.txt file should fix it.
> > 
> > Thanks so much for your help. Unfortunately it seems like
templated.https.html is still failing despite the new -expected.txt I uploaded,
and CQ runs are no longer producing archive_webkit_tests_results due to
> > 
> > > Uncaught Exception: ModuleInjectionError("RecipeApi has no dependency
'build'. (Add it to DEPS?)",)
> > 
> > Maybe the easiest thing to do here is to wait until
https://bugs.chromium.org/p/chromium/issues/detail?id=654701 is fixed :-/
> 
> So the unhandled rejections bug is now fixed, so we can have stable
-expected.txt files, which I uploaded. However wptserve is on the fritz
apparently, so this is currently blocked on
https://bugs.chromium.org/p/chromium/issues/detail?id=669802. The tests all pass
locally with --enable-wptserve.

Update: wptserve is now enabled by default again (crbug.com/618366), and the
imported directory has been renamed as "external" (crbug.com/678788).

[domenic, 2017-04-17 22:24:48]

This was superceded by ricea@'s work in
https://chromium.googlesource.com/chromium/src/+/ba36ef765de08362765f726520fb...
, although we still have one leftover file I've asked ricea@ about separately.