Bug 161310 - Test infrastructure should allow to filter out text output before doing a comparison with the baseline
Summary: Test infrastructure should allow to filter out text output before doing a com...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 160546
  Show dependency treegraph
 
Reported: 2016-08-29 08:33 PDT by youenn fablet
Modified: 2016-12-14 04:23 PST (History)
7 users (show)

See Also:


Attachments
Patch (18.88 KB, patch)
2016-08-29 10:19 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
DumpJSConsoleLogInStderr option (19.31 KB, patch)
2016-09-02 08:13 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Separate tests options file (28.60 KB, patch)
2016-09-05 02:50 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Renaming file to tests-options.json (28.72 KB, patch)
2016-09-05 07:36 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (23.70 KB, patch)
2016-09-07 06:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Back to TestExpectation modifier option (29.64 KB, patch)
2016-12-13 01:59 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (23.57 KB, patch)
2016-12-14 02:22 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (31.39 KB, patch)
2016-12-14 03:12 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-08-29 08:33:14 PDT
As illustrated in bug 160546, some tests may be flaky due to one or two lines in the text output that always change run after run.
Sometimes the test can be corrected, sometimes it cannot.
It would be good if the test infrastructure could filter out these lines before doing the baseline comparison.
This could be triggered for instance with a new expectation in LayoutTests/TestExpectations.
Comment 1 youenn fablet 2016-08-29 10:19:52 PDT
Created attachment 287278 [details]
Patch
Comment 2 Ryosuke Niwa 2016-08-29 13:55:24 PDT
Comment on attachment 287278 [details]
Patch

I don't think we should make this change.  We should make WebKit / test output deterministic instead.
Comment 3 youenn fablet 2016-09-02 08:13:51 PDT
Created attachment 287760 [details]
DumpJSConsoleLogInStderr option
Comment 4 youenn fablet 2016-09-02 08:16:26 PDT
(In reply to comment #3)
> Created attachment 287760 [details]
> DumpJSConsoleLogInStderr option

New option redirects JS console log to stderr.
This retains the ability to access it while allowing to removing potential flakiness.

It seems a bit odd to add that keyword to TestExpectations since it is becoming more like a setup option. This applies also a bit to Slow keyword.
Comment 5 youenn fablet 2016-09-02 08:48:29 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Created attachment 287760 [details]
> > DumpJSConsoleLogInStderr option
> 
> New option redirects JS console log to stderr.
> This retains the ability to access it while allowing to removing potential
> flakiness.
> 
> It seems a bit odd to add that keyword to TestExpectations since it is
> becoming more like a setup option. This applies also a bit to Slow keyword.

New option only applies to WK2. A future patch would do that for WK1
Comment 6 Alexey Proskuryakov 2016-09-02 09:11:12 PDT
> It seems a bit odd to add that keyword to TestExpectations since it is becoming more like a setup option.

Setup options are normally put in tests themselves as WebKitTestRunner comments.
Comment 7 youenn fablet 2016-09-02 09:18:05 PDT
(In reply to comment #6)
> > It seems a bit odd to add that keyword to TestExpectations since it is becoming more like a setup option.
> 
> Setup options are normally put in tests themselves as WebKitTestRunner
> comments.

I never came across such tests. Do you have any example?

As of WPT tests, although test importer could put those options in tests based on a test configuration list, I think it is best to keep the WPT tests as unchanged as possible.
If that is better, I am fine putting that information in a separate place.
If so, we should also do that for Slow tests, since that information can be gathered automatically for WPT tests (meta timeout element).
Comment 8 Alexey Proskuryakov 2016-09-02 09:35:35 PDT
LayoutTests/fast/text/combining-character-sequence-vertical.html and LayoutTests/fast/text/international/system-language/system-font-punctuation.html are two examples.
Comment 9 youenn fablet 2016-09-02 09:55:36 PDT
(In reply to comment #8)
> LayoutTests/fast/text/combining-character-sequence-vertical.html and
> LayoutTests/fast/text/international/system-language/system-font-punctuation.
> html are two examples.

Does the same principle apply to DRT?
Comment 10 Alexey Proskuryakov 2016-09-02 10:09:29 PDT
I don't think that DRT checks any of these flags at this time - they were needed because we had to relaunch WebContent processes for certain options to apply. A more normal way to change settings is via testRunner functions, of course.
Comment 11 youenn fablet 2016-09-05 02:50:45 PDT
Created attachment 287943 [details]
Separate tests options file
Comment 12 youenn fablet 2016-09-05 07:36:55 PDT
Created attachment 287954 [details]
Renaming file to tests-options.json
Comment 13 youenn fablet 2016-09-07 06:30:19 PDT
Created attachment 288132 [details]
Rebasing
Comment 14 youenn fablet 2016-09-21 06:56:15 PDT
Ping review?

LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-cache.html exhibits the same issue after implementing fetch cache mode (in WK2 only as the tests causing always changing console log are failing early in WK1 currently).
Comment 15 Darin Adler 2016-09-26 17:41:49 PDT
Comment on attachment 288132 [details]
Rebasing

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

I do not understand why this is a good option. I am not happy about our current handling of stderr where we note that there was stderr output as if it’s always a bad ting but it does not cause test failures. This causes us to ignore the stderr output most of the time and the list of tests with stderr output becomes a source of noise.

> Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:80
> +        } else if (arg == std::string("--dump-jsconsolelog-in-stderr"))

The conversion to std::string is not needed here, or is it needed in the three other places above.
Comment 16 Ryosuke Niwa 2016-09-26 19:18:50 PDT
Comment on attachment 288132 [details]
Rebasing

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

> LayoutTests/tests-options.json:116
> +    "imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight.html": [

This file shouldn't be in LayoutTests/tests-options.json.
It should be in LayoutTests/imported/w3c/web-platform-tests/w3c/web-platform-tests/

> LayoutTests/tests-options.json:119
> +    "imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-worker.html": [

We should just have W3C modify these tests to not spit out non-deterministic outputs instead.
Comment 17 youenn fablet 2016-09-27 02:42:24 PDT
> I do not understand why this is a good option. I am not happy about our
> current handling of stderr where we note that there was stderr output as if
> it’s always a bad ting but it does not cause test failures. This causes us
> to ignore the stderr output most of the time and the list of tests with
> stderr output becomes a source of noise.

I mostly see the stderr as useful information for debugging.
If some bits of the stderr contains information that should be part of non regression, we might want to update the code to output those messages in the dumped results.


> > Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:80
> > +        } else if (arg == std::string("--dump-jsconsolelog-in-stderr"))
> 
> The conversion to std::string is not needed here, or is it needed in the
> three other places above.

OK
Comment 18 youenn fablet 2016-09-27 02:46:41 PDT
> > LayoutTests/tests-options.json:116
> > +    "imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight.html": [
> 
> This file shouldn't be in LayoutTests/tests-options.json.
> It should be in
> LayoutTests/imported/w3c/web-platform-tests/w3c/web-platform-tests/

We could do that.
Putting the file here makes it possible to remove slow modifier from TestExpectations.
It feels odd to me when seeing something like: my-wonderful-test [ Slow Crash ]
The slow modifier is just setting the timeout option, so it seems more like a test option than a text expectation.

Also, I am not very fond of the TestExpectations format in general.
It is difficult to write new expectations/update expectations on them for instance.

> > LayoutTests/tests-options.json:119
> > +    "imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-worker.html": [
> 
> We should just have W3C modify these tests to not spit out non-deterministic
> outputs instead.

It spits non-deterministic outputs on WebKit test environment not on other test environments.

One purpose of using dynamic URLs is cache busting.
Doing so ensures more stable tests, for instance when running several times the same test.

There might be the possibility to use specific test-runner API, triggered by some metadata.
But then, the tests would no longer be stable on web browsers. That is not attractive to me.
Comment 19 youenn fablet 2016-09-30 08:47:06 PDT
If that helps, I can update the patch so that no logs gets to the stderr
Comment 20 youenn fablet 2016-10-12 06:23:38 PDT
Now that http://trac.webkit.org/changeset/207086> landed, I would like to improve on it based on request-cache.html for WK2.
Would it be possible to move this patch forward?
Comment 21 youenn fablet 2016-11-22 04:41:51 PST
Talking briefly with rniwa, he seems to have hard feelings for removing all console log from dumped text, but for a hand-selected set of tests, this might be ok.

Can we go forward on that? I need this kind of feature to ship fetch API console logging without disabling WPT fetch API tests.
Comment 22 Ryosuke Niwa 2016-12-12 14:22:53 PST
(In reply to comment #18)
> > > LayoutTests/tests-options.json:116
> > > +    "imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight.html": [
> > 
> > This file shouldn't be in LayoutTests/tests-options.json.
> > It should be in
> > LayoutTests/imported/w3c/web-platform-tests/w3c/web-platform-tests/
> 
> We could do that.
> Putting the file here makes it possible to remove slow modifier from
> TestExpectations.
> It feels odd to me when seeing something like: my-wonderful-test [ Slow
> Crash ]
> The slow modifier is just setting the timeout option, so it seems more like
> a test option than a text expectation.

This is really by design.  We didn’t want to have 3-4 configuration files to figure out what a test does.

In the case of W3C tests, we don’t want to modify test files, but for all other test cases, we should be using in-file test options instead.  This whole concept of test-options.json should only exist for the imported W3C tests.

> Also, I am not very fond of the TestExpectations format in general.
> It is difficult to write new expectations/update expectations on them for
> instance.

We can have that debate on webkit-dev. There was a long & heated debate about what format we use for TestExpectations back in 2012: https://lists.webkit.org/pipermail/webkit-dev/2012-June/021131.html
Comment 23 Ryosuke Niwa 2016-12-12 14:23:05 PST
Comment on attachment 288132 [details]
Rebasing

r- based on past review comments.
Comment 24 youenn fablet 2016-12-13 01:59:45 PST
Created attachment 296998 [details]
Back to TestExpectation modifier option
Comment 25 Ryosuke Niwa 2016-12-13 14:35:01 PST
Comment on attachment 296998 [details]
Back to TestExpectation modifier option

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

r=me provided you have a good explanation for how stderr is piped from WTR’s WebContent process to UIProcess.

> Tools/ChangeLog:17
> +
> +

Nit: There are two blank lines here.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:349
> -            if 'SKIP' not in modifiers and 'REBASELINE' not in modifiers and 'SLOW' not in modifiers:
> +            if 'SKIP' not in modifiers and 'REBASELINE' not in modifiers and 'SLOW' not in modifiers and 'DUMPJSCONSOLELOGINSTDERR' not in modifiers:

This is getting really long. Instead, we should just check whether modifiers is empty or it only contains “WontFix”.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1466
> +        fprintf(stderr, "%s", stringBuilder.toString().utf8().data());

How does this even work? injectedBundle.outputText sends TextOutput message to UIProcess.
Comment 26 youenn fablet 2016-12-14 02:22:52 PST
Created attachment 297077 [details]
Patch
Comment 27 youenn fablet 2016-12-14 02:44:53 PST
(In reply to comment #25)
> Comment on attachment 296998 [details]
> Back to TestExpectation modifier option
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296998&action=review
> 
> r=me provided you have a good explanation for how stderr is piped from WTR’s
> WebContent process to UIProcess.
> 
> > Tools/ChangeLog:17
> > +
> > +
> 
> Nit: There are two blank lines here.

OK

> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:349
> > -            if 'SKIP' not in modifiers and 'REBASELINE' not in modifiers and 'SLOW' not in modifiers:
> > +            if 'SKIP' not in modifiers and 'REBASELINE' not in modifiers and 'SLOW' not in modifiers and 'DUMPJSCONSOLELOGINSTDERR' not in modifiers:
> 
> This is getting really long. Instead, we should just check whether modifiers
> is empty or it only contains “WontFix”.

I tried changing this but it fails some webkitpy unit tests.
I added a FIXME though.

> > Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1466
> > +        fprintf(stderr, "%s", stringBuilder.toString().utf8().data());
> 
> How does this even work? injectedBundle.outputText sends TextOutput message
> to UIProcess.

I think the UIProcess is taking care of sharing the stdout/stderr descriptors to the WebProcess.
But you are right, this is not clean.
I updated the patch by adding a DumpToStdErr message.
Comment 28 youenn fablet 2016-12-14 03:12:05 PST
Created attachment 297079 [details]
Patch
Comment 29 WebKit Commit Bot 2016-12-14 04:23:22 PST
Comment on attachment 297079 [details]
Patch

Clearing flags on attachment: 297079

Committed r209798: <http://trac.webkit.org/changeset/209798>
Comment 30 WebKit Commit Bot 2016-12-14 04:23:30 PST
All reviewed patches have been landed.  Closing bug.