Bug 189219 – run-webkit-tests prints confusing messages when test expectations list results that are not compatible with the run options

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

run-webkit-tests prints confusing messages when test expectations list results that are not compatible with the run options

[Simon Fraser (smfr)]

Created attachment 348694 [details]
Patch

[EWS Watchlist]

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.

[Simon Fraser (smfr)]

Created attachment 348894 [details]
Patch

[Simon Fraser (smfr)]

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

[Jonathan Bedard]

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.

[Jonathan Bedard]

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

[Alexey Proskuryakov]

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

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

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

[Alexey Proskuryakov]

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

[Simon Fraser (smfr)]

The logging output being discussed is actually the subject of bug 189293.

[Alexey Proskuryakov]

What parts of the sample output from comment 4 are subject of this bug?

[Simon Fraser (smfr)]

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.

[Alexey Proskuryakov]

That sounds like an indisputable improvement.

[Jonathan Bedard]

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.

[Simon Fraser (smfr)]

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

[Jon Lee]

Comment on attachment 348894 [details]
Patch

rs=me

[WebKit Commit Bot]

Comment on attachment 348894 [details]
Patch

Clearing flags on attachment: 348894

Committed r235764: <https://trac.webkit.org/changeset/235764>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44202453>