[webkit-reviews] review denied: [Bug 70484] [WK2] add flag to only check pixel results if png files exist : [Attachment 117175] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Fehér Zsolt
<feherzs at inf.u-szeged.hu>'s request for review:
Bug 70484: [WK2] add flag to only check pixel results if png files exist
https://bugs.webkit.org/show_bug.cgi?id=70484

Attachment 117175: Patch
https://bugs.webkit.org/attachment.cgi?id=117175&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
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.