[Webkit-unassigned] [Bug 67269] Provides a stand alone CSSWG reftest runner which can be used internally.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 20 02:18:53 PDT 2011


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


Shinichiro Hamaji <hamaji at chromium.org> changed:

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




--- Comment #10 from Shinichiro Hamaji <hamaji at chromium.org>  2011-09-20 02:18:54 PST ---
(From update of attachment 106077)
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)

?

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



More information about the webkit-unassigned mailing list