[webkit-reviews] review granted: [Bug 181589] [GTK][WPE] Add support for unit test expectations : [Attachment 331195] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 181589: [GTK][WPE] Add support for unit test expectations
https://bugs.webkit.org/show_bug.cgi?id=181589

Attachment 331195: Patch

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




--- Comment #2 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 331195
  --> https://bugs.webkit.org/attachment.cgi?id=331195
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.