[webkit-reviews] review granted: [Bug 112898] NRWT: Enable pixel tests when retrying tests : [Attachment 194456] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has granted Ryosuke Niwa
<rniwa at webkit.org>'s request for review:
Bug 112898: NRWT: Enable pixel tests when retrying tests
https://bugs.webkit.org/show_bug.cgi?id=112898

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

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


Looks basically pretty good ... a few nits.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:660
> +	   res, err, _ = logging_run(['--no-pixel',
'failures/unexpected/text-image-checksum.html'], tests_included=True,
host=host)

nit: I'd prefer --no-pixel-tests for this. --no-pixel is kinda of a deprecated
hack.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:-837
> -	  
self.assertEqual(len(stats['http']['tests']['passes']['image.html']['results'])
, 5)

you deleted this assert when you moved this to a free function. Do you need to
add it back in to test_reftest_with_two_notrefs()?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:202
> +		   forced_pixel_tests_in_retry =
self._force_pixel_tests_if_needed()

Nit: I'd probably call this "enable" rather than "force" ... it just sounds
nicer :).

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:211
> +		       self._options.pixel_tests = False

Is this needed?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:271
> +	   self._port.start_helper()

I waffled a bit on whether restarting the helper should be part of this routine
or part of the caller, but I'm not sure if there's a clear win one way or
another.