Bug 56076

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:

Version:

528+ (Nightly build)

Hardware:

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+

Hayato Ito

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

Attachments
check-reftest (1.83 KB, patch) 2011-03-10 02:58 PST, Hayato Ito	no flags	Details Formatted Diff Diff
check-reftest (1.84 KB, patch) 2011-03-14 03:58 PDT, Hayato Ito	no flags	Details Formatted Diff Diff
add-tests (3.37 KB, patch) 2011-03-15 20:41 PDT, Hayato Ito	no flags	Details Formatted Diff Diff
treat-it-as-error (4.34 KB, patch) 2011-03-16 21:03 PDT, Hayato Ito	tony: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Hayato Ito

Comment 1 2011-03-10 02:58:43 PST

Created attachment 85302 [details] check-reftest

Hayato Ito

Comment 2 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

Hayato Ito

Comment 3 2011-03-14 03:58:53 PDT

Created attachment 85666 [details] check-reftest

Adam Barth

Comment 4 2011-03-15 02:22:10 PDT

Comment on attachment 85666 [details] check-reftest This patch lacks a test.

Hayato Ito

Comment 5 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.

Hayato Ito

Comment 6 2011-03-15 20:41:54 PDT

Created attachment 85902 [details] add-tests

Dirk Pranke

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

Hayato Ito

Comment 8 2011-03-16 21:03:26 PDT

Created attachment 86025 [details] treat-it-as-error

Hayato Ito

Comment 9 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.

Dirk Pranke

Comment 10 2011-03-16 22:18:23 PDT

Comment on attachment 86025 [details] treat-it-as-error Looks fine. Thanks!

Tony Chang

Comment 11 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.

Hayato Ito

Comment 12 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.

Hayato Ito

Comment 13 2011-03-17 22:09:43 PDT

Committed r81441: <http://trac.webkit.org/changeset/81441>

Note You need to log in before you can comment on or make changes to this bug.