[webkit-reviews] review granted: [Bug 48616] new-run-webkit-tests: change TestResults to be serializable : [Attachment 72396] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 48616: new-run-webkit-tests: change TestResults to be serializable
https://bugs.webkit.org/show_bug.cgi?id=48616

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

------- Additional Comments from Tony Chang <tony at chromium.org>
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?