Bug 143774 - Streams API tests should have a common timeout
Summary: Streams API tests should have a common timeout
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL:
Keywords:
Depends on:
Blocks: 143363 143778
  Show dependency treegraph
 
Reported: 2015-04-15 06:57 PDT by Xabier Rodríguez Calvar
Modified: 2015-06-16 03:42 PDT (History)
4 users (show)

See Also:


Attachments
Patch (42.65 KB, patch)
2015-04-15 07:04 PDT, Xabier Rodríguez Calvar
ap: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xabier Rodríguez Calvar 2015-04-15 06:57:33 PDT
Streams API tests should have a common timeout
Comment 1 Xabier Rodríguez Calvar 2015-04-15 07:04:14 PDT
Created attachment 250787 [details]
Patch

Sets a default timeout of 150ms for all testharness tests.
Comment 2 Xabier Rodríguez Calvar 2015-04-15 08:09:52 PDT
I know this solution sucks, but there is no other way to get the properties set to the test. setup({timeout: 150}) does not work because those properties are for the test suite, not for the tests themselves.

I suggest we land this because it is needed for our bots and in parallel I'll try to patch testharness upstream.
Comment 3 youenn fablet 2015-04-15 08:12:01 PDT
(In reply to comment #2)
> I suggest we land this because it is needed for our bots and in parallel
> I'll try to patch testharness upstream.


Latest committed streams API patch failed some tests in slower bots because of timeout value being to low. Hence the idea to increase it.
Comment 4 youenn fablet 2015-04-15 08:18:30 PDT
Comment on attachment 250787 [details]
Patch

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

> LayoutTests/resources/testharnessreport.js:16
> +

I would not add this here.
Maybe we will need something like this if more WebKit testharness tests appear.

At the moment, it might be better to add it in each of the 7 test files we have.
That is duplicating 7 lines but in the future, if a specific test file is failing, we could then increase it per test/per test file, without increasing for the other files.
Comment 5 Alexey Proskuryakov 2015-04-15 11:11:44 PDT
I don't think that 150ms ever makes sense in a regression test, unless we retry when the timeout passes. It is a very short period of time, a test can legitimately stall for seconds.
Comment 6 Xabier Rodríguez Calvar 2015-04-16 03:17:58 PDT
(In reply to comment #5)
> I don't think that 150ms ever makes sense in a regression test, unless we
> retry when the timeout passes. It is a very short period of time, a test can
> legitimately stall for seconds.

I don't know if you can do that with testharness, at least automatically. The only solution I can think of is splitting all tests in single files and letting the webkit test runner handle the timeouts, but we are speaking of files with a lot of test, more than 20.
Comment 7 youenn fablet 2015-04-16 04:41:08 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > I don't think that 150ms ever makes sense in a regression test, unless we
> > retry when the timeout passes. It is a very short period of time, a test can
> > legitimately stall for seconds.
> 
> I don't know if you can do that with testharness, at least automatically.
> The only solution I can think of is splitting all tests in single files and
> letting the webkit test runner handle the timeouts, but we are speaking of
> files with a lot of test, more than 20.

There are 2 timeouts: WK test runner and test harness. We want test harness ones to be short enough so that test runner timeout is not hit. 
We could set the test harness timeout per file depending on the number of timeouting tests in the file. The less timeouting tests in a file, the longer the test harness timeout value.
Comment 8 Alexey Proskuryakov 2015-04-16 09:33:32 PDT
Comment on attachment 250787 [details]
Patch

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

> LayoutTests/ChangeLog:23
> +        * resources/testharnessreport.js: Added default property variables
> +        for tests including a timeout of 150ms.

If any timeout is short (shorter than, say, 5 seconds), then tests will be flaky. That's not OK.

Please find a way to test without introducing flakiness. This may mean finding a different way to structure the tests.
Comment 9 youenn fablet 2015-04-16 10:25:04 PDT
For the moment, we could just comment out the failing tests. We would umcomment them as we add features. And no more need to set specific timeouts.
Comment 10 Xabier Rodríguez Calvar 2015-04-16 11:55:32 PDT
(In reply to comment #8)
> Comment on attachment 250787 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250787&action=review
> 
> > LayoutTests/ChangeLog:23
> > +        * resources/testharnessreport.js: Added default property variables
> > +        for tests including a timeout of 150ms.
> 
> If any timeout is short (shorter than, say, 5 seconds), then tests will be
> flaky. That's not OK.
> 
> Please find a way to test without introducing flakiness. This may mean
> finding a different way to structure the tests.

I understand that it is not ok. The issue here is that we have a bunch of tests coming from the spec. Tests related to the same matter are placed in the same file. What we did was translating all those tests from tape to testharness (quick and painless) but we kept the files mapping so that they are easier to maintain. The key here is that a file can contain several tests so as Youenn said we have two timeouts, testharness and WebKitTestRunner.

We can do several things here:

1. Remove (or comment out) the failing tests from the files. This is hard to maintain because for every patch we want to submit we would need to readd or uncomment the harness tests to see if they pass.
2. Create a 1-1 relation between harness tests and webkit tests. This would be hell to maintain because we would need to use either random filenames or name the files after the test description, which is a plain string. Every time a test is changed in the spec we would need to rename our test and that leads to tracing problems, etc.
3. Remove all timeouts. This causes testharness timeouts to trigger webkit timeouts, which renders useless the finegraining of the testharness tests as we don't have output.

Both things reduce flakyness, but have severe issues that make our lifes much more complicated. Another solution that introduces some flakyness (that is under control right now) is keeping things as we have them now, but setting that timeout for the testharness tests (we won't touch webkit's).
Comment 11 Alexey Proskuryakov 2015-04-16 13:18:50 PDT
It is not OK to introduce any known flaky tests. Each failure has a cost, as bot watchers see a red bubble, and need to investigate why it's red.
Comment 12 youenn fablet 2015-04-16 13:19:36 PDT
Flakky tests are painful.
Let's comment out timeouting tests for the moment and remove specific test timeouts.

If no one objects, we will land these changes tomorrow as an unreviewed patch.
Comment 13 youenn fablet 2015-04-17 00:03:32 PDT
(In reply to comment #12)
> Flakky tests are painful.
> Let's comment out timeouting tests for the moment and remove specific test
> timeouts.
> 
> If no one objects, we will land these changes tomorrow as an unreviewed
> patch.

As a first, step, let's remove all timeout values from testharness tests, except for individual tests that actually timeout.
That way, we still ensure that these tests do not crash.
If that solves flakiness issues, we are good.
Otherwise, let's comment out flakky tests and readd them when features will get added.