[webkit-reviews] review denied: [Bug 55457] [NRWT] Add support for reftests to new-run-webkit-tests : [Attachment 85028] add-reftests

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Hayato Ito <hayato at chromium.org>'s
request for review:
Bug 55457: [NRWT] Add support for reftests to new-run-webkit-tests
https://bugs.webkit.org/show_bug.cgi?id=55457

Attachment 85028: add-reftests
https://bugs.webkit.org/attachment.cgi?id=85028&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=85028&action=review

This patch looks fine. Just a few nits. We need to figure out whether we need
to add reftests support to run-webkit-tests. That seems like a lot of work, so
we might be able to get away with adding a mode to new-run-webkit-tests to run
just the reftests and then shelling out from run-webkit-tests to
new-run-webkit-tests or just requiring people to run both until
new-run-webkit-tests replaces old-run-webkit-tests. In either case, that a
discussion to be had on webkit-dev. It doesn't need to block checking this in,
but it probably needs to happen before we actually checkin any reftests.

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:85
> +			      ' expected-html file and expected-mismatch.html
file'

nit: s/expected-html/expected.html

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:117
> +		   # Returns a dummy TestResult. We don't have to rebase for
reftests.
> +		   return TestResult(self._filename)

Should this return None and then the calling code handles None appropriately?
That seems cleaner to me at first glance, but I'm not 100% sure how this code
is used.

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:188
> +		 This arg is optional and shoulbe be used only in reftests
until we have a better way to know

s/shoulbe/should

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:200
> +	       # FIXME: Move this actual file writing code somewhere so that
this fucntion doesn't have any side effects.

s/fucntion/function

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:269
> +	   max_test_time = max(driver_output1.test_time,
driver_output2.test_time)

This should  probably just be the sum of the test times since we use this
information to see how long a test took to run.

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:283
> +	   else:
> +	       if driver_output1.image_hash != driver_output2.image_hash:

This can be an "elif"

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:310
> +	   uris = [self.relative_output_filename(filename, fn) for
> +		   fn in out_names]

fn is not a clear variable name. out_filename perhaps?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:328
> +	   uris = [self.relative_output_filename(filename, fn) for
> +		   fn in out_names]

ditto

> Tools/Scripts/webkitpy/layout_tests/port/base.py:350
> +    def expected_html_filename(self, filename):

how about calling this reftest_expected_filename? The fact that it ends in
.html is an implementation detail.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:354
> +    def expected_mismatch_html_filename(self, filename):

How about calling this reftest_expected_mismatch_filename?

> Tools/Scripts/webkitpy/layout_tests/port/test.py:53
> +	   self.without_expectation_files = False

should this just be self.is_reftest?