Bug 53071 – [NRWT] Pull up a rebaseline feature into a single_test_runner out of each test_type.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Hayato Ito]

This is a separated patch from https://bugs.webkit.org/show_bug.cgi?id=51091.

This patch is only refactoring. There is no functional changes.

To make this patch smaller, I only extracted rebaseline features from test_type/* into a single_test_runner.
Following patches will pull up remaining features which are defined in each test_type::comapre_output() into a single_test_runner and get rid of test_type/* classes.

[Hayato Ito]

Created attachment 80031 [details]
pull-up-rebaseline

[Hayato Ito]

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

[Dirk Pranke]

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?

[Hayato Ito]

Created attachment 81606 [details]
pull-up-rebaseline

[Hayato Ito]

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.

[Tony Chang]

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.

[Hayato Ito]

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.

[Hayato Ito]

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

[Dirk Pranke]

(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.