[webkit-reviews] review denied: [Bug 60605] [NRWT] Reftests should run even when pixel tests are disabled. : [Attachment 114793] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Tony Chang <tony at chromium.org>'s
request for review:
Bug 60605: [NRWT] Reftests should run even when pixel tests are disabled.
https://bugs.webkit.org/show_bug.cgi?id=60605

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

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114793&action=review


> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:130
> +	       return self._run_reftest()

Can you restructure this function so that we test for reftests first, and use
the same dummy/skip logic for both the case where we're rebasing or we're
skipping the reftests? I think that can eliminate at least one branch and make
the logic a little clearer.

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:311
> +	   assert(driver_output1.image_hash or driver_output2.image_hash)

Why is this an OR and not an AND? I guess I'm not sure why you're allowed to
assert anything here if it isn't an AND?

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:257
> +	   return RefTestDriver(self, worker_number, ChromiumDriver)

I think it might make more sense to make the "driver_instance_constructor"
argument a separate property of the Port object, and then pull create_driver()
up into base.Port, since the rest of the code is the same. Then you can
insulate the port subclasses from the existence of the RefTestDriver (or
whatever we end up calling it).

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:75
> +	   self._pixel_tests = force_pixel_results or (self._port and
self._port.get_option('pixel_tests'))

Nit: I think port should always be non-NULL, so you probably don't need that
check?

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:104
> +class RefTestDriver:

Nit: subclass from object.

I think this design makes sense, but I'm not wild about the class name; it
makes me think that this driver is geared specifically to ref tests, as opposed
to a delegating proxy that picks the right driver depending on the test.

Maybe rename this to something like "DriverProxy" (not that that is a great
name either) and add a comment as to why it's necessary?

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:108
> +	       self._pixel_driver = self._driver

I might rename this _reftest_driver instead of _pixel_driver, since the code is
written to only use this field if the test is a reftest.

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:127
> +	       cmd_line += ['; '] + self._pixel_driver.cmd_line()

This doesn't seem right (or perhaps safe) ...  cmd_line() is used for logging
but I think it is also to actually spawn the DRTs (e.g., when it's passed to
ServerProcess in the webkit ports, and when MockDRT uses it). We might have to
return a list of cmd lines or something instead.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:444
> +	   Driver.__init__(self, port, worker_number, force_pixel_results)

It would be good to add a comment or a docstring explaining what
force_pixel_results is for.