Bug 53071

Created attachment 80031 [details] pull-up-rebaseline

Created attachment 81206 [details] pull-up-rebaseline--for-review

The changes look fine to me. Looking at _run_rebaseline(), it's not obvious to me that we should be writing new baselines if the test crashed or timed out. I realize this question predates your change, but maybe you could add a FIXME for someone to revisit this later?

Created attachment 81606 [details] pull-up-rebaseline

Thank you for the review. (In reply to comment #3) > The changes look fine to me. > > Looking at _run_rebaseline(), it's not obvious to me that we should be writing new baselines if the test crashed or timed out. I realize this question predates your change, but maybe you could add a FIXME for someone to revisit this later? Nice point. That wasted my time too some time ago. I added a FIXME. Thank you.

Comment on attachment 81606 [details] pull-up-rebaseline View in context: https://bugs.webkit.org/attachment.cgi?id=81606&action=review > Tools/ChangeLog:10 > + This patch is a first step for eliminating test_type/* classes. > + > + https://bugs.webkit.org/show_bug.cgi?id=53071 Is this covered by existing unit tests? If not, it would be nice to add some (either in this patch or a follow up patch). > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:111 > + self._save_baseline_data(driver_output.image_hash, ".checksum", > + encoding="ascii", > + generate_new_baseline=self._options.new_baseline) In a separate patch, we could just get rid of the encoding flag. If we encode the .checksum files as binary, it should be the same as ascii.

Thank you for the review. I've merged the patch with ToT and am going to commit it manually. (In reply to comment #6) > (From update of attachment 81606 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81606&action=review > > > Tools/ChangeLog:10 > > + This patch is a first step for eliminating test_type/* classes. > > + > > + https://bugs.webkit.org/show_bug.cgi?id=53071 > > Is this covered by existing unit tests? If not, it would be nice to add some (either in this patch or a follow up patch). There is no existing unit tests for rebaseline. It seems that we've started to use abstract file system modules recently, now it is possible to write unit tests for rebaseline. That will be in a follow up patch. > > > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:111 > > + self._save_baseline_data(driver_output.image_hash, ".checksum", > > + encoding="ascii", > > + generate_new_baseline=self._options.new_baseline) > > In a separate patch, we could just get rid of the encoding flag. If we encode the .checksum files as binary, it should be the same as ascii. Thank you. I'll take care of it in a separate patch.

Committed r78014: <http://trac.webkit.org/changeset/78014>

(In reply to comment #7) > Thank you for the review. > I've merged the patch with ToT and am going to commit it manually. > > (In reply to comment #6) > > Is this covered by existing unit tests? If not, it would be nice to add some (either in this patch or a follow up patch). > > There is no existing unit tests for rebaseline. > It seems that we've started to use abstract file system modules recently, now it is possible to write unit tests for rebaseline. That will be in a follow up patch. > There are tests in run_webkit_tests_unittest - see the RebaselineTest class. So, I would expect these changes to have been covered by the existing tests.