Add an option to NRWT to skip failing tests
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