Bug 145313 – Testharness-based tests that time out should be able to produce detailed output

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[youenn fablet]

Currently, the timeout for testharness based tests is set very high so that WebKit timeout always kicks in first.
This prevents testharness to produce meaningful reports, as in a single file, one test may timeout but not the others.
testharness should be able to report which tests are timeouting and which tests are not.

A first approach would be to put testharness timeout as the minimum value of all bots timeout.

One potential issue is that WebKit timeout depends on bot options (Mac, GTK, Debug, Release, Guard Malloc...), while testharness timeouts only depend on the document.
If the first approach does not work, the infrastructure may be upgraded to set the testharness.js timeout value dynamically at the beginning of the test process.

[youenn fablet]

Created attachment 253601 [details]
Testing 5s timeout

[youenn fablet]

Comment on attachment 253601 [details]
Testing 5s timeout

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

> LayoutTests/ChangeLog:8
> +        * resources/testharness.js: Setting timeout to 5 seconds for normal tests and 6 seconds for long tests.

Plan would be to land this patch and see if we have regressions/flaky tests, especially with slow bots.
If so, roll out this patch and set testharness.js timeout dynamically at run-webkit-tests start time.

> LayoutTests/resources/testharness.js:21
> +            "normal":5000,

GTK timeout in release seems to be 6 seconds.

[Xabier Rodríguez Calvar]

For this I would do several things:

1) Declaring all harness tests as Slow so that the long timeout kicks in instead of the short. This increases our changes of finishing even with long harness timeouts as 5s.
2) Declaring harness timeouts as a fraction of WTR timeouts (preferrably as a fraction of the short timeout). This helps to keep coherence between harness and WTR.
3) Forcing somehow that output is meaningful, other than "notifyDone was not called" in case of a WTR timeout. In that case we would still have some information of which tests failed.

[youenn fablet]

(In reply to comment #3)
> For this I would do several things:
> 
> 1) Declaring all harness tests as Slow so that the long timeout kicks in
> instead of the short. This increases our changes of finishing even with long
> harness timeouts as 5s.
> 2) Declaring harness timeouts as a fraction of WTR timeouts (preferrably as
> a fraction of the short timeout). This helps to keep coherence between
> harness and WTR.
> 3) Forcing somehow that output is meaningful, other than "notifyDone was not
> called" in case of a WTR timeout. In that case we would still have some
> information of which tests failed.

I am not familiar with "Slow". Can we set it within the test itself?

The patch here does 3 plus a simple version of 2 (taking the min of all DRT/WTR timeout and substracting some time, hence 5 seconds).

If the 5 seconds timeout is not good enough, testRunner.timeout() could be added to let testharness.js computes its own timeout from WTR/DRT own timeout, be it slow or regular.

[Xabier Rodríguez Calvar]

(In reply to comment #4)
> I am not familiar with "Slow". Can we set it within the test itself?

[Slow] is something you can put in the expectations

> If the 5 seconds timeout is not good enough, testRunner.timeout() could be
> added to let testharness.js computes its own timeout from WTR/DRT own
> timeout, be it slow or regular.

This is what I think it is better.

[Alexey Proskuryakov]

Is there a specific case of a failure that you could point out?

Generally, I don't think that it's a good idea to have layers of "test harnesses" below run-webkit-tests. What run-webkit-tests does should be enough. Even if you fully address timeouts, there is a lot of infrastructure that works best when run-webkit-tests is allowed to run each test. Notably, you won't get good flakiness dashboard history with an intermediate harness.

Marking the tests as slow in TestExpectations is OK as a short term solution, however it doesn't look like we are moving in the right direction here.

[youenn fablet]

(In reply to comment #6)
> Is there a specific case of a failure that you could point out?

In web-platform-tests, there are around 200-300 files that contain more than one test.
If one of this test is sometimes failing, the flakiness dashboard will show which test is failing. This is fine.
If one of this test is sometimes timeouting, the flakiness dashboard will not show which test is timeouting. This is bad, especially since it is due to WebKit way of using testharness.js.

Another usecase is the LayoutTests/streams/reference-implementation tests.
The tests here are organised so as to minimize the resynchronization between WebKit tests and WhatWG reference tests. This is important as the synchronization is an important but tedious activity.

You can look in particular to LayoutTests/streams/reference-implementation/readable-stream-templated.html
We actually needed to comment out some tests because one of the templated test is passing, the other is timeouting. But we cannot use the "timeout: 50" trick since the passing templated test will become flaky in slow bots.

> Generally, I don't think that it's a good idea to have layers of "test
> harnesses" below run-webkit-tests. What run-webkit-tests does should be
> enough. Even if you fully address timeouts, there is a lot of infrastructure
> that works best when run-webkit-tests is allowed to run each test. Notably,
> you won't get good flakiness dashboard history with an intermediate harness.

There are two different points here:
1. Files authored for/originating from WebKit should contain one test only.
Maybe this should be enforced more strongly or at least be advised more clearly, for instance in http://trac.webkit.org/wiki/Writing%20testharness%20Tests
I agree this helps getting good flakiness dashboard here.
One downsides though is that getting all tests of a single feature in a file helps checking coverage.

2. For files imported within WebKit, we should make our infrastructure closer to what is expected to run the tests.
Testharness.js tests are designed with the idea that testharness scripts handle timeout.
Having a mismatch with WebKit is unfortunate here, it makes things worse for the flakiness dashboard.

I also see some benefits in enabling JS tests to know what is the timeout.
For instance, a test could automatically count the number of assertions that were passed and if timeouting, dump that information. This is easy in JS, probably not in rwt/WTR/DRT.

Adding a timeout getter is not a lot of work and seems much more scalable than making tests slow.

[Alexey Proskuryakov]

> In web-platform-tests, there are around 200-300 files that contain more than one test.
> If one of this test is sometimes failing, the flakiness dashboard will show which test is failing. This is fine.

I'm not sure what you mean by that. Certainly the flakiness dashboard will only know the main test file, not the failing subtest.

> 1. Files authored for/originating from WebKit should contain one test only.

This is difficult to formalize. Of course it's OK to test multiple aspects of a feature in one test file, we have plenty of tests with many subtests. However that should play well with the infrastructure - the test should be fast, a failure of any subtest should logically mean that the whole test failed, etc.

> 2. For files imported within WebKit, we should make our infrastructure closer to what is expected to run the tests.

I'm not sure if it's worth the investment. So far, the imported tests don't seem to be very good about finding regressions, but we certainly have a lot of added complexity and flakiness.

> For instance, a test could automatically count the number of assertions that were passed and if timeouting, dump that information. This is easy in JS, probably not in rwt/WTR/DRT.

That makes sense as a debugging facility. I would approach it differently though - the test would use a new testRunner function to store some text that is normally invisible, but gets dumped on timeout. That should be more reliable, as we wouldn't have to depend on the already broken test, which could be just spinning in a  tight loop.

[youenn fablet]

(In reply to comment #8)
> > In web-platform-tests, there are around 200-300 files that contain more than one test.
> > If one of this test is sometimes failing, the flakiness dashboard will show which test is failing. This is fine.
> 
> I'm not sure what you mean by that. Certainly the flakiness dashboard will
> only know the main test file, not the failing subtest.

The flakiness dashboard will show the diff.
In the case of a failing sub-test, we will know which sub-test is failing and which one is not.

In the case of a timeouting sub-test, we have no information at all on which sub-test timeouted. It is as if all sub-tests timeouted.

This is very inconvenient and this is one thing we want to address here.

> > 2. For files imported within WebKit, we should make our infrastructure closer to what is expected to run the tests.
> 
> I'm not sure if it's worth the investment. So far, the imported tests don't
> seem to be very good about finding regressions, but we certainly have a lot
> of added complexity and flakiness.

They may not be that good at regressions but they are very good at finding non-conformities, which is very important.
For instance, the wpt/XMLHttpRequest tests show where WebKit should improve XHR support.
The same applies to the streams reference implementation which are really helpful to handle edge cases properly.

The flakiness and complexity is in part due to the sub-optimal integration of the test framework.

> > For instance, a test could automatically count the number of assertions that were passed and if timeouting, dump that information. This is easy in JS, probably not in rwt/WTR/DRT.
> 
> That makes sense as a debugging facility. I would approach it differently
> though - the test would use a new testRunner function to store some text
> that is normally invisible, but gets dumped on timeout. That should be more
> reliable, as we wouldn't have to depend on the already broken test, which
> could be just spinning in a  tight loop.

When I encounter a timeouting test, it is usually async tests not being able to call done(). For instance a promise is not resolved or a callback is not called or not called at the right time...
In those cases, handling at JS level works very well and is much more flexible. 

This patch (or adding a timeout getter) will handle these cases in a better way.

I have not encountered this kind of spinning tests that timeout.
Does that really happen often?
I would think wpt tests to be written well enough to not fall into that trap.

Tieing a test to a particular test runner is good when there is strong reason (manual tests...).
But overall, I would favor the ability to write the test so that it can be run in other browsers in a manner as close to Webkit.

[Alexey Proskuryakov]

> The flakiness dashboard will show the diff.

What it doesn't show is which test fails when - you would have to click through all red boxes and look at all the diffs manually. This is less than what the dashboard is supposed to provide - it should tell you which tests are flaky and which tests fail. Commenting out subtests and manually looking through diffs is a waste of time.

> I have not encountered this kind of spinning tests that timeout.
> Does that really happen often?

This certainly happens.

It's just the right design to not have already broken code responsible for debugging itself, don't you agree?

[youenn fablet]

(In reply to comment #10)
> > The flakiness dashboard will show the diff.
> 
> What it doesn't show is which test fails when - you would have to click
> through all red boxes and look at all the diffs manually. This is less than
> what the dashboard is supposed to provide - it should tell you which tests
> are flaky and which tests fail. Commenting out subtests and manually looking
> through diffs is a waste of time.

I am not sure to follow.
A good example of the issue is https://bugs.webkit.org/show_bug.cgi?id=145643.
With that patch or similar, this would not have happened.

The flakiness dashboard tells the file is flaky, but it does not tell us which subtest is flaky. If it could show which subtest is flaky(even after a few additional clicks), debugging would be made much more efficient.

> 
> > I have not encountered this kind of spinning tests that timeout.
> > Does that really happen often?
> 
> This certainly happens.

OK.
I have not seen it for WPT/testharness.js tests though.
I guess cross-checks made when upstreaming tests generally ensure that the test is well written enough.

> It's just the right design to not have already broken code responsible for
> debugging itself, don't you agree?

I agree in general but practically speaking, I do not think it will make any difference for testharness tests.

That said, I also like your suggestion of adding a "addTraceText" method. This will be less straightforward and less efficient for that particular issue but it may other uses.

Here is how that could be used: testharness.js allow registering callbacks to store information on each test, especially whenever there is a state change.
That information would be useful as a complement to -stderr.txt traces.

Using that information to generate a -actual.txt might not be straightforward (trace ordering issues for instance).
But this might be worth a try. 
Something like setTimeout(String, Level), the Level parameter controlling whether it goes in -actual.txt for timeouting tests?

Would you be ok with that approach?

[youenn fablet]

> Something like setTimeout(String, Level), the Level parameter controlling
> whether it goes in -actual.txt for timeouting tests?

Maybe testRunner.log(String, Level) would be more appropriate.

> Would you be ok with that approach?

[Alexey Proskuryakov]

I would be against making the testing machinery more complicated just for these tests. If importing W3C tests is impractical, then we simply shouldn't try to have automatic import.

[youenn fablet]

(In reply to comment #13)
> I would be against making the testing machinery more complicated just for
> these tests. If importing W3C tests is impractical, then we simply shouldn't
> try to have automatic import.

I see four steps:
1. Add testRunner.log()
2. Dump result of testRunner.log() when test is crashing/timeouting/failing
3. Upgrade testharnessreport.js to use that facility
4. Generate -actual.txt from log information for timeouting tests.

The first 3 steps would already be beneficial.
Is that too complex?

Step 4 would indeed need some more checking.

[youenn fablet]

Created attachment 254491 [details]
Pure JS version, using new testharness.js callbacks

[youenn fablet]

(In reply to comment #15)
> Created attachment 254491 [details]
> Pure JS version, using new testharness.js callbacks

Using latest testharness.js, this patch checks for any progress made in running the tests.
If there is none during 4 seconds, testharnessreport.js stops the tests and dump the results.

[youenn fablet]

There is an additional bonus of using testharness.js to handle timeouts.
WPT tests contain metadata to identify which tests are slow and which are not.
testharness.js will set its internal timeout accordingly.
No need to modify test expectations.

See https://github.com/w3c/web-platform-tests/blob/master/XMLHttpRequest/xmlhttprequest-timeout-aborted.html for instance.

[youenn fablet]

Created attachment 265586 [details]
Adding timeout as testRunner attribute

[youenn fablet]

(In reply to comment #18)
> Created attachment 265586 [details]
> Adding timeout as testRunner attribute

This patch allows dumping testharness tests state before timing out.
As can be seen from one rebased test in this patch, this can give interesting information for some tests, like for imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/environment-changes/viewport-change.html.

In that particular case, it could even be interesting to ensure that no text regression happens between runs even if the test is constantly timing out.

[youenn fablet]

Created attachment 265668 [details]
Trying to call done for testharness.js timeouting tests

[Build Bot]

Comment on attachment 265668 [details]
Trying to call done for testharness.js timeouting tests

Attachment 265668 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/440888

New failing tests:
imported/w3c/web-platform-tests/html/semantics/document-metadata/the-link-element/link-style-error-01.html
imported/w3c/web-platform-tests/html/semantics/document-metadata/the-style-element/style-error-01.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/environment-changes/viewport-change.html

[Build Bot]

Created attachment 265671 [details]
Archive of layout-test-results from ews103 for mac-mavericks

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

[Build Bot]

Comment on attachment 265668 [details]
Trying to call done for testharness.js timeouting tests

Attachment 265668 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/440884

New failing tests:
imported/w3c/web-platform-tests/html/semantics/document-metadata/the-link-element/link-style-error-01.html
imported/w3c/web-platform-tests/html/semantics/document-metadata/the-style-element/style-error-01.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/environment-changes/viewport-change.html

[Build Bot]

Created attachment 265672 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 265678 [details]
Fixing WK1

[Darin Adler]

Comment on attachment 265678 [details]
Fixing WK1

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

I don’t fully understand what’s being done here. I do have a few comments.

> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/environment-changes/viewport-change.html:46
> -        assert_equals(img.currentSrc, expected[current]);
> +        // WebKit change assert_equals -> assert_true to ensure no flakiness.
> +        assert_true(img.currentSrc == expected[current]);

I do not understand this change.

> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/environment-changes/viewport-change.html:55
> -            assert_equals(img.currentSrc, expected[next]);
> +              // WebKit change assert_equals -> assert_true to ensure no flakiness.
> +              assert_true(img.currentSrc == expected[next]);

I do not understand this change.

> LayoutTests/resources/testharnessreport.js:21
> +    // Restricting calling to testharness timeout() to wptserve tests for the moment.
> +    if (self.testRunner.timeout && window.location.port == 8800)
> +        setTimeout(timeout, self.testRunner.timeout * 0.9);

Should just be testRunner, not self.testRunner. Also can just be location, not window.location.

> LayoutTests/resources/testharnessreport.js:79
> +	}			

Shouldn’t add this trailing whitespace.

[youenn fablet]

The reason for timing out testharness.js-based files is usually that an async_test in the file is not calling done().
testharness.js allows handling that through the "timeout()" function.
When called, this function will stop all tests in the file being run and will call the completion callback.
The completion callback in WebKit is generating a text description and then calling testRunner.done().

The advantages of having a detailed description is:
1. Ensure non-regression for the tests in these previously-timing out files
2. Easier debugging (the timing out tests will be identified in the -actual.txt file)

This patch takes care of these cases by calling "timeout()" when WTR is about to stop running the test.
That is why we introduce testRunner.timeout readonly attribute.
By calling timeout(), WTR can generate a status description of each test inside the file.
As can be seen in rebased -expected.txt files, file expectation goes from TIMEOUT to either FAIL or PASS.

This patch will not handle the cases where JS code in the file is spinning.
These cases will stay as TIMEOUT, with no description.
We could probably further improve that with other testharness.js callbacks but I am not sure how much we would gain there.

One potential issue about the current patch strategy is that if the "timeout()" takes tool long to execute, WTR may consider the file to timeout in some bots. In that case, the file may be flaky.
To check this particular issue, this patch restricts the behavior changes to WPT test cases only.
If successful, this would be extended to all WebKit layout tests.

> > LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/environment-changes/viewport-change.html:46
> > -        assert_equals(img.currentSrc, expected[current]);
> > +        // WebKit change assert_equals -> assert_true to ensure no flakiness.
> > +        assert_true(img.currentSrc == expected[current]);
> 
> I do not understand this change.

When failing, assert_true will display a message that does not vary over test run.
When failing, assert_equals will display a message containing img.currentSrc and expected[current].
This will make the message to vary for each run (expected[current] contains a token which changes for each run). 

By making this change, we allow marking the test expectation to be PASS.
Without this change, we would mark the test expectation as FAIL.

Firefox, IIRC, is evaluating testharness.js results by checking PASS/FAIL/TIMEOUT for each test, and is not considering differences in the failing assertion messages as meaningful, contrary to WebKit.

> 
> > LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/environment-changes/viewport-change.html:55
> > -            assert_equals(img.currentSrc, expected[next]);
> > +              // WebKit change assert_equals -> assert_true to ensure no flakiness.
> > +              assert_true(img.currentSrc == expected[next]);
> 
> I do not understand this change.

Ditto.

> > LayoutTests/resources/testharnessreport.js:21
> > +    // Restricting calling to testharness timeout() to wptserve tests for the moment.
> > +    if (self.testRunner.timeout && window.location.port == 8800)
> > +        setTimeout(timeout, self.testRunner.timeout * 0.9);
> 
> Should just be testRunner, not self.testRunner. Also can just be location,
> not window.location.

OK

> > LayoutTests/resources/testharnessreport.js:79
> > +	}			
> 
> Shouldn’t add this trailing whitespace.

OK

[youenn fablet]

Created attachment 275658 [details]
Removing test change and updated according review

[Build Bot]

Comment on attachment 275658 [details]
Removing test change and updated according review

Attachment 275658 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1103710

New failing tests:
imported/w3c/web-platform-tests/html/dom/dynamic-markup-insertion/opening-the-input-stream/010.html

[Build Bot]

Created attachment 275663 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 275658 [details]
Removing test change and updated according review

Attachment 275658 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1103705

New failing tests:
imported/w3c/web-platform-tests/html/dom/dynamic-markup-insertion/opening-the-input-stream/010.html

[Build Bot]

Created attachment 275665 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275658 [details]
Removing test change and updated according review

Attachment 275658 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1103716

New failing tests:
imported/w3c/web-platform-tests/html/dom/dynamic-markup-insertion/opening-the-input-stream/010.html

[Build Bot]

Created attachment 275667 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 275678 [details]
Fixing tests

[Xabier Rodríguez Calvar]

Comment on attachment 275678 [details]
Fixing tests

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

> Tools/ChangeLog:3
> +        Testharness-based tests that time out should be able to produce a detailled output

Detailed

> Tools/ChangeLog:8
> +       Adding timeout readonly accessor to TestRunner.

Indentation

> Tools/ChangeLog:17
> +        * DumpRenderTree/TestRunner.cpp:
> +        (getTimeoutCallback):
> +        (TestRunner::staticValues):
> +        * DumpRenderTree/TestRunner.h:
> +        (TestRunner::timeout):
> +        * WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:
> +        * WebKitTestRunner/InjectedBundle/TestRunner.h:
> +        (WTR::TestRunner::timeout):

Please, explain what is done here or above.

> LayoutTests/resources/testharnessreport.js:19
> +    // Restricting calling to testharness timeout() to wptserve tests for the moment.

Why wptserve tests only?

> LayoutTests/resources/testharnessreport.js:65
> +	for(var i=0; i<tests.length; i++) {

for(var i = 0; i < tests.length; i++) {

> LayoutTests/resources/testharnessreport.js:76
> +					( (tests[i].name!=null) ? tests[i].name : "" ) + " " +

I think this trailing + " " + is the cause for the PASS tests to have a trailing space at the end, which is really annoying, IMHO. I know we'd have to create new baselines for all these tests, but my obsesive-compulsive disorder and the pre-commit hooks for trailing spaces would be very happy.

[youenn fablet]

Thanks for the review.
See below.

> > Tools/ChangeLog:3
> > +        Testharness-based tests that time out should be able to produce a detailled output
> 
> Detailed

OK

> > Tools/ChangeLog:8
> > +       Adding timeout readonly accessor to TestRunner.
> 
> Indentation

OK

> > Tools/ChangeLog:17
> > +        * DumpRenderTree/TestRunner.cpp:
> > +        (getTimeoutCallback):
> > +        (TestRunner::staticValues):
> > +        * DumpRenderTree/TestRunner.h:
> > +        (TestRunner::timeout):
> > +        * WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:
> > +        * WebKitTestRunner/InjectedBundle/TestRunner.h:
> > +        (WTR::TestRunner::timeout):
> 
> Please, explain what is done here or above.

Will do.

> > LayoutTests/resources/testharnessreport.js:19
> > +    // Restricting calling to testharness timeout() to wptserve tests for the moment.
> 
> Why wptserve tests only?

I'll add more explanation.
The risk is that on slow bots, dumping results is triggered too late to finish properly. This temporary fix limits the change to a small identified number of tests.
If this is a non issue, this change should be deployed for all layout tests.

> > LayoutTests/resources/testharnessreport.js:65
> > +	for(var i=0; i<tests.length; i++) {
> 
> for(var i = 0; i < tests.length; i++) {

OK

> > LayoutTests/resources/testharnessreport.js:76
> > +					( (tests[i].name!=null) ? tests[i].name : "" ) + " " +
> 
> I think this trailing + " " + is the cause for the PASS tests to have a
> trailing space at the end, which is really annoying, IMHO. I know we'd have
> to create new baselines for all these tests, but my obsesive-compulsive
> disorder and the pre-commit hooks for trailing spaces would be very happy.

Fully agreeing....
I'll be happy to help if you want to have a take :)

[Xabier Rodríguez Calvar]

(In reply to comment #37)
> > Why wptserve tests only?
> 
> I'll add more explanation.
> The risk is that on slow bots, dumping results is triggered too late to
> finish properly. This temporary fix limits the change to a small identified
> number of tests.
> If this is a non issue, this change should be deployed for all layout tests.

Ok.

> > > LayoutTests/resources/testharnessreport.js:76
> > > +					( (tests[i].name!=null) ? tests[i].name : "" ) + " " +
> > 
> > I think this trailing + " " + is the cause for the PASS tests to have a
> > trailing space at the end, which is really annoying, IMHO. I know we'd have
> > to create new baselines for all these tests, but my obsesive-compulsive
> > disorder and the pre-commit hooks for trailing spaces would be very happy.
> 
> Fully agreeing....
> I'll be happy to help if you want to have a take :)

Lol, I would like to have time for it, but I don't. As a compromise I can accept creating a follow up bug to solve this (that will be orphan for some time I guess).

[Xabier Rodríguez Calvar]

Btw, I didn't r+ cause it didn't have r?. Are you waiting for some more input or anything?

[youenn fablet]

Created attachment 275767 [details]
Fixing GTK baseline

[youenn fablet]

> Lol, I would like to have time for it, but I don't. As a compromise I can
> accept creating a follow up bug to solve this (that will be orphan for some
> time I guess).

Sounds good to me.
It seems like a regexp script should do the trick.

[Xabier Rodríguez Calvar]

Comment on attachment 275767 [details]
Fixing GTK baseline

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

> Tools/ChangeLog:18
> +        Adding timeout readonly accessor to TestRunner.
> +
> +        * DumpRenderTree/TestRunner.cpp:
> +        (getTimeoutCallback): The timeout getter.
> +        (TestRunner::staticValues): Adding "timeout" property to DumpRenderTree.
> +        * DumpRenderTree/TestRunner.h:
> +        (TestRunner::timeout): The timeout getter.
> +        * WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl: Adding timeout readonly attribute.
> +        * WebKitTestRunner/InjectedBundle/TestRunner.h: Adding timeout getter.
> +        (WTR::TestRunner::timeout):
> +

Please, ellaborate this a bit more. I can see that you are declaring things, but I cannot guess why they are needed or what they are used for.

> LayoutTests/ChangeLog:9
> +        * platform/gtk/imported/w3c/web-platform-tests/fetch/api/request/request-cache-expected.txt: GTK specifc baseline.

specifc -> specific

[youenn fablet]

Created attachment 276004 [details]
Patch for landing

[youenn fablet]

(In reply to comment #42)
> Please, ellaborate this a bit more. I can see that you are declaring things,
> but I cannot guess why they are needed or what they are used for.
> 
> > LayoutTests/ChangeLog:9
> > +        * platform/gtk/imported/w3c/web-platform-tests/fetch/api/request/request-cache-expected.txt: GTK specifc baseline.
> 
> specifc -> specific

Thanks for the review.
I updated the logs accordingly.

[WebKit Commit Bot]

Comment on attachment 276004 [details]
Patch for landing

Clearing flags on attachment: 276004

Committed r199225: <http://trac.webkit.org/changeset/199225>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.