new-run-webkit-tests: change TestResults to be serializable
Created attachment 72292 [details] Patch
Hi Tony, Ojan, Could one of you take a look at this?
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
Created attachment 72396 [details] Patch
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.
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?
Created attachment 72409 [details] Patch
Comment on attachment 72409 [details] Patch Clearing flags on attachment: 72409 Committed r70943: <http://trac.webkit.org/changeset/70943>
All reviewed patches have been landed. Closing bug.
(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).