[webkit-reviews] review denied: [Bug 67269] Provides a stand alone CSSWG reftest runner which can be used internally. : [Attachment 106077] Patch

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied Ai Makabi
<makabi at google.com>'s request for review:
Bug 67269: Provides a stand alone CSSWG reftest runner which can be used
internally.
https://bugs.webkit.org/show_bug.cgi?id=67269

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

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106077&action=review


Sorry for the big latency... I somehow missed this patch. I think this patch
needs some works for readability.

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:47
> +    def __init__(self):

We might need documentation for each fields. Especially, we might need to
explain the key and values of result_* fields.

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:56
> +class TestCSSWGReftest():

We might need a docstring which describes how to use this class.

BTW, is this the best name for this class? I'd call this CSSWGReftestRunner or
something like this, but I'm not sure.

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:62
> +    def make_reftest_file_data(self, reftests_dir_path):

Cannot we move the code from this function to __init__ ?

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:63
> +	   """Given a reftests directory, makes a dictionary

I think the summary in a docstring should be a single line. Maybe something
like:

"""Initializes this object with the given reftests directory.

More descriptions about self.test_files...

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:71
> +	       raise AssertionError("Put %s in the %s" % (reftests_dir_path,
myport.layout_tests_dir()))

I think raising AssertionError in this way isn't a good idea... But I've found
there are some code which manually raise AssertionError. Hmm...

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:76
> +		   absolute_path = os.path.join(root, file)

I think this path isn't an absolute path, right? Maybe path_from_root and
path_from_testdir (or test_name?) or something like them?

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:90
> +	   """This test compares match/mismatch for rendering results of

Runs tests and prints the results?

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:107
> +    def _compare_match_mismatch(self):

How about check_results?

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:111
> +	       test_file_hash = self.test_files[test_file].image_hash

How about actual_hash?

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:112
> +	       if self.test_files[test_file].matches:

Do we need this if?

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:113
> +		   self._is_matching(test_file, test_file_hash)

How about passing test_files[test_file] instead of test_file. There are many
"test_files[test_file]" in is_matching.

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:117
> +    def _is_matching(self, test_file, test_file_hash):

"is_matching" sounds like a boolean function and this function should not have
a side-effect. How about check_matching?

> Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:123
> +		   _log.error("There is no match file: %s, %s", test_file,
match_file)

It is unclear which file doesn't exist. How about

("Match file %s not found for %s", match_file, test_file)

?