Summary: | Add an option to NRWT to skip failing tests | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||
Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | dpranke, eric, mihaip, ojan | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Adam Barth
2011-05-05 13:53:00 PDT
Created attachment 92461 [details]
Patch
Comment on attachment 92461 [details]
Patch
Seems reasonable to me.
View in context: https://bugs.webkit.org/attachment.cgi?id=92461&action=review > Tools/ChangeLog:12 > + I wanted to write a test for this change, but I couldn't figure out how. try harder, or ask? :) > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:342 > + # FIXME: This method is complicated and needs unit testing! There are tests for this method in run_webkit_tests_unittest.py. See, for example, test_run_part() or test_run_single_file(). You will want to call the get_tests_run() helper function with your command line flag added, and then asser that the tests that ran are the tests you expected to run. > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:365 > + self._test_files -= failing I think this will still run flaky tests, which may or may not be what you want. I also had to double-check the code to see if FAIL would do what you want, i.e., if it actually included IMAGE, IMAGE+TEXT, CRASH, etc. as well as tests marked as FAIL. It looks like it does, but this is why you need tests. FAIL is a deprecated option that should go away at some point. You should also log something indicating that you're doing this, since this could easily confuse people otherwise. Something like self._printer.print_expected('Skipping failing tests') (see the example for --force on line 464). Thank you for looking Dirk! Comment on attachment 92461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92461&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:364 > + failing = self._expectations.get_tests_with_result_type( > + test_expectations.FAIL) This will only exclude tests marked "FAIL" in test_expectaitons. It won't exclude TEXT, IMAGE, IMAGE+TEXT, etc. As the code is now, you'll need to exclude each one. You can iterate through TestExpectations.EXPECTATIONS for all the values. Comment on attachment 92461 [details]
Patch
Whoops. NM. This code is...confusing.
(In reply to comment #6) > (From update of attachment 92461 [details]) > Whoops. NM. This code is...confusing. Hence my comments, and the need for tests to verify that it's doing what we actually want it to do. Committed r85927: <http://trac.webkit.org/changeset/85927> I added a test before landing. In the course of adding the tests I discovered that these tests are massively slow: https://bugs.webkit.org/show_bug.cgi?id=60350 |