[Webkit-unassigned] [Bug 55457] [NRWT] Add support for reftests to new-run-webkit-tests

[bugzilla-daemon at webkit.org]

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


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #85028|review?                     |review-
               Flag|                            |




--- Comment #6 from Ojan Vafai <ojan at chromium.org>  2011-03-08 23:38:32 PST ---
(From update of attachment 85028)
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?

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