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>