Bug 70484 - [WK2] add flag to only check pixel results if png files exist
Summary: [WK2] add flag to only check pixel results if png files exist
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 72841
  Show dependency treegraph
 
Reported: 2011-10-20 02:09 PDT by Fehér Zsolt
Modified: 2012-02-08 12:00 PST (History)
12 users (show)

See Also:


Attachments
Patch (7.18 KB, patch)
2011-10-20 02:10 PDT, Fehér Zsolt
no flags Details | Formatted Diff | Diff
Patch (8.24 KB, patch)
2011-10-20 09:15 PDT, Fehér Zsolt
ojan: review-
ojan: commit-queue-
Details | Formatted Diff | Diff
Diff (4.00 KB, text/plain)
2011-10-26 10:13 PDT, Fehér Zsolt
no flags Details
Patch (4.14 KB, patch)
2011-10-27 01:21 PDT, Fehér Zsolt
no flags Details | Formatted Diff | Diff
Patch (4.17 KB, patch)
2011-10-28 00:43 PDT, Fehér Zsolt
tony: review-
tony: commit-queue-
Details | Formatted Diff | Diff
Patch (10.67 KB, patch)
2011-11-20 18:22 PST, Fehér Zsolt
dpranke: review-
Details | Formatted Diff | Diff
Patch (10.70 KB, patch)
2011-11-30 05:20 PST, Fehér Zsolt
dpranke: review-
dpranke: commit-queue-
Details | Formatted Diff | Diff
Patch (5.77 KB, patch)
2012-01-24 05:29 PST, Fehér Zsolt
no flags Details | Formatted Diff | Diff
Patch (5.95 KB, patch)
2012-01-29 19:24 PST, Fehér Zsolt
no flags Details | Formatted Diff | Diff
Patch (7.18 KB, patch)
2012-01-30 01:30 PST, Fehér Zsolt
no flags Details | Formatted Diff | Diff
Patch (11.22 KB, patch)
2012-02-08 08:59 PST, Fehér Zsolt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fehér Zsolt 2011-10-20 02:09:06 PDT
Turn on the pixel test on WK2
Comment 1 Fehér Zsolt 2011-10-20 02:10:13 PDT
Created attachment 111744 [details]
Patch
Comment 2 Fehér Zsolt 2011-10-20 09:15:35 PDT
Created attachment 111789 [details]
Patch
Comment 3 Balazs Kelemen 2011-10-20 09:28:13 PDT
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.
Comment 4 Ojan Vafai 2011-10-20 09:30:25 PDT
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.
Comment 5 Fehér Zsolt 2011-10-26 10:13:32 PDT
Created attachment 112558 [details]
Diff
Comment 6 Fehér Zsolt 2011-10-26 10:16:51 PDT
(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?
Comment 7 Fehér Zsolt 2011-10-27 01:21:17 PDT
Created attachment 112648 [details]
Patch
Comment 8 Zoltan Horvath 2011-10-27 05:51:22 PDT
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
Comment 9 Fehér Zsolt 2011-10-28 00:43:49 PDT
Created attachment 112825 [details]
Patch
Comment 10 Zoltan Horvath 2011-11-04 00:47:49 PDT
Ping for review.
Comment 11 Csaba Osztrogonác 2011-11-04 00:55:10 PDT
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.
Comment 12 Tony Chang 2011-11-04 10:12:49 PDT
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 (...)
Comment 13 Dirk Pranke 2011-11-04 17:41:22 PDT
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?
Comment 14 Fehér Zsolt 2011-11-07 07:02:32 PST
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.
Comment 15 Fehér Zsolt 2011-11-07 07:06:38 PST
Comment on attachment 112825 [details]
Patch

I can make the fix, based on your comments and i write some unit test to this patch.
Comment 16 Fehér Zsolt 2011-11-20 18:22:50 PST
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?
Comment 17 WebKit Review Bot 2011-11-20 18:25:29 PST
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.
Comment 18 Dirk Pranke 2011-11-21 12:39:23 PST
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.
Comment 19 Dirk Pranke 2011-11-21 12:39:55 PST
(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?
Comment 20 Fehér Zsolt 2011-11-22 04:41:09 PST
(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)
Comment 21 Dirk Pranke 2011-11-28 15:26:10 PST
(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)
Comment 22 Fehér Zsolt 2011-11-30 05:19:59 PST
(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)).
Comment 23 Fehér Zsolt 2011-11-30 05:20:31 PST
Created attachment 117175 [details]
Patch

I uploaded the new patch with unittest and i made the fixes, what D. Pranke ask!
Comment 24 Dirk Pranke 2011-11-30 12:36:45 PST
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.
Comment 25 Fehér Zsolt 2011-12-12 06:02:06 PST
(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
Comment 26 Fehér Zsolt 2012-01-24 05:29:07 PST
Created attachment 123717 [details]
Patch

Sorry for the delay, i was on leave.
I corrected my patch
I hope, it meets now :)
Comment 27 WebKit Review Bot 2012-01-24 07:01:26 PST
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
Comment 28 Balazs Kelemen 2012-01-24 09:27:25 PST
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.
Comment 29 Fehér Zsolt 2012-01-29 19:24:43 PST
Created attachment 124486 [details]
Patch
Comment 30 Fehér Zsolt 2012-01-30 01:30:59 PST
Created attachment 124508 [details]
Patch
Comment 31 Balazs Kelemen 2012-01-30 01:52:27 PST
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.
Comment 32 Balazs Kelemen 2012-01-30 02:07:02 PST
Comment on attachment 123717 [details]
Patch

Landed in http://trac.webkit.org/changeset/106222
Comment 33 Balazs Kelemen 2012-01-30 02:27:33 PST
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.
Comment 34 Balazs Kelemen 2012-01-30 02:30:40 PST
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?
Comment 35 Csaba Osztrogonác 2012-01-30 02:40:44 PST
Rollout landed in http://trac.webkit.org/changeset/106224
Comment 37 Fehér Zsolt 2012-01-30 06:05:28 PST
(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
Comment 38 Fehér Zsolt 2012-02-08 08:59:18 PST
Created attachment 126093 [details]
Patch

I did the repairs.
Comment 39 WebKit Review Bot 2012-02-08 11:59:54 PST
Comment on attachment 126093 [details]
Patch

Clearing flags on attachment: 126093

Committed r107113: <http://trac.webkit.org/changeset/107113>
Comment 40 WebKit Review Bot 2012-02-08 12:00:04 PST
All reviewed patches have been landed.  Closing bug.