Summary: | rebaseline-chromium-webkit-tests should ignore reftests. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hayato Ito <hayato> | ||||||||||
Component: | Tools / Tests | Assignee: | 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
Hayato Ito
2011-03-09 23:07:56 PST
Created attachment 85302 [details]
check-reftest
The patch depends on another patch which is under the review. https://bugs.webkit.org/show_bug.cgi?id=55457 Created attachment 85666 [details]
check-reftest
Comment on attachment 85666 [details]
check-reftest
This patch lacks a test.
Okay. Let me add a test. (In reply to comment #4) > (From update of attachment 85666 [details]) > This patch lacks a test. Created attachment 85902 [details]
add-tests
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. Created attachment 86025 [details]
treat-it-as-error
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 on attachment 86025 [details]
treat-it-as-error
Looks fine. Thanks!
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 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. Committed r81441: <http://trac.webkit.org/changeset/81441> |