[Webkit-unassigned] [Bug 72841] NRWT: option --skip-pixel-test-if-no-baseline support on DRT

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72841


Dirk Pranke <dpranke at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #134242|review?, commit-queue?      |review+, commit-queue-
               Flag|                            |




--- Comment #24 from Dirk Pranke <dpranke at chromium.org>  2012-03-28 12:24:26 PST ---
(From update of attachment 134242)
View in context: https://bugs.webkit.org/attachment.cgi?id=134242&action=review

>> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:149
>> +                test_input.should_run_pixel_test = False
> 
> This could be:
> 
> if self._port.get_option('pixel_tests') and self._port.get_option('skip_pixel_test_if_no_baseline'):
>     test_input.should_run_pixel_test = (self._port.expected_image(test_input.test_name) != None)
> else:
>      test_input.should_run_pixel_test = self._port.get_option('pixel_tests')

I'm not sure that this is clearer; the logic is kinda convoluted either way.

>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:41
>> +        self.run_pixel_test = run_pixel_test
> 
> Nit: I like should_run_pixel_test (sounds like a bool) better than run_pixel_test (sounds like an action or function).

works for me.

>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:227
>> +        self._driver.start(True, [])
> 
> It's not obvious to me why we pass True here? Does this mean perftestrunner will have pixel dumping enabled (is that the old behavior)? Maybe it's OK because DriverProxy.start will be removed soon?

The value is overridden in practice by the run_test() call, but picking the right default might save us from starting a non-pixel-test DRT and then having to start a second one down the road (or vice versa). This should probably either be self._port.get_option('pixel_tests') or stay as self._pixel_tests (and then keep line 184 as well.

Tony's other nits seem fine to me. I'll mark this R+, CQ-; you can upload another patch w/ the nits addressed and then anyone should be able to CQ+ it.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.