[webkit-reviews] review denied: [Bug 60605] [NRWT] Reftests should run even when pixel tests are disabled. : [Attachment 115047] 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 115047: Patch
https://bugs.webkit.org/attachment.cgi?id=115047&action=review

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


(In reply to comment #48)
> >> 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?
> 
> Another way to write it would be "driver_output1.image_hash is not None and
driver_output2.image_hash is not None".  This was why it used to say PASS
previously, we were getting no image hashes but None == None, so the test was
passing.
> 

Got 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?
> 
> There were tests in base_unittests.py that passed in None for the port.  I
fixed the tests instead.
>

Great.
 
> >> 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.
> 
> cmd_line() is only used to spawn DRTs within a driver (e.g., WebKitDriver
uses it for that).  It's safe to change it here because DriverProxy never calls
cmd_line() directly.  AFAICT, the API is only for logging.
> 

Hmm. I'm not quite sure I follow you, but I'll take your word for it.

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

Thinking about this further ... can we rearrange this slightly so that the
Driver doesn't need to know about self._port.get_option('pixel_tests')? I.e.,
since Drivers are now only constructed by Port.create_driver(), just modify
that code to know whether this driver needs pixel_tests (and then we can just
call this parameter pixel_tests).

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:114
> +	       self._reftest_driver = driver_instance_constructor(port,
worker_number, force_pixel_results=True)

See comments above ... I think we can restructure this so that
force_pixel_tests can be renamed pixel_tests and we can explicitly tell the
constructor whether we want pixel tests or not.