Bug 60295 – Add an option to NRWT to skip failing tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Add an option to NRWT to skip failing tests

[Adam Barth]

Created attachment 92461 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 92461 [details]
Patch

Seems reasonable to me.

[Dirk Pranke]

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

[Eric Seidel (no email)]

Thank you for looking Dirk!

[Ojan Vafai]

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.

[Ojan Vafai]

Comment on attachment 92461 [details]
Patch

Whoops. NM. This code is...confusing.

[Dirk Pranke]

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

[Adam Barth]

Committed r85927: <http://trac.webkit.org/changeset/85927>

[Adam Barth]

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