Add TestOutput classes to webkitpy.

• Component: New Bugs
• Status: RESOLVED FIXED

[James Kozianski]

Add TestOutput classes to webkitpy.

[James Kozianski]

Created attachment 78363 [details]
Patch

[Eric Seidel (no email)]

I'll need to stare at this for longer and think about how this integrates with all the rest of our test results infrastructure (which ORWT code is using more and more, like in bug 52048).

[James Kozianski]

Created attachment 79548 [details]
Patch

[James Kozianski]

I've cut out the modifications to the port and common/net files leaving just the TestOutput classes if that will help (probably should have been two different patches in the first place).

[Eric Seidel (no email)]

Comment on attachment 79548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79548&action=review

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

What does _main_file mean?  is that the original test file?

[Eric Seidel (no email)]

Comment on attachment 79548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79548&action=review

> Tools/Scripts/webkitpy/common/net/testoutput.py:95
> +    def __eq__(self, other):
> +        return other != None and self.name() == other.name() and self.type() == other.type() and self.platform() == other.platform() and self.is_actual() == other.is_actual() and self.same_content(other)

Although I don't believe in wrapping just to fit on punch cards, I think some wrapping might make this mor readable.

> Tools/Scripts/webkitpy/common/net/testoutputset.py:70
> +        files = []
> +        for filename in self._zip_file.namelist():
> +            files.append(self._zip_file.open(filename))
> +        return files

This is better written as a list comprehension.

> Tools/Scripts/webkitpy/common/net/testoutputset.py:98
> +        outputs = []
> +        for image_file in image_files:
> +            checksum_file_name = re.sub(re.compile('\.png'), '.checksum', image_file.name())

Anytime you're using this pattern, a helper function + a list comprehension is normally cleaner.

> Tools/Scripts/webkitpy/common/net/testoutputset.py:128
> +        outputs = []
> +        for builder in self._builders:
> +            outputs += builder.outputs_for(name, **kwargs)
> +        return outputs

list comprehension.

> Tools/Scripts/webkitpy/common/net/testoutputset_unittest.py:35
> +    def __str__(self):
> +        return "FakeZip"

Why override __str__?

[Eric Seidel (no email)]

Comment on attachment 79548 [details]
Patch

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.

[Eric Seidel (no email)]

Comment on attachment 79548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79548&action=review

I think we need another round here.  Most helpful would be some explanation as to how TestOutput is supposed to be used.  I.e. does TestOutput encompass all of the outputs from a given test (-diffs.txt, -expected.txt, -diffs.html, -expected.checksum, etc.) or just some subset?  If so, which subset?  And what is _main_file supposed to be?  And why can it be either an expeted or an actual?

The TestOutputSet.outputs_for function is probably the most important function in this diff, since it explans how these things are supposed to be created from a layout test results archive.  But I'm not sure I fully understand it yet (needs to be simplified and broken down a bit).

> Tools/Scripts/webkitpy/common/net/testoutputset.py:33
> +class TestOutputSet(object):
> +    """A set of test outputs"""

So this sounds a lot like LayoutTestResults is now, no?

> Tools/Scripts/webkitpy/common/net/testoutputset.py:53
> +        return AggregateTestOutputSet(output_sets)

This class doesn't exist?

> Tools/Scripts/webkitpy/common/net/testoutputset.py:57
> +        z = DirectoryFileSet(path)

"z" isn't a very good name, and I doubt this needs to be a local anyway.

> Tools/Scripts/webkitpy/common/net/testoutputset.py:58
> +        return TestOutputSet('local %s builder' % platform, platform, z)

If you're worried about line length, I would have put the string in its own local before I put the file set in one.  Then agian, we don't really worry about line length in webkit.  We (intentionally) do not follow the 80c limit of PEP8.

> Tools/Scripts/webkitpy/common/net/testoutputset.py:74
> +        target_type = kwargs.get('target_type', None)
> +        exact_match = kwargs.get('exact_match', False)

Seems we should just list these as arguments with default values.

> Tools/Scripts/webkitpy/common/net/testoutputset.py:94
> +        checksum_files = []
> +        text_files = []
> +        image_files = []
> +        for output_file in self.files():
> +            name_match = name_matcher.search(output_file.name())
> +            actual_match = actual_matcher.search(output_file.name())
> +            expected_match = expected_matcher.search(output_file.name())
> +            if name_match and (actual_match or (self._include_expected and expected_match)):
> +                if output_file.name().endswith('.checksum'):
> +                    checksum_files.append(output_file)
> +                elif output_file.name().endswith('.txt'):
> +                    text_files.append(output_file)
> +                elif output_file.name().endswith('.png'):
> +                    image_files.append(output_file)

This confuses me.  I would have made this a helper method, used a list comprehension, and then zip, to zip the returned 3-tuple into 3 separate lists.

> Tools/Scripts/webkitpy/common/net/testoutputset.py:113
> +            outputs = filter(lambda r: r.name() == name, outputs)

Personally I tend to use [output for output in outputs where output.name() == name] more often than I use filter.  But filter is probably more-readable here, especially if we give 'r' a better name (see the webkit-style guide on variable naming).

> Tools/Scripts/webkitpy/common/net/testoutputset.py:115
> +        outputs = filter(lambda r: target_type is None or target_type == r.type(), outputs)

target_type in [None, r.type()]?

[James Kozianski]

Comment on attachment 79548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79548&action=review

>> Tools/Scripts/webkitpy/common/net/testoutput.py:40
>> +        self._main_file = files[0]
> 
> What does _main_file mean?  is that the original test file?

The main file is the one that is used to derive the name of the test and also the contents of the test. So for text test outputs it's the singular .txt file, but for image outputs (that have checksums and images) we use the checksum file if it is there, or the image one as a fallback.

I've added a comment to that effect.

>> Tools/Scripts/webkitpy/common/net/testoutput.py:46
>> +            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?

I moved the assignment of _platform down to here.

I kind of like having None as an indicator that you're explicitly not setting a platform, but I can change it if it's more idiomatic?

>> Tools/Scripts/webkitpy/common/net/testoutput.py:63
>> +            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. :)

Any ideas where to put it? Do you mean it should be a top level function in this module?

One school of thought would be that other people shouldn't reuse this function, rather they should reuse this class :-)

>> Tools/Scripts/webkitpy/common/net/testoutput.py:74
>> +            self._test_name = filename[:filename.rindex(os.path.sep)]
> 
> This seems like os.path.basename or os.path.split, no?

Yes, you're right, but I think this function is actually dead code anyway, so I've removed it.

>> Tools/Scripts/webkitpy/common/net/testoutput.py:77
>> +        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.

A contents() function makes sense if we continue with the idea that a TestOutput is just a text output or an image output, but if we make it so that a TestOutput refers to all the different kinds of test output together then it won't be necessary.

>> Tools/Scripts/webkitpy/common/net/testoutput.py:81
>> +            file.save_to(path)
> 
> What does save_to do?  Is this mockable?  I assume _files is a list/tuple of file objects?

I added the function comment """Have the files in this TestOutput write themselves to the disk at the specified location."""

This is mockable - just pass in fake files in the first place, and yes, _files is a list of FileSetFileHandles (as defined in webkitpy/common/system/fileset.py).

>> Tools/Scripts/webkitpy/common/net/testoutput.py:85
>> +        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.

So the reason why this class is called TestOutput instead of Result or something similar (which was its original name until Dirk suggested I change it) is because this represents the raw output of a test, not a comparison between an expected output and an actual output. So one TestOutput might be the output we expect from a test, whereas another might be the actual output from a test.

I agree that it would make sense for the run of a test to produce something more than this (I'm imagining a TestResult class that contains all the things you mention), but the part encapsulated by this class still needs to be a first class concept (ie: be named), because we need to talk about expected results that don't have associated actual results and vice versa.

>> Tools/Scripts/webkitpy/common/net/testoutput.py:92

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

We override this function in ImageTestOutput, which has more intricate content comparison logic.

>> Tools/Scripts/webkitpy/common/net/testoutput.py:95
>> +        return other != None and self.name() == other.name() and self.type() == other.type() and self.platform() == other.platform() and self.is_actual() == other.is_actual() and self.same_content(other)
> 
> Although I don't believe in wrapping just to fit on punch cards, I think some wrapping might make this mor readable.

Done.

>> Tools/Scripts/webkitpy/common/net/testoutput.py:98
>> +        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
>> +        return self.is_actual() and not other.is_actual()
> 
> I'm very confused by this.

The idea is that if you have an 'expected' output and an 'actual' output, the 'actual' is newer in a sense because it was presumably generated after the expectation was.

Maybe we should scrap this and just have the callers explicitly state what they want? I'll remove it and add it back in a later change if it makes things a lot cleaner.

>> Tools/Scripts/webkitpy/common/net/testoutput.py:105
>> +        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.)

Yeah it's after the operator according to PEB8.
Done.

>> Tools/Scripts/webkitpy/common/net/testoutput.py:124
>> +        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?

I extracted a _path_to_platform() function (or did you mean to make it more widely exposed)?

This doesn't need to consider platform fallback - that is handled externally. If a test output needs to be changed to a different platform then retarget() is called with a new platform.

>> Tools/Scripts/webkitpy/common/net/testoutput.py:129
>> +            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.

I've added the following function comment:

"""Save the files of this TestOutput to the appropriate directory inside the LayoutTests directory. Typically this means that these files will be saved in "LayoutTests/platform/<platform>/, or simply LayoutTests if the platform is None."""

>> Tools/Scripts/webkitpy/common/net/testoutput.py:133
>> +            file.delete()
> 
> Not understanding what all a TestOutput encompasses, it's difficult to know what "delete" is supposed to do.

I've added the following function comment:

"""Deletes the files that comprise this TestOutput from disk. This fails if the files are virtual files (eg: the files may reside inside a remote zip file)."""

It's use in the code is for deleting TestOutputs that have been identified as duplicates in LayoutTests.

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

With the current conception of TestOutput the run of a test would be a list of TestOutputs each with a different type.

I made it that way because it would allow me to dedupe TestOutputs on a finer granularity - this way a test might have its expected text output in a 'mac' platform directory, but have different image outputs in 'mac-leopard' and 'mac-snowleopard' for example. I don't know if in a situation like that it's better to have the text output duped so there's a copy next to each image - can you advise?

>> Tools/Scripts/webkitpy/common/net/testoutputset.py:33
>> +    """A set of test outputs"""
> 
> So this sounds a lot like LayoutTestResults is now, no?

Yeah, maybe. It looks like LayoutTestResults just returns names of tests, whereas this provides actual files. I can imagine the current LayoutTestResults potentially turning into a factory method on this class for creating a TestOutputSet from the results.html for a particular builder.

>> Tools/Scripts/webkitpy/common/net/testoutputset.py:53
>> +        return AggregateTestOutputSet(output_sets)
> 
> This class doesn't exist?

It's defined further down. As it is quite a small helper class I figured I would leave it in this file.

>> Tools/Scripts/webkitpy/common/net/testoutputset.py:57
>> +        z = DirectoryFileSet(path)
> 
> "z" isn't a very good name, and I doubt this needs to be a local anyway.

Haha yes, that is a particularly bad name. I've inlined it as per your suggestion.

>> Tools/Scripts/webkitpy/common/net/testoutputset.py:58
>> +        return TestOutputSet('local %s builder' % platform, platform, z)
> 
> If you're worried about line length, I would have put the string in its own local before I put the file set in one.  Then agian, we don't really worry about line length in webkit.  We (intentionally) do not follow the 80c limit of PEP8.

Right.

>> Tools/Scripts/webkitpy/common/net/testoutputset.py:70
>> +        return files
> 
> This is better written as a list comprehension.

Done.

>> Tools/Scripts/webkitpy/common/net/testoutputset.py:74
>> +        exact_match = kwargs.get('exact_match', False)
> 
> Seems we should just list these as arguments with default values.

I feel that it aids readability at the call sites to have them specified as keyword arguments.

outs.outputs_for('some-test', None, True) is harder to read than
outs.outputs_for('some-test', exact_match=True)

>> Tools/Scripts/webkitpy/common/net/testoutputset.py:94

> 
> This confuses me.  I would have made this a helper method, used a list comprehension, and then zip, to zip the returned 3-tuple into 3 separate lists.

I've pulled this out into a helper method, but I'm not sure how you mean to translate this into a list comprehension, or how zip would be helpful here. Could you clarify?

>> Tools/Scripts/webkitpy/common/net/testoutputset.py:98
>> +            checksum_file_name = re.sub(re.compile('\.png'), '.checksum', image_file.name())
> 
> Anytime you're using this pattern, a helper function + a list comprehension is normally cleaner.

Yep, that makes sense. I've pulled out a helper function here, and removed a for loop below.

>> Tools/Scripts/webkitpy/common/net/testoutputset.py:113
>> +            outputs = filter(lambda r: r.name() == name, outputs)
> 
> Personally I tend to use [output for output in outputs where output.name() == name] more often than I use filter.  But filter is probably more-readable here, especially if we give 'r' a better name (see the webkit-style guide on variable naming).

Ok. I've changed r to output.

>> Tools/Scripts/webkitpy/common/net/testoutputset.py:115
>> +        outputs = filter(lambda r: target_type is None or target_type == r.type(), outputs)
> 
> target_type in [None, r.type()]?

Done.

>> Tools/Scripts/webkitpy/common/net/testoutputset.py:128
>> +        return outputs
> 
> list comprehension.

Done.

>> Tools/Scripts/webkitpy/common/net/testoutputset_unittest.py:35
>> +        return "FakeZip"
> 
> Why override __str__?

Just for making debugging slightly easier to parse.

[James Kozianski]

Created attachment 79684 [details]
Patch

[Dirk Pranke]

Comment on attachment 79684 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79684&action=review

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

Do we need to call this LayoutTestOutput to distinguish it from other kinds of tests (e.g., JSC tests, Perf tests, etc.)?

Also, I think I've mentioned this before, in another bug/context, but we use the word "test" to refer to two different kinds of things. 

The first is a given Layout Test (e.g., fast/html/article-element.html) -- that's the sense being used here -- and the second is one particular kind of check against the output. I.e., checking the text output, checking the image output, etc. The latter shows up in layout_tests/layout_package/test_failures, layout_tests/test_types, and in a few other places. I'd really like to find a different noun for the latter kind of thing, but have yet to think of anything good. "Check" maybe?

At any rate, I'm looking for suggestions. That said, we might want to update the docstring to this class to be clear we're talking about all of the related "checks" for a single "test". It's possible to read this description and get slightly confused otherwise.

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

Maybe add a comment here that the set of test outputs is usually (always?) from a whole test run?

> Tools/Scripts/webkitpy/common/net/testoutputset_unittest.py:50
> +        print "FakeZip> cp %s %s" % (filename, path)

Do you need these print statements (here and in delete())? I don't see you capturing stdout output anywhere, but maybe I'm missing something. If not, you should probably delete them because generally we don't like our unit tests to print anything.

Also, maybe consider passing in a filesystem_mock object to the constructor and having extract write to that?

I need some zip-wrapper objects for writing tests for the rebaseline-chromium-webkit-tests script so we should use the same objects if possible. In which case, I'd want FakeZip to be in a separate module so I can import it.

[James Kozianski]

(In reply to comment #11)
> (From update of attachment 79684 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79684&action=review
> 
> > Tools/Scripts/webkitpy/common/net/testoutput.py:34
> > +class TestOutput(object):
> 
> Do we need to call this LayoutTestOutput to distinguish it from other kinds of tests (e.g., JSC tests, Perf tests, etc.)?

LayoutTests seem to be the most prominent kind of tests (in webkitpy if not WebKit), so I think it would be okay to leave it unadorned, and to prefix the other TestOutput classes if we need to introduce them.

> 
> Also, I think I've mentioned this before, in another bug/context, but we use the word "test" to refer to two different kinds of things. 
> 
> The first is a given Layout Test (e.g., fast/html/article-element.html) -- that's the sense being used here -- and the second is one particular kind of check against the output. I.e., checking the text output, checking the image output, etc. The latter shows up in layout_tests/layout_package/test_failures, layout_tests/test_types, and in a few other places. I'd really like to find a different noun for the latter kind of thing, but have yet to think of anything good. "Check" maybe?

Right - test the noun and test the verb. I think Check is a good substitute for test as a verb.

> 
> At any rate, I'm looking for suggestions. That said, we might want to update the docstring to this class to be clear we're talking about all of the related "checks" for a single "test". It's possible to read this description and get slightly confused otherwise.

Yep, I've clarified the comment.

> 
> > Tools/Scripts/webkitpy/common/net/testoutputset.py:33
> > +    """A set of test outputs"""
> 
> Maybe add a comment here that the set of test outputs is usually (always?) from a whole test run?

This class is actually quite generic. It can represent a set of test outputs from a buildbot, or it could be the outputs for a particular test's run, or it could be the -expected outputs in a LayoutTest directory.

> 
> > Tools/Scripts/webkitpy/common/net/testoutputset_unittest.py:50
> > +        print "FakeZip> cp %s %s" % (filename, path)
> 
> Do you need these print statements (here and in delete())? I don't see you capturing stdout output anywhere, but maybe I'm missing something. If not, you should probably delete them because generally we don't like our unit tests to print anything.

Done.

> 
> Also, maybe consider passing in a filesystem_mock object to the constructor and having extract write to that?

Done.

> 
> I need some zip-wrapper objects for writing tests for the rebaseline-chromium-webkit-tests script so we should use the same objects if possible. In which case, I'd want FakeZip to be in a separate module so I can import it.

Done, and I renamed FakeZip to MockZip to fit in with MockFileSystem.

[James Kozianski]

Created attachment 79891 [details]
Patch

[Ojan Vafai]

Comment on attachment 79891 [details]
Patch

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?

[James Kozianski]

Created attachment 80715 [details]
Patch

[James Kozianski]

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

[Ojan Vafai]

Comment on attachment 79891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79891&action=review

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

As we said in person is_rebaseline_of is more clear and serves the same purpose.

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

s/is_rebaseline_of/is_new_baseline_for/ ?

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

But this function doesn't take platform/file pairs, it just takes the platform. I see what Eric's original comment was getting at, but I don't think it actually makes sense. It would if we were ultimately creating just a single string. But we're creating a path and a filename. In either case, I think having this as a separate function is easier to read, but it's not useful enough to try to make generic. Certainly as is, it doesn't make sense to me to pass in platform as an argument.

It's not a big deal either way though.

[Ojan Vafai]

Comment on attachment 80715 [details]
Patch

r+ once you address comment 17.

[James Kozianski]

Created attachment 81183 [details]
Patch

[James Kozianski]

(In reply to comment #17)
> (From update of attachment 79891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79891&action=review
> 
> >>> 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.
> 
> As we said in person is_rebaseline_of is more clear and serves the same purpose.

Done.

> 
> > Tools/Scripts/webkitpy/common/net/testoutput.py:111
> > +    def is_rebaseline_of(self, other):
> 
> s/is_rebaseline_of/is_new_baseline_for/ ?

Done.

> 
> >>> 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).
> 
> But this function doesn't take platform/file pairs, it just takes the platform. I see what Eric's original comment was getting at, but I don't think it actually makes sense. It would if we were ultimately creating just a single string. But we're creating a path and a filename. In either case, I think having this as a separate function is easier to read, but it's not useful enough to try to make generic. Certainly as is, it doesn't make sense to me to pass in platform as an argument.
> 
> It's not a big deal either way though.

Yep. Done.

[James Kozianski]

Created attachment 81184 [details]
Patch

[Ojan Vafai]

Comment on attachment 81184 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81184&action=review

Just a couple more nits I noticed. Sorry. :)

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

is_new_baseline_for? a "rebaseline" is not really a thing. :)

> Tools/Scripts/webkitpy/common/net/testoutputset.py:79
> +            if name_match and (actual_match or (self._include_expected and expected_match)):

nit: typical webkit style for this would be to invert the if clause and add a continue. makes for less deeply nested indents and arguably more readable code.

[James Kozianski]

(In reply to comment #22)
> (From update of attachment 81184 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81184&action=review
> 
> Just a couple more nits I noticed. Sorry. :)

It's cool :-)

> 
> > Tools/Scripts/webkitpy/common/net/testoutput.py:95
> > +    def is_new_rebaseline_for(self, other):
> 
> is_new_baseline_for? a "rebaseline" is not really a thing. :)

Done.

> 
> > Tools/Scripts/webkitpy/common/net/testoutputset.py:79
> > +            if name_match and (actual_match or (self._include_expected and expected_match)):
> 
> nit: typical webkit style for this would be to invert the if clause and add a continue. makes for less deeply nested indents and arguably more readable code.

Done.

[James Kozianski]

Created attachment 81433 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 81433 [details]
Patch

Clearing flags on attachment: 81433

Committed r77780: <http://trac.webkit.org/changeset/77780>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/77780 might have broken GTK Linux 64-bit Debug