Bug 60295

Summary: Add an option to NRWT to skip failing tests
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Severity: Normal CC: dpranke, eric, mihaip, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch ojan: review+

Description Adam Barth 2011-05-05 13:53:00 PDT
Add an option to NRWT to skip failing tests
Comment 1 Adam Barth 2011-05-05 13:55:43 PDT
Created attachment 92461 [details]
Comment 2 Eric Seidel (no email) 2011-05-05 14:12:37 PDT
Comment on attachment 92461 [details]

Seems reasonable to me.
Comment 3 Dirk Pranke 2011-05-05 14:16:58 PDT
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).
Comment 4 Eric Seidel (no email) 2011-05-05 14:32:09 PDT
Thank you for looking Dirk!
Comment 5 Ojan Vafai 2011-05-05 14:37:15 PDT
Comment on attachment 92461 [details]

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 6 Ojan Vafai 2011-05-05 14:38:17 PDT
Comment on attachment 92461 [details]

Whoops. NM. This code is...confusing.
Comment 7 Dirk Pranke 2011-05-05 14:40:21 PDT
(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.
Comment 8 Adam Barth 2011-05-06 00:33:51 PDT
Committed r85927: <http://trac.webkit.org/changeset/85927>
Comment 9 Adam Barth 2011-05-06 00:44:10 PDT
I added a test before landing.  In the course of adding the tests I discovered that these tests are massively slow: