run-webkit-tests prints confusing messages when test expectations list results that are not compatible with the run options
Created attachment 348694 [details] Patch
Attachment 348694 [details] did not pass style-queue: ERROR: Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:187: multiple statements on one line (semicolon) [pep8/E702] [5] ERROR: Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:206: trailing whitespace [pep8/W291] [5] ERROR: Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:208: multiple statements on one line (semicolon) [pep8/E702] [5] ERROR: Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:211: trailing whitespace [pep8/W291] [5] ERROR: Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:615: at least two spaces before inline comment [pep8/E261] [5] ERROR: Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:982: at least two spaces before inline comment [pep8/E261] [5] ERROR: Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:988: trailing whitespace [pep8/W291] [5] Total errors found: 7 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348894 [details] Patch
Some sample output: fast/borders/border-image-fill-no-border.html -> ref test hashes didn't match but diff passed [786/11865] fast/animation/request-animation-frame-unparented-iframe-crash.html passed unexpectedly [1493/11865] fast/canvas/canvas-blending-fill-style.html passed unexpectedly fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect-altered.html -> test was a pass (expected) but is now a leak (unexpected) fast/canvas/canvas-ellipse-connecting-line.html -> ref test hashes didn't match but diff passed [3799/11865] fast/encoding/mailto-always-utf-8.html passed unexpectedly [4038/11865] fast/forms/mailto/formenctype-attribute-button-html.html passed unexpectedly [4050/11865] fast/forms/mailto/formenctype-attribute-input-2.html passed unexpectedly [4054/11865] fast/forms/mailto/formenctype-attribute-input-html.html passed unexpectedly [4063/11865] fast/forms/append-children-during-form-submission.html passed unexpectedly fast/forms/mailto/formenctype-attribute-input-html.html -> test was a pass (unexpected) but is now a leak (expected) fast/forms/mailto/formenctype-attribute-input-2.html -> test was a pass (unexpected) but is now a leak (expected) fast/forms/mailto/formenctype-attribute-button-html.html -> test was a pass (unexpected) but is now a leak (expected) fast/encoding/mailto-always-utf-8.html -> test was a pass (unexpected) but is now a leak (expected) [4826/11865] fast/forms/empty-textarea-toggle-disabled.html passed unexpectedly fast/images/animated-gif-no-layout.html -> ref test hashes didn't match but diff passed [5266/11865] fast/history/page-cache-geolocation-active-watcher.html passed unexpectedly [5306/11865] fast/canvas/webgl/webgl2-getActiveUniforms.html passed unexpectedly [5387/11865] fast/hidpi/image-srcset-svg-canvas.html passed unexpectedly [5483/11865] fast/hidpi/video-controls-in-hidpi.html passed unexpectedly fast/hidpi/video-controls-in-hidpi.html -> test was a pass (unexpected) but is now a leak (expected) fast/history/page-cache-geolocation-active-watcher.html -> test was a pass (unexpected) but is now a leak (expected) [6092/11865] fast/images/drag-pdf-as-image.html passed unexpectedly fast/images/gif-loop-count.html -> ref test hashes didn't match but diff passed fast/images/hidpi-directly-composited-image-on-subpixel-position.html -> ref test hashes didn't match but diff passed [6976/11865] fast/loader/policy-delegate-action-hit-test-zoomed.html passed unexpectedly fast/images/pdf-as-image-crop-box.html -> ref test hashes didn't match but diff passed
Comment on attachment 348894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348894&action=review > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner_unittest.py:125 > + runner._options.world_leaks = False Is this True by default? > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:601 > + def get_expectations_or_pass(self, test): Is there ever a case where this isn't what we want? I would be in favor of either baking this into get_expectations or providing a 'default' argument to get_expectations. > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:982 > + def filtered_expectations_for_test(self, test, pixel_tests_are_enabled, world_leaks_are_enabled): Can we pass the options in instead so this would look like: def filtered_expectations_for_test(self, test, is_ref_test, options): .... That way, we don't need to unpack the options in 2 places. > Tools/Scripts/webkitpy/layout_tests/views/buildbot_results_unittest.py:92 > + def test_print_unexpected_results_with_options(self): I'm a little unclear what we're trying to test here. It looks like every change to pixel_tests and world_leaks results in the same DASHED_LINE output.
(In reply to Simon Fraser (smfr) from comment #4) > Some sample output: > >.... > fast/images/pdf-as-image-crop-box.html -> ref test hashes didn't match but > diff passed To be clear, this is the old output, right?
> fast/encoding/mailto-always-utf-8.html -> test was a pass (unexpected) but is now a leak (expected) I don't understand what this is saying (beyond "something tricky is going on, you may need to dig into details").
(In reply to Jonathan Bedard from comment #5) > Comment on attachment 348894 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348894&action=review > > > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner_unittest.py:125 > > + runner._options.world_leaks = False > > Is this True by default? It's currently False by default but should be True by default on some platforms in future. I want to set it to an explicit value here. > > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:601 > > + def get_expectations_or_pass(self, test): > > Is there ever a case where this isn't what we want? I would be in favor of > either baking this into get_expectations or providing a 'default' argument > to get_expectations. That would be the old function that still exists below. > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:982 > > + def filtered_expectations_for_test(self, test, pixel_tests_are_enabled, world_leaks_are_enabled): > > Can we pass the options in instead so this would look like: > > def filtered_expectations_for_test(self, test, is_ref_test, options): > .... > > That way, we don't need to unpack the options in 2 places. Sure. > > Tools/Scripts/webkitpy/layout_tests/views/buildbot_results_unittest.py:92 > > + def test_print_unexpected_results_with_options(self): > > I'm a little unclear what we're trying to test here. It looks like every > change to pixel_tests and world_leaks results in the same DASHED_LINE output. Right, it's testing that if you run without --world-leaks, but some tests have expected [ Leak ], those are NOT reported as unexpected failures (i.e. the output is blank other than the dashed line). Similarly, if a non-ref test is [ ImageOnlyFailure ], and you run without --pixel-tests, we should not report that test as an unexpected pass.
(In reply to Jonathan Bedard from comment #6) > (In reply to Simon Fraser (smfr) from comment #4) > > Some sample output: > > > >.... > > fast/images/pdf-as-image-crop-box.html -> ref test hashes didn't match but > > diff passed > > To be clear, this is the old output, right? I did not change this output.
(In reply to Alexey Proskuryakov from comment #7) > > fast/encoding/mailto-always-utf-8.html -> test was a pass (unexpected) but is now a leak (expected) > > I don't understand what this is saying (beyond "something tricky is going > on, you may need to dig into details"). This is trying to communicate something to the user that is more complex because leak detection is async: when the test first ran, it passed, but later on, leak detection found a leak and turn the test into a Leak failure. I welcome suggestions at better wording.
I guess the main confusion for me comes from "is now a leak (expected)", which sounds like there was some change with regards to leaking, but is actually trying to say that there was no change. How about one of these? fast/encoding/mailto-always-utf-8.html -> test was an unexpected pass but it caused an expected world leak fast/encoding/mailto-always-utf-8.html -> additional result: still caused a world leak fast/encoding/mailto-always-utf-8.html -> additional result: leaked the world expectedly
The logging output being discussed is actually the subject of bug 189293.
What parts of the sample output from comment 4 are subject of this bug?
Actually none! I pasted that output into the wrong bug! This bug deals with the message at the end, that says you have unexpected failures when you don't have any that can be detected by the current run options.
That sounds like an indisputable improvement.
My only change would be that filtered_expectations_for_test(...) accept options so that we don't have to parse the options in two places. Other than that, unofficial r=me.
(In reply to Jonathan Bedard from comment #16) > My only change would be that filtered_expectations_for_test(...) accept > options so that we don't have to parse the options in two places. Other than > that, unofficial r=me. test_expectations.py currently doesn't know anything about options, so I'm a bit reluctant to do this.
Comment on attachment 348894 [details] Patch rs=me
Comment on attachment 348894 [details] Patch Clearing flags on attachment: 348894 Committed r235764: <https://trac.webkit.org/changeset/235764>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44202453>