Bug 70484 – [WK2] add flag to only check pixel results if png files exist

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Fehér Zsolt]

Turn on the pixel test on WK2

[Fehér Zsolt]

Created attachment 111744 [details]
Patch

[Fehér Zsolt]

Created attachment 111789 [details]
Patch

[Balazs Kelemen]

Comment on attachment 111744 [details]
Patch

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

In general the concept is good but I'm not happy with the semantics of the new switch (wk2_pixel_test).

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:100
> +        if self._port.driver_name() == "WebKitTestRunner" and self._port.get_option('wk2_pixel_test'):
> +            return self._should_run_pixel_test

1. wk2_pixel_test does not express the concept. I would use a separate switch, something like --skip-pixel-test-if-no-baseline. It would be valid only with -p (alias --pixel-tests), so it should be ignored with a warning otherwise
2. it's ok to only support it with WTR, but in that case we should die with error message if it is used with DRT

> Tools/WebKitTestRunner/TestInvocation.cpp:118
> +void TestInvocation::setIsWK2PixelTest(const std::string& expectedPixelHash)
> +{
> +    if (!expectedPixelHash.length())
> +        return;
> +    m_dumpPixels = true;
> +    m_expectedPixelHash = expectedPixelHash;
> +}

According to 1, you could do it in setIsPixelTest. You should still have an extra bool if you want to check that there is no empty hash when running without the --skip... switch, but I don't thing it is really necessary.

[Ojan Vafai]

Comment on attachment 111789 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:289
> +        optparse.make_option("--wk2-pixel-test", action="store_true",
> +            dest="wk2_pixel_test", help="Enable pixel-to-pixel PNG comparisons on WebKit2"),

Why not just reuse --pixel? You already know if you're webkit2 from the driver_name. Seems like you could simplify this patch a lot if you did so.

If there's a reason you need a new flag, you should explain it in the changelog.

[Fehér Zsolt]

Created attachment 112558 [details]
Diff

[Fehér Zsolt]

(In reply to comment #4)
> (From update of attachment 111789 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111789&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:289
> > +        optparse.make_option("--wk2-pixel-test", action="store_true",
> > +            dest="wk2_pixel_test", help="Enable pixel-to-pixel PNG comparisons on WebKit2"),
> 
> Why not just reuse --pixel? You already know if you're webkit2 from the driver_name. Seems like you could simplify this patch a lot if you did so.
> 
> If there's a reason you need a new flag, you should explain it in the changelog.

The new flag have a speaking name now. What do you think, that would be good?

[Fehér Zsolt]

Created attachment 112648 [details]
Patch

[Zoltan Horvath]

Comment on attachment 112648 [details]
Patch

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

If you don't set r? on the patch consider that it won't be reviewed!

> Tools/ChangeLog:9
> +        we will run pixel-tests in WebKit2, when the *-expected.png file is

you don't need 'is', you need 'for' rather than 'to' => 
when the *-expected.png file exists for the test.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:87
> +            _log.error("--skip-pixel-test-if-no-baseline is only supperted in WebKitTestRunner")

supported rather than supperted

[Fehér Zsolt]

Created attachment 112825 [details]
Patch

[Zoltan Horvath]

Ping for review.

[Csaba Osztrogonác]

I cc-ed NRWT experts, Tony and Dirk.

I think we should change the name of the bug, because the patch and the bug title is different. The proposed patch isn't enable pixel tests for Qt-WK2 platform, but make it possible to run pixel tests if there aren't checked in png results for all tests.

[Tony Chang]

Comment on attachment 112825 [details]
Patch

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

Can you write some python unit tests for this flag?

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:113
> +        if self._port.driver_name() == "WebKitTestRunner" and self._port.get_option('skip_pixel_test_if_no_baseline') and self._port.get_option('pixel_tests'):

Can you use self._options.skip_pixel_test_if_no_baseline and self._options.pixel_tests instead?

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:86
> +        if options.skip_pixel_test_if_no_baseline and not (port.driver_name() == "WebKitTestRunner"):

Nit: != seems a little clearer than not (...)

[Dirk Pranke]

Comment on attachment 112825 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:113
>> +        if self._port.driver_name() == "WebKitTestRunner" and self._port.get_option('skip_pixel_test_if_no_baseline') and self._port.get_option('pixel_tests'):
> 
> Can you use self._options.skip_pixel_test_if_no_baseline and self._options.pixel_tests instead?

I think self._options is shared between this object and the enclosing worker object, meaning if you set this to False here, all subsequent tests run in that worker will also not use pixel tests. I'm guessing you don't want that.

>> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:86
>> +        if options.skip_pixel_test_if_no_baseline and not (port.driver_name() == "WebKitTestRunner"):
> 
> Nit: != seems a little clearer than not (...)

Why is this change specific to WebKitTestRunner ? Why wouldn't we want to support this in DRT as well?

[Fehér Zsolt]

Comment on attachment 112825 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:86
>>> +        if options.skip_pixel_test_if_no_baseline and not (port.driver_name() == "WebKitTestRunner"):
>> 
>> Nit: != seems a little clearer than not (...)
> 
> Why is this change specific to WebKitTestRunner ? Why wouldn't we want to support this in DRT as well?

Because we need now this patch on WK2, but an another patch we can supported in DRT also.

[Fehér Zsolt]

Comment on attachment 112825 [details]
Patch

I can make the fix, based on your comments and i write some unit test to this patch.

[Fehér Zsolt]

Created attachment 116019 [details]
Patch

The unittest, don't work for me, i think it is, becasue of a bug in nrwt.
Any idea why don't work?

[WebKit Review Bot]

Attachment 116019 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/Scripts/webkitpy/layout_tests/contro..." exit_code: 1

Tools/Scripts/webkitpy/layout_tests/controllers/skip_pixel_test_if_no_baseline.py:25:  expected 2 blank lines, found 1  [pep8/E302] [5]
Tools/Scripts/webkitpy/layout_tests/models/test_input.py:55:  no newline at end of file  [pep8/W292] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Pranke]

Comment on attachment 116019 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:113
> +                if skip_pixel_test_if_no_baseline.skip_pixel_test(self._port, test_input.test_name):

Can't you just replace this with:

test_input.should_run_pixel_test = (port.expected_image(test_name) == None)

and delete skip_pixel_test_if_no_baseline.py ?

Also, I still don't understand why you are limiting this flag to WebKitTestRunner. It doesn't seem like anything will break if you run this on other ports?

> Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:159
> +            if skip_pixel_test_if_no_baseline.skip_pixel_test(port, test_name):

I suggest you just inline the function here as well.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:69
> +            return -1

As I suggested above, I don't think this check is necessary.

[Dirk Pranke]

(In reply to comment #16)
> Created an attachment (id=116019) [details]
> Patch
> 
> The unittest, don't work for me, i think it is, becasue of a bug in nrwt.
> Any idea why don't work?

Can you provide more information on why it isn't working? What is happening?

[Fehér Zsolt]

(In reply to comment #19)
> (In reply to comment #16)
> > Created an attachment (id=116019) [details] [details]
> > Patch
> > 
> > The unittest, don't work for me, i think it is, becasue of a bug in nrwt.
> > Any idea why don't work?
> 
> Can you provide more information on why it isn't working? What is happening?

I think one of the method, which call this function work wrong. The unittest system throw this message:

FAIL: test_skip_pixel_test_if_no_baseline_option (webkitpy.layout_tests.port.webkit_unittest.WebKitPortTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/feherzsolt/webkit/Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py", line 162, in test_skip_pixel_test_if_no_baseline_option
    self.assertTrue(test_name in self.png_list)
AssertionError: False is not true

----------------------------------------------------------------------
Ran 1237 tests in 22.667s

FAILED (failures=1)

[Dirk Pranke]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #16)
> > > Created an attachment (id=116019) [details] [details] [details]
> > > Patch
> > > 
> > > The unittest, don't work for me, i think it is, becasue of a bug in nrwt.
> > > Any idea why don't work?
> > 
> > Can you provide more information on why it isn't working? What is happening?
> 
> I think one of the method, which call this function work wrong. The unittest system throw this message:
> 
> FAIL: test_skip_pixel_test_if_no_baseline_option (webkitpy.layout_tests.port.webkit_unittest.WebKitPortTest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/home/feherzsolt/webkit/Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py", line 162, in test_skip_pixel_test_if_no_baseline_option
>     self.assertTrue(test_name in self.png_list)
> AssertionError: False is not true
> 

This is telling you that skip_pixel_test returned False, i.e., that one of the test files did have an expected image, but that that test wasn't in  your png_list. You'll have to add some debug statements to figure out which test it is, and what port.expected_image() is returning (and why).

Also, we try not to hard-code in real test names, and the TestWebKitPort is probably using a MockHost, which hopefully means a mock filesystem with a fake list of tests (set in webkitpy/layout_tests/port/test.py). If that is true, you should use some of the fake test names instead of the real test names you're using.


> ----------------------------------------------------------------------
> Ran 1237 tests in 22.667s
> 
> FAILED (failures=1)

[Fehér Zsolt]

(In reply to comment #18)
> (From update of attachment 116019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116019&action=review
> 
> 
> Also, I still don't understand why you are limiting this flag to WebKitTestRunner. It doesn't seem like anything will break if you run this on other ports?
> 

Because we need now this patch on WK2, but an another patch we can supported in DRT also (In Bug: 72841(ID)).

[Fehér Zsolt]

Created attachment 117175 [details]
Patch

I uploaded the new patch with unittest and i made the fixes, what D. Pranke ask!

[Dirk Pranke]

Comment on attachment 117175 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:203
> +        self.assertEquals(png_list, self.png_list)

Okay, inlining the function made it clear that we weren't actually testing anything interesting by testing just that skip_pixel_test() returns the correct value. Further, writing tests in webkit_unittest that are so heavily dependent on test.py indicated that there was something wrong. I'd just delete this whole routine.

What we need to test is that the actual change in worker.py is working correctly. The best way to do that is to is to add another test to run_webkit_tests_integrationtest.py with the correct flags. See something like test_repeat_each() for an example of how to check that we only ran the tests you expected to run.

Of course, the integration tests all run using the TestPort, not the WebKitPort, and so they won't be able to check the restriction on this only working with WebKitTestRunner. We can either get rid of the generic restriction on this flag (in line 67 of run_webkit_test.py), or we can add a test-wk2 port implementation to port/test.py and override port.driver_name() accordingly.

I would probably vote for just removing the check in run_webkit_tests.py; I think you're being overly cautious since the command line flag is kind of obscure, you're probably the only one to use it, and you're planning to add this to DRT as well, but if you would rather be safe than sorry, adding the test-wk2 flag is fine as well.

Sorry for the back-and-forth on this :(.

The patch looks fine otherwise.

[Fehér Zsolt]

(In reply to comment #24)
> (From update of attachment 117175 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117175&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:203
> > +        self.assertEquals(png_list, self.png_list)
> 
> Okay, inlining the function made it clear that we weren't actually testing anything interesting by testing just that skip_pixel_test() returns the correct value. Further, writing tests in webkit_unittest that are so heavily dependent on test.py indicated that there was something wrong. I'd just delete this whole routine.
> 
> What we need to test is that the actual change in worker.py is working correctly. The best way to do that is to is to add another test to run_webkit_tests_integrationtest.py with the correct flags. See something like test_repeat_each() for an example of how to check that we only ran the tests you expected to run.
> 
> Of course, the integration tests all run using the TestPort, not the WebKitPort, and so they won't be able to check the restriction on this only working with WebKitTestRunner. We can either get rid of the generic restriction on this flag (in line 67 of run_webkit_test.py), or we can add a test-wk2 port implementation to port/test.py and override port.driver_name() accordingly.
> 
> I would probably vote for just removing the check in run_webkit_tests.py; I think you're being overly cautious since the command line flag is kind of obscure, you're probably the only one to use it, and you're planning to add this to DRT as well, but if you would rather be safe than sorry, adding the test-wk2 flag is fine as well.
> 
> Sorry for the back-and-forth on this :(.
> 
> The patch looks fine otherwise.

OK, i working on it now

[Fehér Zsolt]

Created attachment 123717 [details]
Patch

Sorry for the delay, i was on leave.
I corrected my patch
I hope, it meets now :)

[WebKit Review Bot]

Comment on attachment 123717 [details]
Patch

Attachment 123717 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11212813

New failing tests:
media/audio-garbage-collect.html

[Balazs Kelemen]

Comment on attachment 123717 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:112
> +            if self._port.driver_name() == "WebKitTestRunner" and self._port.get_option('skip_pixel_test_if_no_baseline') and self._port.get_option('pixel_tests'):
> +                test_input.should_run_pixel_test = (self._port.expected_image(test_input.test_name) != None)

I think a #FIXME comment telling that DRT should also support the option would be useful.

[Fehér Zsolt]

Created attachment 124486 [details]
Patch

[Fehér Zsolt]

Created attachment 124508 [details]
Patch

[Balazs Kelemen]

Err, I don't think we need one more iteration just because of the comment. I'm going to land the patch which has the r+ with the comment.

[Balazs Kelemen]

Comment on attachment 123717 [details]
Patch

Landed in http://trac.webkit.org/changeset/106222

[Balazs Kelemen]

Smg is definitely wrong with this: http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r106222%20(19365)/results.html. Ossy is going to rollout.

[Balazs Kelemen]

Comment on attachment 123717 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/models/test_input.py:41
> -    def __init__(self, test_name, timeout):
> +    def __init__(self, test_name, timeout, should_run_pixel_test=True):
>          """Holds the input parameters for a test.
>          Args:

I think this is the reason why it broke the bot. This force pixel tests always, isn't it?

[Csaba Osztrogonác]

Rollout landed in http://trac.webkit.org/changeset/106224

[Csaba Osztrogonác]

And an other thing is wrong. Check http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r106222%20%2819365%29/results.html

diff URL's don't work, because diff files aren't in the archive:
http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20WebKit2/builds/19365/steps/archive-test-results/logs/stdio

[Fehér Zsolt]

(In reply to comment #34)
> (From update of attachment 123717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123717&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_input.py:41
> > -    def __init__(self, test_name, timeout):
> > +    def __init__(self, test_name, timeout, should_run_pixel_test=True):
> >          """Holds the input parameters for a test.
> >          Args:
> 
> I think this is the reason why it broke the bot. This force pixel tests always, isn't it?

I don't think so, why i use this variable only, when the --skip-pixel-test-if-no-baseline option is enabled

[Fehér Zsolt]

Created attachment 126093 [details]
Patch

I did the repairs.

[WebKit Review Bot]

Comment on attachment 126093 [details]
Patch

Clearing flags on attachment: 126093

Committed r107113: <http://trac.webkit.org/changeset/107113>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.