Bug 187003

Summary: Add support for WPT manifests
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: clopez, lforschler, Ms2ger, rego, rniwa, youennf, zan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=186132
https://bugs.webkit.org/show_bug.cgi?id=82245
https://bugs.webkit.org/show_bug.cgi?id=203784
https://bugs.webkit.org/show_bug.cgi?id=204469

Description Frédéric Wang (:fredw) 2018-06-25 09:12:12 PDT
On 01/06/2018 19:50, Robert Ma wrote:
> Chromium stopped relying on magic filenames for WPT reftests a long time ago. We now have WPT manifest support built into run_webkit_tests: if a test is in WPT, we get its reference from the WPT manifest instead of mangling the filename. The logic is hooked into the base port (not sure how similar our code bases are nowadays, but you'll get the idea).
>
> Hence, there's no renaming during import or export.
>
> I'd suggest you to investigate supporting WPT manifest properly, for the following reasons:
>
>     WPT reftests can be complicated. Each test can have multiple match/mismatch references. The relationship actually forms a digraph. It's much easier/reliable to use the manifest than parsing the <link> tags.
>     Testharness tests can have some metadata like long timeout and variants (one test file expands to multiple tests), which are also encoded in the manifest.
>
> The WPT manifest Python module is 2/3 compatible with limited dependencies and it should be possible to import it to webkitpy.
Comment 1 Frédéric Wang (:fredw) 2018-06-27 04:26:20 PDT
Some more references about expectation manifest:
https://wptrunner.readthedocs.io/en/latest/expectation.html#expectation-manifests
https://github.com/web-platform-tests/wpt/tree/master/tools/wptrunner#expectation-data

This is supported by upstream WPT but also by runners from Mozilla and Chromium repositories.

As mentioned in comment 0, this avoids the renaming of -ref.html => -expected.html for reftests, which is causing issues like bug 186132.

In addition, this can also avoid the need for -expected.txt files for testharness.js tests (bug 82245).
Comment 2 youenn fablet 2018-06-27 07:48:38 PDT
(In reply to Frédéric Wang (:fredw) from comment #1)
> Some more references about expectation manifest:
> https://wptrunner.readthedocs.io/en/latest/expectation.html#expectation-
> manifests
> https://github.com/web-platform-tests/wpt/tree/master/tools/
> wptrunner#expectation-data
> 
> This is supported by upstream WPT but also by runners from Mozilla and
> Chromium repositories.
> 
> As mentioned in comment 0, this avoids the renaming of -ref.html =>
> -expected.html for reftests, which is causing issues like bug 186132.

This is a pain point and I agree aligning with WPT makes sense for ref tests.
We can probably generate/update the manifest at import time and use it to know whether a WPT test is a ref test or not.

We could later extend it to determine the list of WPT tests to run, but that can be done/discussed as a follow-up.

> In addition, this can also avoid the need for -expected.txt files for
> testharness.js tests (bug 82245).

I would tend to decorrelate this particular issue from the use of manifests.
Comment 3 Frédéric Wang (:fredw) 2018-06-27 10:18:44 PDT
(In reply to youenn fablet from comment #2)
> > In addition, this can also avoid the need for -expected.txt files for
> > testharness.js tests (bug 82245).
> 
> I would tend to decorrelate this particular issue from the use of manifests.

Yes, I agree this would be nice but being able to properly import/export reftests seems more important for now.
Comment 4 Ryosuke Niwa 2018-06-27 12:45:49 PDT
I don't think we should do this. There was a long discussion about it in webkit-dev. See https://lists.webkit.org/pipermail/webkit-dev/2011-November/018470.html
Comment 5 Frédéric Wang (:fredw) 2018-06-27 14:29:15 PDT
(In reply to Frédéric Wang (:fredw) from comment #1)
> Some more references about expectation manifest:
> https://wptrunner.readthedocs.io/en/latest/expectation.html#expectation-
> manifests
> https://github.com/web-platform-tests/wpt/tree/master/tools/
> wptrunner#expectation-data

I think I've been mixing up things, there are optional expectations manifests but there is also the generated MANIFEST.json ( https://wptrunner.readthedocs.io/en/latest/usage.html#id2 ). I think in most cases, just using MANIFEST.json will be enough to solve the issues we have with reftests.

(In reply to Ryosuke Niwa from comment #4)
> I don't think we should do this. There was a long discussion about it in
> webkit-dev. See
> https://lists.webkit.org/pipermail/webkit-dev/2011-November/018470.html

I wonder if this discussion is really up-to-date given Robert Ma's comment that Chromium finally decided to switch to WPT manifests instead of using the filename convention or link tags.

How do you plan to solve the issues with reftest import/export?
Comment 6 youenn fablet 2018-06-27 14:38:54 PDT
> I think I've been mixing up things, there are optional expectations
> manifests but there is also the generated MANIFEST.json (
> https://wptrunner.readthedocs.io/en/latest/usage.html#id2 ). I think in most
> cases, just using MANIFEST.json will be enough to solve the issues we have
> with reftests.

Right, the idea would be to read manifest.json from webkitpy whenever trying to run some WPT tests.
That would give us the reference files that should be validated for a given test.
That does seem ok to me.

I do not know how much this is used but I think WPT allows to check a given test against several references. This would probably require more work on webkitpy side.
Comment 7 Ryosuke Niwa 2018-06-27 14:56:36 PDT
(In reply to youenn fablet from comment #6)
> > I think I've been mixing up things, there are optional expectations
> > manifests but there is also the generated MANIFEST.json (
> > https://wptrunner.readthedocs.io/en/latest/usage.html#id2 ). I think in most
> > cases, just using MANIFEST.json will be enough to solve the issues we have
> > with reftests.
>
> I do not know how much this is used but I think WPT allows to check a given
> test against several references. This would probably require more work on
> webkitpy side.

Darin objected to this kind of solution in the past and I have no reason to believe his opinion has changed. There are a number of others who have objected to respecting manifest.json in the past.

Regardless, if you strongly feel about this matter, you should start a new webkit-dev thread.