[webkit-reviews] review granted: [Bug 73613] [NRWT] reftest should support having multiple references per test : [Attachment 117581] 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 73613: [NRWT] reftest should support having multiple references per test
https://bugs.webkit.org/show_bug.cgi?id=73613

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

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


The patch looks basically fine. I found a few places where I'd rework things a
bit or add comments to make them clearer (at least to me).

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:725
> +    def test_reftest_skipped_if_unlisted(self):

It wasn't immediately clear to me what "unlisted" meant, so I'd probably add a
comment here that you're testing what happens if there's an old-style reftest
found in a directory containing a manifest file.

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:70
> +		       _log.error('The reftest (%s) can not have an expectation
file. Please remove (%s).',

I would probably log this as:

"%s is both a reftest and has an expected output file: %s"

it may be that the test shouldn't actually be a reftest, rather than that it
shouldn't have the expected output.

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:273
> +	       # Reftest fails if any mismatch matches; it succeeds if any
match matches

I think it would be clearer if you pull this comment up before the for
statement, and combine it with the comment on line 267, something like:

# A reftest can have multiple match references and multiple mismatch
references; 
# the test fails if any mismatch matches and all of the matches don't match. 
# To minimize the number of references we have to check, we run all of the
mismatches first, then the matches, and short-circuit out as soon as we can.
# Note that sorting by the expectation sorts "!=" before "==" so this is easy
to do.

It's a bit wordy, but the logic is non-obvious to the  uninitiated, so I think
it's worth it to be wordy here.

And then I would get rid of the "putAllMismatchBeforeMatch" variable (which
threw me until I realized you were just aliasing 'sorted'.

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:275
> +		   test_result_writer.write_test_result(self._port,
self._test_name, test_output, reference_output, test_result.failures)

can this just be a "break"?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:446
> +	   return reftest_list[filename]

Nit: these last three lines could be just "return reftest_list.get(filename,
[])".

> Tools/Scripts/webkitpy/layout_tests/port/base.py:1064
> +	       parsed_list[key] = []

Nit: these three lines can be written as "parsed_list.setdefault(key,
filesystem.join(...)". That returns parsed_list[key] as well, so you can
actually chain in the next line, but I don't know if that would be more or less
readable in this case.