Bug 56076

Summary: rebaseline-chromium-webkit-tests should ignore reftests.
Product: WebKit Reporter: Hayato Ito <hayato>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dpranke, hamaji, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 55457    
Bug Blocks: 36065    
Attachments:
Description Flags
check-reftest
none
check-reftest
none
add-tests
none
treat-it-as-error tony: review+

Description Hayato Ito 2011-03-09 23:07:56 PST
Please see the master bug for the rationale:
https://bugs.webkit.org/show_bug.cgi?id=36065

We are updating new-run-webkit-tests to support reftests. Rebaseline doesn't make sense for reftests, so rebaseline-chromium-webkit-test should ignore test files used by reftests.
Comment 1 Hayato Ito 2011-03-10 02:58:43 PST
Created attachment 85302 [details]
check-reftest
Comment 2 Hayato Ito 2011-03-10 03:00:58 PST
The patch depends on another patch which is under the review.
  https://bugs.webkit.org/show_bug.cgi?id=55457
Comment 3 Hayato Ito 2011-03-14 03:58:53 PDT
Created attachment 85666 [details]
check-reftest
Comment 4 Adam Barth 2011-03-15 02:22:10 PDT
Comment on attachment 85666 [details]
check-reftest

This patch lacks a test.
Comment 5 Hayato Ito 2011-03-15 02:45:10 PDT
Okay. Let me add a test.

(In reply to comment #4)
> (From update of attachment 85666 [details])
> This patch lacks a test.
Comment 6 Hayato Ito 2011-03-15 20:41:54 PDT
Created attachment 85902 [details]
add-tests
Comment 7 Dirk Pranke 2011-03-16 20:23:00 PDT
Comment on attachment 85902 [details]
add-tests

View in context: https://bugs.webkit.org/attachment.cgi?id=85902&action=review

> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:256
> +                self._rebaselining_tests = []

If you return like this, then Rebaseliner.run() will return True (success), and we won't think the rebaselining failed, which is probably not what we want to happen.

I think you either need to change this to raise an Exception or modify the return value to a three-state thing (no tests to rebaseline, error, or proceed).

Otherwise, the patch looks fine.
Comment 8 Hayato Ito 2011-03-16 21:03:26 PDT
Created attachment 86025 [details]
treat-it-as-error
Comment 9 Hayato Ito 2011-03-16 21:06:38 PDT
Thank you for the review.

(In reply to comment #7)
> (From update of attachment 85902 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85902&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:256
> > +                self._rebaselining_tests = []
> 
> If you return like this, then Rebaseliner.run() will return True (success), and we won't think the rebaselining failed, which is probably not what we want to happen.
> 
> I think you either need to change this to raise an Exception or modify the return value to a three-state thing (no tests to rebaseline, error, or proceed).
> 
> Otherwise, the patch looks fine.

That's good point.

I've updated the patch so that we can distinguish 'no-tests' and 'reftest-error' in a slightly different way from your suggestion.
Comment 10 Dirk Pranke 2011-03-16 22:18:23 PDT
Comment on attachment 86025 [details]
treat-it-as-error

Looks fine. Thanks!
Comment 11 Tony Chang 2011-03-17 09:58:49 PDT
Comment on attachment 86025 [details]
treat-it-as-error

View in context: https://bugs.webkit.org/attachment.cgi?id=86025&action=review

> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:242
> +          False if reftests are wrongly marked as 'needs rebaseliing' or True

rebaseliing -> rebaselining

> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:257
> +                self._rebaselining_tests = []

Nit: Should this be set()?  Maybe __init__ should use set() too.
Comment 12 Hayato Ito 2011-03-17 22:00:34 PDT
Comment on attachment 86025 [details]
treat-it-as-error

Hi Tony, thank you for the review.

I'll submit the patch after I fix the following minor points.

View in context: https://bugs.webkit.org/attachment.cgi?id=86025&action=review

>> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:242
>> +          False if reftests are wrongly marked as 'needs rebaseliing' or True
> 
> rebaseliing -> rebaselining

Done

>> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:257
>> +                self._rebaselining_tests = []
> 
> Nit: Should this be set()?  Maybe __init__ should use set() too.

Done. I'll also fix __init__.

It looks the current code doesn't assume self._rebaselining_tests must be 'set'. It seems that it must be 'iteratable' (list, set and so on).
To be consistent with the return value of self._test_expectatations.get_rebaselining_failures(), 'set' is better here.
Comment 13 Hayato Ito 2011-03-17 22:09:43 PDT
Committed r81441: <http://trac.webkit.org/changeset/81441>