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

[bugzilla-daemon at webkit.org]

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


Eric Seidel <eric at webkit.org> changed:

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




--- Comment #8 from Eric Seidel <eric at webkit.org>  2011-01-19 21:24:28 PST ---
(From update of attachment 79548)
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()]?

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