Bug 161693

Created attachment 288151 [details] Patch

(In reply to comment #1) > Created attachment 288151 [details] > Patch gtk error is unrelated to this patch (bot was unable to process it until a change caused the patch to not be applicable). win error was due to WASM missing file/config apparently. Putting patch as r?

Comment on attachment 288151 [details] Patch Is this really an improvement? Will these tests have dynamic messages if we were to make them pass?

> Is this really an improvement? Will these tests have dynamic messages if we > were to make them pass? It is not needed if we can make them pass. This option should only be used transitorily. For some of the tests mentioned, the situation is like this for months and make all tests belonging to the same file unusable.

Comment on attachment 288151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288151&action=review I’m not going to say review+ because I am not sure this is the best solution. The cost of this is that when a test does fail, we won’t be able to see which assertion failed. And if we turn the real failures back on then we will get failures because of expected files that expect the failure log to be disabled. So is that cost worth the benefit of less flakiness? Is there some other way of achieving that goal? Maybe we can adjust things so that the code that compares the results with expected results can understand what may vary? > Tools/DumpRenderTree/DumpRenderTree.h:69 > + bool shouldLogTestharnessFailures { true }; The word "harness" should have a capital H here. > Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:80 > + } else if (arg == std::string("--no-testharness-failure-log")) I’m surprised that the creation of a std::string is needed here just to compare with a string literal. I don’t think it really is needed. I suggest adding a "-" before the word "harness", since "testharness" is not a word. > Tools/DumpRenderTree/TestRunner.cpp:1778 > + TestRunner* controller = static_cast<TestRunner*>(JSObjectGetPrivate(thisObject)); I suggest auto here instead of repeating the type name twice. > Tools/DumpRenderTree/TestRunner.h:375 > + void setShouldLogTestharnessFailures(bool inStderr) { m_shouldLogTestHarnessFailures = inStderr; } > + bool shouldLogTestharnessFailures() const { return m_shouldLogTestHarnessFailures; } The word "harness" should have a capital H here. > Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:36 > + readonly attribute boolean shouldLogTestharnessFailures; The word "harness" should have a capital H here. > Tools/WebKitTestRunner/TestController.cpp:1077 > + } else if (arg == std::string("--no-testharness-failure-log")) I’m surprised that the creation of a std::string is needed here just to compare with a string literal. I don’t think it really is needed. I suggest adding a "-" before the word "harness", since "testharness" is not a word.

(In reply to comment #5) > Comment on attachment 288151 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=288151&action=review > > I’m not going to say review+ because I am not sure this is the best solution. Thanks for the review. Chris, Alex, since you are also working with WPT, do you have any opinion on whether you would need/have needed this feature? I still think this has value even if I no longer need this for fetch API.

Comment on attachment 288151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288151&action=review r=me with comments. For the record, I do think the best solution would be to avoid such flaky asserts upstream in web-platform-tests. However, based on feedback, I think this is unlikely to happen. As a result, I think this patch is the second best option and we can do this easily and quickly. This is MUCH better than skipping tests. >> Tools/DumpRenderTree/DumpRenderTree.h:69 >> + bool shouldLogTestharnessFailures { true }; > > The word "harness" should have a capital H here. Agreed >> Tools/DumpRenderTree/DumpRenderTreeCommon.cpp:80 >> + } else if (arg == std::string("--no-testharness-failure-log")) > > I’m surprised that the creation of a std::string is needed here just to compare with a string literal. I don’t think it really is needed. > > I suggest adding a "-" before the word "harness", since "testharness" is not a word. Agreed. >> Tools/DumpRenderTree/TestRunner.cpp:1778 >> + TestRunner* controller = static_cast<TestRunner*>(JSObjectGetPrivate(thisObject)); > > I suggest auto here instead of repeating the type name twice. auto* or on one line below. > Tools/DumpRenderTree/TestRunner.h:374 > + void setShouldLogTestharnessFailures(bool inStderr) { m_shouldLogTestHarnessFailures = inStderr; } Wrong naming: inStderr >> Tools/DumpRenderTree/TestRunner.h:375 >> + bool shouldLogTestharnessFailures() const { return m_shouldLogTestHarnessFailures; } > > The word "harness" should have a capital H here. Agreed. > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:133 > + options=[option for option in self._tests_options.get(test_file, []) if option.startswith("--")]) Will need changing if you take into consideration my comment below. >> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:36 >> + readonly attribute boolean shouldLogTestharnessFailures; > > The word "harness" should have a capital H here. Agreed. > Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:75 > + void setShouldLogTestharnessFailures(bool inStderr) { m_shouldLogTestHarnessFailures = inStderr; } Wrong naming: inStderr >> Tools/WebKitTestRunner/TestController.cpp:1077 >> + } else if (arg == std::string("--no-testharness-failure-log")) > > I’m surprised that the creation of a std::string is needed here just to compare with a string literal. I don’t think it really is needed. > > I suggest adding a "-" before the word "harness", since "testharness" is not a word. I agree with Darin that the std::string() is likely not needed. > Tools/WebKitTestRunner/TestInvocation.h:103 > + bool m_shouldLogTestHarnessFailures { false }; Sounds to me like the default should be true. > LayoutTests/resources/testharnessreport.js:71 > + message = " (Assertion failure log disabled)" I think we should just not print anything in this case but I am OK with this if other people think it is useful. Printing "(Assertion failure log disabled)" make the output look ugly to me. > LayoutTests/tests-options.json:2 > "imported/w3c/web-platform-tests/XMLHttpRequest/progress-events-response-data-gzip.htm": [ I think this file should be specific to web-platform-tests and should be moved to LayoutTest/imported/w3c/web-platform-tests or maybe LayoutTest/imported/w3c/ if we need it for w3c csswg tests. > LayoutTests/tests-options.json:6 > + "--no-testharness-failure-log" I think this should be a more simple keyword since it is typed by humans and also for consistency with "slow". Maybe "hideFailureMessages" or similar.

(In reply to comment #6) > Chris, Alex, since you are also working with WPT, do you have any opinion on > whether you would need/have needed this feature? I think ideally we wouldn't need this because all the tests would always have deterministic output. Since that isn't the case, we need something like this. We should bring it up when we discuss this in a few weeks.

https://crbug.com/679742 is the equivalent bug on Chromium, so at least we're not a total outlier here.

<rdar://problem/105463943>