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

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Eric Seidel <eric at webkit.org>  2011-01-19 21:12:35 PST ---
(From update of attachment 79548)
View in context: https://bugs.webkit.org/attachment.cgi?id=79548&action=review

> Tools/Scripts/webkitpy/common/net/testoutput.py:46
> +        if self._platform is None:
> +            self._platform = self._extract_platform(filename)

I would have set platform closer to this code.

Should platform have a default argument of None in the constructor?

> Tools/Scripts/webkitpy/common/net/testoutput.py:63
> +        if 'LayoutTests' in path:
> +            path = path[1 + path.index('LayoutTests'):]
> +        if 'layout-test-results' in path:
> +            path = path[1 + path.index('layout-test-results'):]
> +        if 'platform' in path:
> +            path = path[2 + path.index('platform'):]

I probably would have made this a separate function.  i suspect other parts of the layout test code will eventually want to share this.  Basically you're converting from any (possibly absolute) path to a relative-to-LayoutTests path, which is useful. :)

> Tools/Scripts/webkitpy/common/net/testoutput.py:74
> +        self._test_name = filename
> +        if os.path.sep in filename:
> +            self._test_name = filename[:filename.rindex(os.path.sep)]

This seems like os.path.basename or os.path.split, no?

> Tools/Scripts/webkitpy/common/net/testoutput.py:77
> +    def contents(self):
> +        return self._main_file.contents()

main_file is unclear to me, so it's difficult to know if this makes sense to have a "contents" function on thsi class.

> Tools/Scripts/webkitpy/common/net/testoutput.py:81
> +    def save_to(self, path):
> +        for file in self._files:
> +            file.save_to(path)

What does save_to do?  Is this mockable?  I assume _files is a list/tuple of file objects?

> Tools/Scripts/webkitpy/common/net/testoutput.py:85
> +    def is_actual(self):
> +        """Is this output the actual output of a test? (As opposed to expected output.)"""
> +        return self._is_actual

Strange that this class has dual modality like this.  Do you create TestOutput instances for both the -actual and the -expetected files?  If so, why is there a _files array here and not just one-per file?  Seems a TestOutput should include outputs at once, including all the diffs, actual and expected.  But maybe I don't understand the purpose of this class yet.

> Tools/Scripts/webkitpy/common/net/testoutput.py:92
> +    def same_content(self, other):
> +        return self.contents() == other.contents()

I'm not sure this adds much, but OK.

> Tools/Scripts/webkitpy/common/net/testoutput.py:98
> +    def __hash__(self):
> +        return hash(str(self.name()) + str(self.type()) + str(self.platform()))

Difficult to read __eq__ so difficult to tell if this is right, but iirc you tested it, so I trust you. :)

> Tools/Scripts/webkitpy/common/net/testoutput.py:102
> +    def is_newer_than(self, other):
> +        """'New' outputs are those actually coming from a test."""
> +        return self.is_actual() and not other.is_actual()

I'm very confused by this.

> Tools/Scripts/webkitpy/common/net/testoutput.py:105
> +    def is_rebaseline_of(self, other):
> +        return self.name() == other.name() and self.type() == other.type() and self.platform() == other.platform() and self.is_actual() and (not other.is_actual())

Again I think a little wrapping would help make this readable.  Just wrap right before the "and" would be my suggestion (at least I think before the and is webkit style... maybe it's after?  Maybe we need to check PEP8.)

> Tools/Scripts/webkitpy/common/net/testoutput.py:124
> +        if self._platform is None:
> +            platform_component = ""
> +        else:
> +            platform_component = "platform/%s/" % self._platform
> +        extension = os.path.splitext(file.name())[1]
> +        path = '%s/%s' % (path, platform_component)
> +        filename = self.name() + '-expected' + extension

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?

Does this need to take into account platform fallback?

> Tools/Scripts/webkitpy/common/net/testoutput.py:129
> +    def install(self, path):
> +        for file in self._files:
> +            self._install_file(file, path)

It's unclear to me what "isntall" means here, but I guess I don't undestand where these TestOutput objects are coming from.

> Tools/Scripts/webkitpy/common/net/testoutput.py:133
> +    def delete(self):
> +        for file in self._files:
> +            file.delete()

Not understanding what all a TestOutput encompasses, it's difficult to know what "delete" is supposed to do.

> Tools/Scripts/webkitpy/common/net/testoutput.py:136
> +class TextTestOutput(TestOutput):

Unclear why we have subclasses here.  Since my current understanding is that a TestOutput is all the output files for a given test from a given test run.

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