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

[bugzilla-daemon at webkit.org]

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





--- Comment #16 from James Kozianski <koz at chromium.org>  2011-01-31 21:28:53 PST ---
(In reply to comment #14)
> (From update of attachment 79891 [details])
> 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.

Done.

> 
> > Tools/Scripts/webkitpy/common/net/testoutput.py:60
> > +        test_name = filename
> 
> this line doesn't seem to add much.

Removed.

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

This is useful for rebaselining - if one TestOutput is 'newer' than the other, then it means that we can overwrite the old one.

> 
> > Tools/Scripts/webkitpy/common/net/testoutput.py:128
> > +    def _path_to_platform(self, platform):
> 
> Why pass in platform instead of just using self._platform?

A previous comment from eseidel: "Seems like the first part of this is some sort of install_path function instead?  I could see other pieces of code wanting to have some way to generate paths from platform, file pairs, no?"

It seems like overkill to put it such a function in its own module though (and where exactly such a module would be put is another question).

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

Done.

> 
> > 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"?

Done.

> 
> > Tools/Scripts/webkitpy/common/net/testoutput.py:149
> > +    def delete(self):
> 
> why do we need this?

For deleting duplicate TestOutputs.

> 
> > 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.

So we should ignore checksums then? (FIXME added).

> 
> > 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.

Done.

> 
> > Tools/Scripts/webkitpy/common/net/testoutputset.py:129
> > +    def sub_builders(self):
> 
> what is "sub" about these builders?

Yeah, it should just be builders, huh?
Done.

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