[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





--- Comment #23 from Tony Chang <tony at chromium.org>  2012-03-28 10:19:22 PST ---
(From update of attachment 134242)
View in context: https://bugs.webkit.org/attachment.cgi?id=134242&action=review

The changes to WTR seem fine. Just some style nits in the python code. I'll let Dirk give the final review.

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:99
> +        driver_input = DriverInput(self._test_name, self._timeout, image_hash, bool(self._reference_files))
> +        if not self._should_run_pixel_test:
> +            driver_input.run_pixel_test = False
> +        else:
> +            driver_input.run_pixel_test = True

Can we pass "not self._should_run_pixel_test" as the 4th argument to the DriverInput constructor?  It looks like we're just overwriting the value after it's set.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:149
> +            if self._port.get_option('pixel_tests'):
> +                if 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 = True
> +            else:
> +                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')

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:41
> -        self.is_reftest = is_reftest
> +        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).

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:227
> -        self._driver.start(self._pixel_tests, [])
> +        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?

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:207
>          return DriverInput(test_name, 0, checksum, is_reftest)

Nit: It might be more readable to make the 4th argument more explicit.  E.g., run_pixel_test=is_reftest.

> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:272
>          return DriverInput(test_name, timeout, checksum, is_reftest)

Nit: It might be more readable to make the 4th argument more explicit.  E.g., run_pixel_test=is_reftest.

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