Bug 203784 – Add support for finding test references via <link rel="match"> and use that in WPT

• Status: ASSIGNED
• Component: Tools / Tests

[Simon Fraser (smfr)]

We should parse test files to look for <link rel="match"> to support WPT-style ref tests. Right now we have to clone a bunch of files on import.

[Simon Fraser (smfr)]

Created attachment 382739 [details]
Patch

[Simon Fraser (smfr)]

Also need to:
1. Fix wpt import script to not copy the refs to -expected*
2. Warn about removing existing -expected.html on new imports and/or do the removal.
3. When both a ref and expected exist, compare the file contents?

[Simon Fraser (smfr)]

Created attachment 382896 [details]
Patch

[youenn fablet]

Comment on attachment 382896 [details]
Patch

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

> Tools/ChangeLog:14
> +        as reference files. On import, existing -expected.html files will be removed.

Is it implemented in this patch? If so, can we add a unit test?

> Tools/Scripts/webkitpy/port/base.py:65
> +from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup, SoupStrainer

I believe WPT is computing this in LayoutTests/imported/w3c/web-platform-tests/tools/manifest/sourcefile.py.
They use html5lib. In the long run, maybe we should just copy LayoutTests/imported/w3c/web-platform-tests/tools python scripts directly.

> Tools/Scripts/webkitpy/port/base.py:-538
> -        result = self._get_reftest_list(test_name)

Can we remove def _get_reftest_list?

> Tools/Scripts/webkitpy/port/base.py:550
> +                _log.warning('Multiple references are not supported. Importing the first ref defined in {}'.format(test_name))

I did not know about the previous reftest list support in webkitpy. Are we able to support testing multiple reference files?
The warning message could be either updated or removed.

> Tools/Scripts/webkitpy/port/base.py:584
> +    def reference_files(self, test_name, device_type=None):

Can we add unit tests for this method?

[Simon Fraser (smfr)]

(In reply to youenn fablet from comment #4)
> Comment on attachment 382896 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382896&action=review
> 
> > Tools/ChangeLog:14
> > +        as reference files. On import, existing -expected.html files will be removed.
> 
> Is it implemented in this patch? If so, can we add a unit test?

That already happens on import (non-tracked filed are deleted).

> > Tools/Scripts/webkitpy/port/base.py:65
> > +from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup, SoupStrainer
> 
> I believe WPT is computing this in
> LayoutTests/imported/w3c/web-platform-tests/tools/manifest/sourcefile.py.
> They use html5lib. In the long run, maybe we should just copy
> LayoutTests/imported/w3c/web-platform-tests/tools python scripts directly.

BeautifulSoup uses html5lib. This is like the code in TestParser, but we don't need a full DOM for each test, just the <link> tags. We have to run this on every test, so it needs to be efficient, which is why I didn't just use TestParser.

> > Tools/Scripts/webkitpy/port/base.py:-538
> > -        result = self._get_reftest_list(test_name)
> 
> Can we remove def _get_reftest_list?

Yes: webkit.org/b/203783

> > Tools/Scripts/webkitpy/port/base.py:550
> > +                _log.warning('Multiple references are not supported. Importing the first ref defined in {}'.format(test_name))
> 
> I did not know about the previous reftest list support in webkitpy. Are we
> able to support testing multiple reference files?

No

> The warning message could be either updated or removed.

This is straight from TestParser.analyze_test()

> > Tools/Scripts/webkitpy/port/base.py:584
> > +    def reference_files(self, test_name, device_type=None):
> 
> Can we add unit tests for this method?

Sure. Ideally this code wouldn't live in base.py. I tried moving it, and got hung up on some MockDRT stuff.

[Simon Fraser (smfr)]

Created attachment 383107 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 383108 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 383132 [details]
Patch

[Ryosuke Niwa]

(In reply to Simon Fraser (smfr) from comment #0)
> We should parse test files to look for <link rel="match"> to support
> WPT-style ref tests. Right now we have to clone a bunch of files on import.

I don't think we should do this. There was a long discussion about avoiding this so that we don't have a graph of tests referencing one another.

[youenn fablet]

(In reply to Ryosuke Niwa from comment #9)
> (In reply to Simon Fraser (smfr) from comment #0)
> > We should parse test files to look for <link rel="match"> to support
> > WPT-style ref tests. Right now we have to clone a bunch of files on import.
> 
> I don't think we should do this. There was a long discussion about avoiding
> this so that we don't have a graph of tests referencing one another.

What is the exact issue here?
Since we will continue running 1-1 only, running the tests seems equivalent.
For instance, we might be running the same test twice (A vs.B and B vs. A) with both approaches but with the patch, it might be easier to detect it.

[youenn fablet]

Comment on attachment 383132 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:550
> +                _log.warning('Multiple references are not supported. Importing the first ref defined in {}'.format(test_name))

This code is coming from TestParser whose purpose is to import tests.
The purpose of this code is to run tests, so we should update the message accordingly.
Or maybe just remove the warning altogether since we know this is a limitation of webkitpy.

[youenn fablet]

LGTM to me but I'd like to hear about Ryosuke issues before we land the patch.

[Ryosuke Niwa]

(In reply to youenn fablet from comment #10)
> (In reply to Ryosuke Niwa from comment #9)
> > (In reply to Simon Fraser (smfr) from comment #0)
> > > We should parse test files to look for <link rel="match"> to support
> > > WPT-style ref tests. Right now we have to clone a bunch of files on import.
> > 
> > I don't think we should do this. There was a long discussion about avoiding
> > this so that we don't have a graph of tests referencing one another.
> 
> What is the exact issue here?

The issue is that this makes it hard to find the expected result, mismatch, and create unwarranted dependency between files. For example, you can use another test as the expected result. If you modified the test due to a code change, a random other test may start failing.

See this thread: https://lists.webkit.org/pipermail/webkit-dev/2011-November/018470.html

[Ryosuke Niwa]

Comment on attachment 383132 [details]
Patch

I don't think we should land this patch until a discussion on webkit-dev happens.

[Ryosuke Niwa]

Also ee https://bugs.webkit.org/show_bug.cgi?id=66295

[youenn fablet]

We might think of temporary workarounds then.
One of which could be to have the -ref.html file be a redirect to the actual reference file if the reference file is not in the same directory.
This could be done by the test importer.

[Simon Fraser (smfr)]

I mailed webkit-dev. Since WPT use <link rel=> so extensively now, I think we should reconsider.

[Ryosuke Niwa]

(In reply to Simon Fraser (smfr) from comment #17)
> I mailed webkit-dev. Since WPT use <link rel=> so extensively now, I think
> we should reconsider.

I think reconsidering the decision is good. After all, I was the one who proposed this change last time around. Replied in the thread.

[Simon Fraser (smfr)]

Created attachment 383201 [details]
Patch

[zalan]

Comment on attachment 383201 [details]
Patch

rs=me

[Alexey Proskuryakov]

Comment on attachment 383201 [details]
Patch

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

> Tools/ChangeLog:3
> +        Add support for finding test references via <link rel="match">

Catching up on e-mail thread, I see that there is a ~3% overhead for test runtime. That's kind of outrageous, let's not land this then.

[Carlos Alberto Lopez Perez]

Comment on attachment 383201 [details]
Patch

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

> Tools/ChangeLog:10
> +        When looking for test references, we now check for a <link rel="match">
> +        or <link rel="mismatch"> before looking in the filesystem for "-expected" files.
> +        A warning is logged if both exist.

The performance hit (3%?) is likely to be much lower if the order of searching for expected results is inverted.

[Simon Fraser (smfr)]

Comment on attachment 383201 [details]
Patch

I think what we should do is generate MANIFEST.json in imported/w3c/web-platform-tests at import time, and commit it, then use it to find references just for wpt.

[Frédéric Wang (:fredw)]

(In reply to Simon Fraser (smfr) from comment #23)
> Comment on attachment 383201 [details]
> Patch
> 
> I think what we should do is generate MANIFEST.json in
> imported/w3c/web-platform-tests at import time, and commit it, then use it
> to find references just for wpt.

I think some people proposed a workflow where we add WPT tests in WebKit and then use the export script to open github PR. I'm not sure this approach is really used, but if that's the case, these people should also be sure to re-generate MANIFEST.json

[Radar WebKit Bug Importer]

<rdar://problem/69332113>

[Ryosuke Niwa]

FWIW, we're not using this mechanism for non-WPT tests.

[Manuel Rego Casasnovas]

Since https://bugs.webkit.org/show_bug.cgi?id=234510 we're now importing the -ref files too, which makes us to have a lot of duplicated files using -expected.html & -ref.html at the same time.
See for example the following folder: https://github.com/WebKit/WebKit/tree/main/LayoutTests/imported/w3c/web-platform-tests/html/dom/elements/global-attributes

I believe it'd be really nice to fix this bug and get rid of -expected.html files on WPT imported folder. Otherwise we're going to end up duplicating most of the references as we sync different folders.

[David Kilzer (:ddkilzer)]

*** Bug 249720 has been marked as a duplicate of this bug. ***