Bug 53071 - [NRWT] Pull up a rebaseline feature into a single_test_runner out of each test_type.
Summary: [NRWT] Pull up a rebaseline feature into a single_test_runner out of each tes...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 53004 53063
Blocks: 51091
  Show dependency treegraph
 
Reported: 2011-01-25 00:07 PST by Hayato Ito
Modified: 2011-02-09 13:20 PST (History)
3 users (show)

See Also:


Attachments
pull-up-rebaseline (11.65 KB, patch)
2011-01-25 00:10 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
pull-up-rebaseline--for-review (11.80 KB, patch)
2011-02-04 05:08 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
pull-up-rebaseline (11.85 KB, patch)
2011-02-07 23:58 PST, Hayato Ito
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2011-01-25 00:07:40 PST
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.
Comment 1 Hayato Ito 2011-01-25 00:10:46 PST
Created attachment 80031 [details]
pull-up-rebaseline
Comment 2 Hayato Ito 2011-02-04 05:08:11 PST
Created attachment 81206 [details]
pull-up-rebaseline--for-review
Comment 3 Dirk Pranke 2011-02-07 13:02:12 PST
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?
Comment 4 Hayato Ito 2011-02-07 23:58:56 PST
Created attachment 81606 [details]
pull-up-rebaseline
Comment 5 Hayato Ito 2011-02-08 00:01:03 PST
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 6 Tony Chang 2011-02-08 11:37:29 PST
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.
Comment 7 Hayato Ito 2011-02-08 21:29:25 PST
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.
Comment 8 Hayato Ito 2011-02-08 21:33:26 PST
Committed r78014: <http://trac.webkit.org/changeset/78014>
Comment 9 Dirk Pranke 2011-02-09 13:20:55 PST
(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.