Bug 203784

Created attachment 382739 [details] Patch

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?

Created attachment 382896 [details] Patch

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?

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

Created attachment 383107 [details] Patch

Created attachment 383108 [details] Patch

Created attachment 383132 [details] Patch

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

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

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.

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

(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

Comment on attachment 383132 [details] Patch I don't think we should land this patch until a discussion on webkit-dev happens.

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

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.

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

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

Created attachment 383201 [details] Patch

Comment on attachment 383201 [details] Patch rs=me

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.

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.

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.

(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

<rdar://problem/69332113>

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

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.

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

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