Bug 60295 - Add an option to NRWT to skip failing tests
Summary: Add an option to NRWT to skip failing tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-05 13:53 PDT by Adam Barth
Modified: 2011-05-06 00:44 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.08 KB, patch)
2011-05-05 13:55 PDT, Adam Barth
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Patch
Comment 2 Eric Seidel (no email) 2011-05-05 14:12:37 PDT
Comment on attachment 92461 [details]
Patch

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

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:

https://bugs.webkit.org/show_bug.cgi?id=60350