[webkit-reviews] review granted: [Bug 66837] Parse reftest.list and extract types of ref tests : [Attachment 117322] 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 66837: Parse reftest.list and extract types of ref tests
https://bugs.webkit.org/show_bug.cgi?id=66837

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

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


Patch generally looks fine, just a few nits and one question that I'm not sure
about the answer to but I'm not sure that it should block the patch ...

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:454
> +	   self.assertTrue(json_string.find('"num_flaky":0') != -1)

any particular reason these two lines changed, or was it just to make things
more consistent?

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:76
> +	   if reftest_expected_mismatch_filename and
fs.exists(reftest_expected_mismatch_filename):

Given that we can't rely on files being named '-expected.html' any more, the
error message on line 78+ is slightly misleading. Can you rewrite it to
something like "One test file cannot have both match and mismatch references.
Please remove either %s or %s" ?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:151
> +	   self._reftest_list = dict()

Style nit: I'd usually use {} instead of dict() here, but it doesn't really
matter.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:449
> +	       return None

Should this be either expected_filename(test_name, '.html') or
expected_filename(test_name, '-mismatch.html') as well for compatibility with
the branch in  lines 439-443? I.e., maybe this function should never return
None? I'd have to look at the calling code to see if this actually matters.

> Tools/Scripts/webkitpy/layout_tests/port/test_files_unittest.py:87
> +== test-3.html test-ref.html"""

Style nit: I think these days we usually try to avoid HEREDOC-style multiline
strings and instead use string concatenation, so that the indentation is
preserved.