Bug 48616 – new-run-webkit-tests: change TestResults to be serializable

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

new-run-webkit-tests: change TestResults to be serializable

[Dirk Pranke]

Created attachment 72292 [details]
Patch

[Dirk Pranke]

Hi Tony, Ojan,

Could one of you take a look at this?

[Tony Chang]

Why did you pick json?  Wouldn't pickle be a more appropriate format for passing data between python processes?

http://docs.python.org/library/pickle.html

[Dirk Pranke]

Created attachment 72396 [details]
Patch

[Dirk Pranke]

Patch updated to use cPickle instead ....

I was using JSON because I wasn't that familiar with pickling and I have a long-standing bias against language specific marshalling mechanisms for objects (instead of neutral wire formats). But that bias probably didn't make any sense in this case, and the code is certainly shorter and cleaner now.

[Tony Chang]

Comment on attachment 72396 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:42
> +    return [FailureTimeout, FailureCrash, FailureMissingResult,
> +            FailureTextMismatch, FailureMissingImageHash,
> +            FailureMissingImage, FailureImageHashMismatch,
> +            FailureImageHashIncorrect]

Nit: Can this be a tuple stored in just a variable?  E.g.,
_ALL_FAILURE_CLASSES = (...)

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:93
> +    def __eq__(self, other):
> +        return self.__class__.__name__ == other.__class__.__name__

If you're going to have an __eq__, can you add __ne__ too?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:225
> +    OUT_FILENAMES = ["-actual.txt", "-expected.txt", "-diff.txt",
> +                     "-wdiff.html", "-pretty-diff.html"]

Nit: Can this be a tuple?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results.py:55
> +    def __eq__(self, other):
> +        return (self.filename == other.filename and
> +                self.failures == other.failures and
> +                self.test_run_time == other.test_run_time and
> +                self.time_for_diffs == other.time_for_diffs and
> +                self.total_time_for_all_diffs == other.total_time_for_all_diffs)

Can you define __ne__ too?

[Dirk Pranke]

Created attachment 72409 [details]
Patch

[Dirk Pranke]

Comment on attachment 72409 [details]
Patch

Clearing flags on attachment: 72409

Committed r70943: <http://trac.webkit.org/changeset/70943>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 72396 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72396&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:42
> > +    return [FailureTimeout, FailureCrash, FailureMissingResult,
> > +            FailureTextMismatch, FailureMissingImageHash,
> > +            FailureMissingImage, FailureImageHashMismatch,
> > +            FailureImageHashIncorrect]
> 
> Nit: Can this be a tuple stored in just a variable?  E.g.,
> _ALL_FAILURE_CLASSES = (...)
> 

Done. I made the variable public (since the method was) and had to move it to the end of the file, in order for the needed symbols to be defined.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:93
> > +    def __eq__(self, other):
> > +        return self.__class__.__name__ == other.__class__.__name__
> 
> If you're going to have an __eq__, can you add __ne__ too?
> 

Done. I was thinking Python dealt with this automatically, but apparently I needed to def __ne__(self, other); raise NotImplementedError for it to do that, at which point implementing it directly was easier. Added another test for this as well.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py:225
> > +    OUT_FILENAMES = ["-actual.txt", "-expected.txt", "-diff.txt",
> > +                     "-wdiff.html", "-pretty-diff.html"]
> 
> Nit: Can this be a tuple?
>

Done.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results.py:55
> > +    def __eq__(self, other):
> > +        return (self.filename == other.filename and
> > +                self.failures == other.failures and
> > +                self.test_run_time == other.test_run_time and
> > +                self.time_for_diffs == other.time_for_diffs and
> > +                self.total_time_for_all_diffs == other.total_time_for_all_diffs)
> 
> Can you define __ne__ too?

Done (same as above).