[webkit-reviews] review denied: [Bug 52136] Add TestOutput classes to webkitpy. : [Attachment 79548] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied James Kozianski <koz at chromium.org>'s
request for review:
Bug 52136: Add TestOutput classes to webkitpy.
https://bugs.webkit.org/show_bug.cgi?id=52136

Attachment 79548: Patch
https://bugs.webkit.org/attachment.cgi?id=79548&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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()]?