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
Created attachment 361173 [details] Patch
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.
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.
> 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'.
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.
(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.
(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.
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.
(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.
(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.
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.
Created attachment 361901 [details] Patch
Comment on attachment 361901 [details] Patch Oops, wrong patch.
(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.
> 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.
a previous discussion: [webkit-dev] WPT tests and runtime-enabled features https://lists.webkit.org/pipermail/webkit-dev/2017-June/029165.html
(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
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).
Created attachment 385333 [details] Patch
Youenn, can I ask you review?
Thank you! I will land it unless anyone objects.
Comment on attachment 385333 [details] Patch Clearing flags on attachment: 385333 Committed r253459: <https://trac.webkit.org/changeset/253459>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57898667>