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

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from James Kozianski <koz at chromium.org>  2011-01-20 19:28:16 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: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.

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