Bug 161310 – Test infrastructure should allow to filter out text output before doing a comparison with the baseline

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

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.

[youenn fablet]

Created attachment 287278 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 287278 [details]
Patch

I don't think we should make this change.  We should make WebKit / test output deterministic instead.

[youenn fablet]

Created attachment 287760 [details]
DumpJSConsoleLogInStderr option

[youenn fablet]

(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.

[youenn fablet]

(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

[Alexey Proskuryakov]

> 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.

[youenn fablet]

(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).

[Alexey Proskuryakov]

LayoutTests/fast/text/combining-character-sequence-vertical.html and LayoutTests/fast/text/international/system-language/system-font-punctuation.html are two examples.

[youenn fablet]

(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?

[Alexey Proskuryakov]

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.

[youenn fablet]

Created attachment 287943 [details]
Separate tests options file

[youenn fablet]

Created attachment 287954 [details]
Renaming file to tests-options.json

[youenn fablet]

Created attachment 288132 [details]
Rebasing

[youenn fablet]

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).

[Darin Adler]

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.

[Ryosuke Niwa]

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.

[youenn fablet]

> 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

[youenn fablet]

> > 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.

[youenn fablet]

If that helps, I can update the patch so that no logs gets to the stderr

[youenn fablet]

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?

[youenn fablet]

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.

[Ryosuke Niwa]

(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

[Ryosuke Niwa]

Comment on attachment 288132 [details]
Rebasing

r- based on past review comments.

[youenn fablet]

Created attachment 296998 [details]
Back to TestExpectation modifier option

[Ryosuke Niwa]

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.

[youenn fablet]

Created attachment 297077 [details]
Patch

[youenn fablet]

(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.

[youenn fablet]

Created attachment 297079 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 297079 [details]
Patch

Clearing flags on attachment: 297079

Committed r209798: <http://trac.webkit.org/changeset/209798>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.