Bug 181589 - [GTK][WPE] Add support for unit test expectations
Summary: [GTK][WPE] Add support for unit test expectations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, InRadar
Depends on:
Blocks:
 
Reported: 2018-01-12 06:11 PST by Carlos Garcia Campos
Modified: 2018-01-15 00:19 PST (History)
7 users (show)

See Also:


Attachments
Patch (35.35 KB, patch)
2018-01-12 06:20 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-01-12 06:11:47 PST
We currently have a way to skip tests by annotating them in the api test runner script. The main problem of this approach is that we skip test when they fail in the bots and we never notice if they stop failing, keeping the tests skipped forever. This is indeed the case of several WebKit2 C API tests. Annotating skipped tests in the script itself is not a good idea either.
Comment 1 Carlos Garcia Campos 2018-01-12 06:20:50 PST
Created attachment 331195 [details]
Patch

This will also allow us to handle flaky tests, by re-running the unexpected failures.
Comment 2 Michael Catanzaro 2018-01-13 09:23:33 PST
Comment on attachment 331195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331195&action=review

I haven't looked in detail at the python code, but it looks sane. Bonus points for including unit tests.

I don't really like json, but this is clearly superior to what we had before. Thanks.

The next step would be to update the expectations for WPE, since the WPE API tests are in real bad shape.

I would name the file TestExpectations.json, instead of expectations.json, since it will be more familiar to WebKit developers. The purpose of the file should then be immediately obvious.

> Tools/ChangeLog:10
> +        We currently have a way to skip tests by annotating them in the api test runner script. The main problem of this
> +        approach is that we skip test when they fail in the bots and we never notice if they stop failing, keeping the
> +        tests skipped forever. This is indeed the case of several WebKit2 C API tests. Annotating skipped tests in the

I think it's understood that skipped tests are broken and need to be fixed. We just need to actually start fixing them instead of ignoring them....

> Tools/Scripts/webkitpy/common/test_expectations_unittest.py:33
> +class MockTestExpectations(TestExpectations):

Are these tests run automatically, as part of testwebkitpy? If not, please find a way to make sure they are. Nobody (except maybe you) is ever going to run this manually: having tests is not worthwhile unless they're running on the bots.

IMO it's fine to run this test on all platforms.
Comment 3 Carlos Garcia Campos 2018-01-14 00:48:06 PST
(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 331195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331195&action=review
> 
> I haven't looked in detail at the python code, but it looks sane. Bonus
> points for including unit tests.
> 
> I don't really like json, but this is clearly superior to what we had
> before. Thanks.
> 
> The next step would be to update the expectations for WPE, since the WPE API
> tests are in real bad shape.

Yes, that's my idea.

> I would name the file TestExpectations.json, instead of expectations.json,
> since it will be more familiar to WebKit developers. The purpose of the file
> should then be immediately obvious.

Ok.

> > Tools/ChangeLog:10
> > +        We currently have a way to skip tests by annotating them in the api test runner script. The main problem of this
> > +        approach is that we skip test when they fail in the bots and we never notice if they stop failing, keeping the
> > +        tests skipped forever. This is indeed the case of several WebKit2 C API tests. Annotating skipped tests in the
> 
> I think it's understood that skipped tests are broken and need to be fixed.
> We just need to actually start fixing them instead of ignoring them....

That's not how things work unfortunately. Very often we skip unit tests that stop passing for a unknown reason only to keep the bots green, sometimes it even only fails on the bots and work locally. Then, the test suddenly start passing again for another unknown reason, but since it's skipped we never notice it. It has happened so many times, all WebKit2 C API tests currently skipped pass for me locally, and I bet they also pass in the bots, we'll see when this patch lands.

> > Tools/Scripts/webkitpy/common/test_expectations_unittest.py:33
> > +class MockTestExpectations(TestExpectations):
> 
> Are these tests run automatically, as part of testwebkitpy? If not, please
> find a way to make sure they are. Nobody (except maybe you) is ever going to
> run this manually: having tests is not worthwhile unless they're running on
> the bots.
> 
> IMO it's fine to run this test on all platforms.

Yes, they are run automatically for all platforms.
Comment 4 Carlos Garcia Campos 2018-01-15 00:17:55 PST
Committed r226944: <https://trac.webkit.org/changeset/226944>
Comment 5 Radar WebKit Bug Importer 2018-01-15 00:19:54 PST
<rdar://problem/36514890>