[Webkit-unassigned] [Bug 52136] Add TestOutput classes to webkitpy.

[bugzilla-daemon at webkit.org]

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


Ojan Vafai <ojan at chromium.org> changed:

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




--- Comment #14 from Ojan Vafai <ojan at chromium.org>  2011-01-30 19:38:47 PST ---
(From update of attachment 79891)
View in context: https://bugs.webkit.org/attachment.cgi?id=79891&action=review

This is close. Just a few more things...

> Tools/Scripts/webkitpy/common/net/testoutput.py:45
> +        self._main_file = files[0]

This _main_file stuff is just confusing. It seems like you only need "contents()" for checking same_contents(), right? so how about getting rid of contents() and instead making same_contents abstract and have the subclasses implement it since it seems subclass specific.

> Tools/Scripts/webkitpy/common/net/testoutput.py:60
> +        test_name = filename

this line doesn't seem to add much.

> Tools/Scripts/webkitpy/common/net/testoutput.py:105
> +    def is_newer_than(self, other):

?

> Tools/Scripts/webkitpy/common/net/testoutput.py:128
> +    def _path_to_platform(self, platform):

Why pass in platform instead of just using self._platform?

> Tools/Scripts/webkitpy/common/net/testoutput.py:135
> +    def _install_file(self, file, path):

s/_install_file/_save_expected_result/ ? (see below)

> Tools/Scripts/webkitpy/common/net/testoutput.py:141
> +    def install(self, path_to_layout_tests):

install is a weird word for this. how about..."save_expected_results"?

> Tools/Scripts/webkitpy/common/net/testoutput.py:149
> +    def delete(self):

why do we need this?

> Tools/Scripts/webkitpy/common/net/testoutput.py:181
> +        if self.has_checksum() and other.has_checksum():
> +            return self._checksum_file.contents() == other._checksum_file.contents()

We shouldn't assume the checksums actually match the image files. It has def happened that people commit the new checksum but forget to commit the new image. In either case, a FIXME is sufficient for now.

> Tools/Scripts/webkitpy/common/net/testoutputset.py:33
> +    """A set of test outputs"""

This comment doesn't add anything over the class name. Just delete it. It's pretty clear what this class does.

> Tools/Scripts/webkitpy/common/net/testoutputset.py:129
> +    def sub_builders(self):

what is "sub" about these builders?

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