Bug 145839 – [Streams API] Sync tests with spec

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

We need to do this again

+++ This bug was initially created as a clone of Bug #143669 +++

Sync Streams API tests with spec

[Xabier Rodríguez Calvar]

Created attachment 254719 [details]
Patch

Rebased the tests against the latest spec tests and utils. Updated expectations.

[youenn fablet]

Comment on attachment 254719 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Considered all changes in the spec tests and brought them to WebKit.

Great to see all those new tests!
Eager to make them PASS ;)

> LayoutTests/ChangeLog:13
> +        important as it allowed us to remove all our custom tests that are now covered by the spec.

It is good that our tests be integrated as reference tests.
I guess a further patch will remove the custom redundant tests?

> LayoutTests/resources/gc.js:-15
> -            gcRec(n-1);

Can you detail why this is no longer needed?
I guess the purpose of gcRec is to be able to run the tests in not-rwt environments.

> LayoutTests/resources/gc.js:10
> +        else {

Maybe remove the "{" and corresponding "}" ?

> LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:36
>  PASS getReader() should return a reader that acts errored 

This passing test has the same name as one that is timeouting (line 43).
If that is the same templated test that both PASS and TIMEOUT, we will probably end up in a flaky PASS test.
Can we check that before landing?
We may need to comment this test temporarily until we upstream more features (async start/pull e.g.).

> LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:43
> +TIMEOUT getReader() should return a reader that acts errored Test timed out

Timeouting test referred above.

> LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:56
> +FAIL draining the stream via read() should cause the reader closed promise to fulfill and locked to be false assert_false: stream should no longer be locked expected false got undefined

It seems that we were passing this test.
Is it the case of some missing ReadableStream feature or is it a regression?
In the latter case, maybe we can file a bug to keep track of it?

[Xabier Rodríguez Calvar]

(In reply to comment #2)
> Great to see all those new tests!
> Eager to make them PASS ;)

We have a lot of work :)

> It is good that our tests be integrated as reference tests.
> I guess a further patch will remove the custom redundant tests?

I forgot, let me upload them again O:)

> > LayoutTests/resources/gc.js:-15
> > -            gcRec(n-1);
> 
> Can you detail why this is no longer needed?
> I guess the purpose of gcRec is to be able to run the tests in not-rwt
> environments.

Domenic made that change when adapting the tests before merging them at the spec. I guess his (and my) opinion is that that method is not reliable to force the gc to work so he (and I) preferred to add a warning in those cases and have that test dumping that warning.

> > LayoutTests/resources/gc.js:10
> > +        else {
> 
> Maybe remove the "{" and corresponding "}" ?

Ok.

> > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:36
> >  PASS getReader() should return a reader that acts errored 
> 
> This passing test has the same name as one that is timeouting (line 43).
> If that is the same templated test that both PASS and TIMEOUT, we will
> probably end up in a flaky PASS test.

I run tests 10 times with --force and they behave like that all the time. The case of this discrepancy can be that these are templated tests and that means that some tests are run more that once with different conditions or starting points. I don't think they are flaky.

> Can we check that before landing?

I'll do it now.

> We may need to comment this test temporarily until we upstream more features
> (async start/pull e.g.).

I uncommented all tests because I think they are marked already as flaky at the expectations until we find a better solution for the timeouts and the integration between WTR and testharness.

> > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:56
> > +FAIL draining the stream via read() should cause the reader closed promise to fulfill and locked to be false assert_false: stream should no longer be locked expected false got undefined
> 
> It seems that we were passing this test.
> Is it the case of some missing ReadableStream feature or is it a regression?
> In the latter case, maybe we can file a bug to keep track of it?

The cause for that test is the "locked" attribute, so it is a missing RS feature. If you want I can file a bug, but the test is failing anyway and if that happens, it is already under my radar :)

[youenn fablet]

> > It is good that our tests be integrated as reference tests.
> > I guess a further patch will remove the custom redundant tests?
> 
> I forgot, let me upload them again O:)

As you like, doing the removal as another patch seems good too.

> > > LayoutTests/resources/gc.js:-15
> > > -            gcRec(n-1);
> > 
> > Can you detail why this is no longer needed?
> > I guess the purpose of gcRec is to be able to run the tests in not-rwt
> > environments.
> 
> Domenic made that change when adapting the tests before merging them at the
> spec. I guess his (and my) opinion is that that method is not reliable to
> force the gc to work so he (and I) preferred to add a warning in those cases
> and have that test dumping that warning.

Having the warning in not-rwt environments seems like a good idea.
Keeping the particular gcRec method does no harm though, it just increases the chance that a gc will actually happen.

> > > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:36
> > >  PASS getReader() should return a reader that acts errored 
> > 
> > This passing test has the same name as one that is timeouting (line 43).
> > If that is the same templated test that both PASS and TIMEOUT, we will
> > probably end up in a flaky PASS test.
> 
> I run tests 10 times with --force and they behave like that all the time.
> The case of this discrepancy can be that these are templated tests and that
> means that some tests are run more that once with different conditions or
> starting points. I don't think they are flaky.

The issue is on slow bots: the test that PASS has only 50 ms to finish, which is not always ok on slow bots.
Increasing the timeout value for that specific test may fix that. 1s might be good enough.

> I uncommented all tests because I think they are marked already as flaky at
> the expectations until we find a better solution for the timeouts and the
> integration between WTR and testharness.

I may be wrong but I don't think that any streams test is marked flaky in TestExpectations. If that is the case, we should try to remove these flaky expectations.

[Xabier Rodríguez Calvar]

(In reply to comment #4)
> > > It is good that our tests be integrated as reference tests.
> > > I guess a further patch will remove the custom redundant tests?
> > 
> > I forgot, let me upload them again O:)
> 
> As you like, doing the removal as another patch seems good too.

I think it is better to do it now as it is part of a whole.

> > > > LayoutTests/resources/gc.js:-15
> > > > -            gcRec(n-1);
> > > 
> > > Can you detail why this is no longer needed?
> > > I guess the purpose of gcRec is to be able to run the tests in not-rwt
> > > environments.
> > 
> > Domenic made that change when adapting the tests before merging them at the
> > spec. I guess his (and my) opinion is that that method is not reliable to
> > force the gc to work so he (and I) preferred to add a warning in those cases
> > and have that test dumping that warning.
> 
> Having the warning in not-rwt environments seems like a good idea.
> Keeping the particular gcRec method does no harm though, it just increases
> the chance that a gc will actually happen.

Ok, I buy it :) I'll add it again. Actually as part of reworking the custom tests I have to touch that.

> The issue is on slow bots: the test that PASS has only 50 ms to finish,
> which is not always ok on slow bots.
> Increasing the timeout value for that specific test may fix that. 1s might
> be good enough.

The problem is not the test timeouts because those tests would timeout even if we had 10s as they always timeout. I think in this case, the shorter the better because they will timeout anyway.

I'll go thru the tests that set timeouts (to let promises run and the like) and work them out by increasing the time.

> > I uncommented all tests because I think they are marked already as flaky at
> > the expectations until we find a better solution for the timeouts and the
> > integration between WTR and testharness.
> 
> I may be wrong but I don't think that any streams test is marked flaky in
> TestExpectations. If that is the case, we should try to remove these flaky
> expectations.

You are completely right, my mistake. I remember that Alexey had added and I thought that they were still in place.

[Xabier Rodríguez Calvar]

Created attachment 254798 [details]
Patch

Honored Youenn's comments. I also tested the remaining custom tests and adapted some of them to the referecence implementation behavior.

[youenn fablet]

Comment on attachment 254798 [details]
Patch

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

> LayoutTests/ChangeLog:13
> +        important as it allowed us to some of our custom tests that are now covered by the spec.

Changelog is missing some new entries (LayoutTests/streams/readable-stream-controller-error-expected.txt e.g.)
It may be good to state that some WebKit specific tests were removed as we integrate their upstreamed reference test suite version.

Maybe also explain in the Changelog that several changes to WebKit specific tests are editorial but will help the upstream process?

> LayoutTests/streams/readable-stream-controller-error-expected.txt:4
> +FAIL Erroring a ReadableStream without any value assert_equals: expected (undefined) undefined but got (object) object "Error: Error function called."

We discussed with Xabier this case.
In case of controller.error(), we should return undefined as value (curretnly we use a default message).
Plan is to fix ReadableJSStream::storeError accordingly as a future patch.

> LayoutTests/streams/readable-stream-expected.txt:-4
> -PASS ReadableStream should be able to call start method within prototype chain of its source 

All 3 tests are now in reference-implementation files hence why we can remove them safely.

> LayoutTests/streams/reference-implementation/readable-stream-cancel.html:30
> +                }), 500);

Xabier, can you tell again why the timeout here is 500 and the other timeout is 1000?
Shouldn't it be the reverse?

> LayoutTests/streams/reference-implementation/readable-stream-reader.html:194
> +var test5 = async_test('Multiple readers can access the stream in sequence');

It is good to see all those previously commented out tests now getting live.

> LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:36
>  PASS getReader() should return a reader that acts errored 

I still fear that this particular test may turn TIMEOUT in slow bots.
If I am not mistaken, it will only PASS if ending faster than 5O ms.
Hopefully, this will always happen as this is really a sync test (exception throwing).

If that is ok, let's ensure everything is fine at cq time.

> LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:42
> +PASS Running templatedRSErrored with ReadableStream (errored via returning a rejected promise in start) 

I wonder why we do not fail this test, at least until 145792 is actually landed.

> LayoutTests/streams/reference-implementation/readable-stream-templated.html:244
> +        setTimeout(test3.step_func(function() { test3.done(); }), 1000);

I do not know what is the typical WebKit policy for setting the timeout value in this kind of case.
1000 seems large enough to ensure we are compliant.

[youenn fablet]

> > LayoutTests/streams/reference-implementation/readable-stream-cancel.html:30
> > +                }), 500);
> 
> Xabier, can you tell again why the timeout here is 500 and the other timeout
> is 1000?
> Shouldn't it be the reverse?

Actually no, it is good like that!

Anyway, hoping to see these tests soon be run on bots :)

[youenn fablet]

> > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:36
> >  PASS getReader() should return a reader that acts errored 
> 
> I still fear that this particular test may turn TIMEOUT in slow bots.
> If I am not mistaken, it will only PASS if ending faster than 5O ms.
> Hopefully, this will always happen as this is really a sync test (exception
> throwing).
> 
> If that is ok, let's ensure everything is fine at cq time.

Rebasing after latest streams API patch may solve this hopefully.

[Xabier Rodríguez Calvar]

(In reply to comment #7)
> > LayoutTests/ChangeLog:13
> > +        important as it allowed us to some of our custom tests that are now covered by the spec.
> 
> Changelog is missing some new entries
> (LayoutTests/streams/readable-stream-controller-error-expected.txt e.g.)
> It may be good to state that some WebKit specific tests were removed as we
> integrate their upstreamed reference test suite version.
> 
> Maybe also explain in the Changelog that several changes to WebKit specific
> tests are editorial but will help the upstream process?

I think I explained it though I missed some words when writing. I'll rewrite it and try to be more verbose. Btw, what do you mean by editorial?

It is no problem as I have to recompile and rerun after the latest landing.

> > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:36
> >  PASS getReader() should return a reader that acts errored 
> 
> I still fear that this particular test may turn TIMEOUT in slow bots.
> If I am not mistaken, it will only PASS if ending faster than 5O ms.
> Hopefully, this will always happen as this is really a sync test (exception
> throwing).
> 
> If that is ok, let's ensure everything is fine at cq time.

I'll recheck it too.

> > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:42
> > +PASS Running templatedRSErrored with ReadableStream (errored via returning a rejected promise in start) 
> 
> I wonder why we do not fail this test, at least until 145792 is actually
> landed.

I'll recheck it too.

[youenn fablet]

 
> I think I explained it though I missed some words when writing. I'll rewrite
> it and try to be more verbose. Btw, what do you mean by editorial?

Some files are changed (removal of tests/addition of tests/changing of tests).
Some other files are changed but not in a functional way, hence "editorial"/improving style.

> > > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:42
> > > +PASS Running templatedRSErrored with ReadableStream (errored via returning a rejected promise in start) 
> > 
> > I wonder why we do not fail this test, at least until 145792 is actually
> > landed.
> 
> I'll recheck it too.

Thanks

[Xabier Rodríguez Calvar]

Created attachment 254876 [details]
Patch

Honored Youenn's comments. I also tested the remaining custom tests and adapted some of them to the referecence implementation behavior.

[Xabier Rodríguez Calvar]

(In reply to comment #10)
> > Changelog is missing some new entries
> 
> I think I explained it though I missed some words when writing. I'll rewrite
> it and try to be more verbose. Btw, what do you mean by editorial?
> 
> It is no problem as I have to recompile and rerun after the latest landing.

Done.

> > > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:36
> > >  PASS getReader() should return a reader that acts errored 
> > 
> > I still fear that this particular test may turn TIMEOUT in slow bots.
> > If I am not mistaken, it will only PASS if ending faster than 5O ms.
> > Hopefully, this will always happen as this is really a sync test (exception
> > throwing).
> > 
> > If that is ok, let's ensure everything is fine at cq time.
> 
> I'll recheck it too.

I removed that timeout as both instances pass the test. Good catch. I also checked the other timeouts and removed some of them that are passing now.

> > > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:42
> > > +PASS Running templatedRSErrored with ReadableStream (errored via returning a rejected promise in start) 
> > 
> > I wonder why we do not fail this test, at least until 145792 is actually
> > landed.
> 
> I'll recheck it too.

Bug 145792 landed and test passes properly, so moved on.

[youenn fablet]

That looks good to me.
Once landed, we can get proper coverage for bug 145965 patch.

[WebKit Commit Bot]

Comment on attachment 254876 [details]
Patch

Clearing flags on attachment: 254876

Committed r185586: <http://trac.webkit.org/changeset/185586>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.