Bug 194283 – webkit-patch print-expectations fails to format TestExpectationLine with DumpJSConsoleLogInStdErr

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Fujii Hironori]

webkit-patch print-expectations fails to format TestExpectationLine with DumpJSConsoleLogInStdErr

> $ ./Tools/Scripts/webkit-patch print-expectations --port=wincairo imported/w3c/web-platform-tests/fetch/
> --lint-test-files warnings:
> LayoutTests/platform/gtk/TestExpectations:3782 Path does not exist. imported/w3c/web-platform-tests/web-animations/timing-model/animations/current-time.html
> 
> Traceback (most recent call last):
>   File "./Tools/Scripts/webkit-patch", line 84, in <module>
>     main()
>   File "./Tools/Scripts/webkit-patch", line 79, in main
>     WebKitPatch(os.path.abspath(__file__)).main()
>   File "/mnt/c/webkit/ga/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
>     result = command.check_arguments_and_execute(options, args, self)
>   File "/mnt/c/webkit/ga/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
>     return self.execute(options, args, tool) or 0
>   File "/mnt/c/webkit/ga/Tools/Scripts/webkitpy/tool/commands/queries.py", line 501, in execute
>     print('\n'.join(self._format_lines(options, port_name, lines)))
>   File "/mnt/c/webkit/ga/Tools/Scripts/webkitpy/tool/commands/queries.py", line 525, in _format_lines
>     output.append("%s" % line.to_string(None, include_modifiers, include_expectations, include_comment=False))
>   File "/mnt/c/webkit/ga/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py", line 460, in to_string
>     include_modifiers, include_expectations, include_comment)
>   File "/mnt/c/webkit/ga/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py", line 510, in _format_line
>     result += ' [ %s ]' % ' '.join(sorted(set(new_expectations)))
> TypeError: sequence item 0: expected string, NoneType found

[Fujii Hironori]

Created attachment 361173 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 361173 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:244
> +        'DumpJSConsoleLogInStdErr': 'DUMPJSCONSOLELOGINSTDERR',

Would we need changes in the flakiness dashboard to support this value?

I think that we are hitting a design mistake with regards to DumpJSConsoleLogInStdErr - it is not an expectation, so mixing it with expectations causes trouble.

[Jonathan Bedard]

This bug very much confuses me. Why would we have a test expectation with DumpJSConsoleLogInStdErr in it? That feels like an abuse of the TestExpectations.

[youenn fablet]

> Would we need changes in the flakiness dashboard to support this value?

No

> I think that we are hitting a design mistake with regards to
> DumpJSConsoleLogInStdErr - it is not an expectation, so mixing it with
> expectations causes trouble.

It is not really a test expectation, it is closer to 'Slow' in the sense it is changing the way the test is run.
It was put there and not in some other file for convenience following the 'one place is better than two'.

[Fujii Hironori]

DumpJSConsoleLogInStdErr has been introduced in Bug 161310.

webkit-test-runner magic comments or window.testRunner or window.internals can be the alternatives.

Some imported tests have been modifed to have a webkit-test-runner magic comment.
E.g. https://trac.webkit.org/browser/webkit/trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html?rev=241112#L1

internals.setConsoleMessageListener() doesn't prevent console log.

Settings::logsPageMessagesToSystemConsoleEnabled doesn't prevent console log.

[youenn fablet]

(In reply to Fujii Hironori from comment #5)
> DumpJSConsoleLogInStdErr has been introduced in Bug 161310.
> 
> webkit-test-runner magic comments or window.testRunner or window.internals
> can be the alternatives.

DumpJSConsoleLogInStdErr is mostly useful for imported tests that we do not want to modify.
For tests that we can modify, there are usually ways, though not always, to not rely on it.

> Some imported tests have been modifed to have a webkit-test-runner magic
> comment.
> E.g.
> https://trac.webkit.org/browser/webkit/trunk/LayoutTests/imported/w3c/web-
> platform-tests/css/css-logical/logical-box-border-color.html?rev=241112#L1


When reimporting these files, these comments will be lost by the importer and one will need to add them again. I hope these changes are expected to be temporary until we can activate the feature globally.

The test importer could try to preserve these comments.
In some cases, this is not possible though.
Tests like like imported/w3c/web-platform-tests/fetch/api/basic/mode-same-origin.any.html are templates generated by the WPT server.
The only way would be to modify the WPT server and we would certainly not be able to upstream these changes.

An alternative is to apply DumpJSConsoleLogInStdErr to all WPT tests but we decided not to do that as it sometimes catches regressions.

Another alternative would be to have some ways to grab the test options outside of the test content itself, either a central file or a file close to each test. I don't think we reached consensus there.

[Fujii Hironori]

(In reply to youenn fablet from comment #6)
> Another alternative would be to have some ways to grab the test options
> outside of the test content itself, either a central file or a file close to
> each test. I don't think we reached consensus there.

This seems the best. 
How about the idea reusing tests-options.json for such purpose?
https://trac.webkit.org/browser/webkit/trunk/LayoutTests/tests-options.json?rev=241112
tests-options.json has been introduced in Bug 161601.

[Fujii Hironori]

I realize a tiny problem of DumpJSConsoleLogInStdErr that it outputs into stderr even though it is not a error message.
Windows port has "EDITING DELEGATE:" filter in do_text_results_differ.
https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/port/win.py?rev=241112#L108
We can have "CONSOLE:" filter for the purpose without modifying DRT and WTR.
This approach wouldn't pollute the content of -stderr.txt.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #7)
> How about the idea reusing tests-options.json for such purpose?

This is not easy because magic comments are retrieved by DRT and WTR, not by webkit-test-runner.
DRT and WTR need to be changed to parse tests-options.json.

It is easier to use a option file for each test, which is placed close to the test.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #8)
> I realize a tiny problem of DumpJSConsoleLogInStdErr that it outputs into
> stderr even though it is not a error message.

Darin mentioned this in Bug 161310 Comment 15.
I didn't read the lengthy discussion. I should have read it first.

[Fujii Hironori]

Bug 177027 has introduced dumpJSConsoleLogInStdErr test option.

> webkit-test-runner [ dumpJSConsoleLogInStdErr=true ]

I think we should use this for non-imported tests instead of using DumpJSConsoleLogInStdErr expectation in TestExpectations.

[Fujii Hironori]

Created attachment 361901 [details]
Patch

[Fujii Hironori]

Comment on attachment 361901 [details]
Patch

Oops, wrong patch.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #11)
> I think we should use this for non-imported tests instead of using
> DumpJSConsoleLogInStdErr expectation in TestExpectations.

Filed Bug 194586.

[youenn fablet]

> Tests like like imported/w3c/web-platform-tests/fetch/api/basic/mode-same-origin.any.html are templates generated by the WPT server.

If we decide to continue with these templated .any.html, we could actually modify them to include <!-- webkit-test-runner --> comments, as I guess the test runner is reading the file and not the HTTP resource.
We would then need to instruct the importer to preserve any of these comments.

[Fujii Hironori]

a previous discussion:

[webkit-dev] WPT tests and runtime-enabled features
https://lists.webkit.org/pipermail/webkit-dev/2017-June/029165.html

[Fujii Hironori]

(In reply to youenn fablet from comment #15)
> > Tests like like imported/w3c/web-platform-tests/fetch/api/basic/mode-same-origin.any.html are templates generated by the WPT server.
> 
> If we decide to continue with these templated .any.html, we could actually
> modify them to include <!-- webkit-test-runner --> comments, as I guess the
> test runner is reading the file and not the HTTP resource.
> We would then need to instruct the importer to preserve any of these
> comments.

youenn, do you prefer this approach? I think it is easier to use a option file for each test, which is placed close to the test, as you mentionsed in comment 6.
E.g. imported/w3c/web-platform-tests/fetch/api/basic/mode-same-origin.any-option.txt

[Fujii Hironori]

As Youenn mentioned in Bug 204793 Comment 5, there is no consensus on removing all DumpJSConsoleLogInStdErr expectation at the moment yet.
I'm going to fix the original issue of formatting DumpJSConsoleLogInStdErr (comment 0).

[Fujii Hironori]

Created attachment 385333 [details]
Patch

[Fujii Hironori]

Youenn, can I ask you review?

[Fujii Hironori]

Thank you!
I will land it unless anyone objects.

[Fujii Hironori]

Comment on attachment 385333 [details]
Patch

Clearing flags on attachment: 385333

Committed r253459: <https://trac.webkit.org/changeset/253459>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57898667>