On Fri, Nov 8, 2019 at 2:15 PM Simon Fraser <simon.fra...@apple.com> wrote: > > > On Nov 8, 2019, at 2:07 PM, Ryosuke Niwa <rn...@webkit.org> wrote: > > > > On Fri, Nov 8, 2019 at 2:01 PM Simon Fraser <simon.fra...@apple.com> wrote: > >> > >> I'd like to land a patch to support finding test references via <link > >> rel="match/mismatch">: > >> https://bugs.webkit.org/show_bug.cgi?id=203784 > >> > >> There has been some discussion about this in the past: > >> https://lists.webkit.org/pipermail/webkit-dev/2011-November/018470.html > >> > >> But I think the benefits outweigh the drawbacks. As that mail states: > >> > >> *Link element approach* > >> Pros: > >> > >> - Can reuse same ref. file for multiple tests > >> > >> Still true. > >> > >> - Can have multiple ref. files for single test > >> > >> True but no something that we support, and I haven't see any WPT use this > >> (our importer throws an error if it sees this) > >> > >> - Information is self-contained in the test file > >> > >> Still true > >> > >> - We may get away with test suite build step > >> > >> It certainly simplifies WPT test import. > >> > >> Currently importing some CSS suites (e.g. css-backgrounds) results in > >> broken -expected.html files because copying them breaks references to sub > >> resources. > >> > >> (It turns out that we can't convert W3C ref tests to use WebKit conventions > >> due to the first two points.) > >> > >> We're doing this much more now, and the "multiple references" point is > >> moot, so I think we can import WPT tests mostly as-is. > >> > >> Cons: > >> > >> - Requires us modifying each port's DRT to support this format > >> > >> No, it just requires webkitpy hacking which I've done in the patch. > > > > I'm not certain writing a bunch of regular expressions in webkitpy is > > a reliable mechanism to find expected results. Another issue I found > > back then was that it significantly slowed run-webkit-tests' startup > > time because WPT has a workflow to find all tests & their expected > > results upfront before any tests could run. > > The patch uses html5lib (via BeautifulSoup), which is exactly what WPT, and > our importer use to find the ref tests. > > We don't find references up-front; only when running each test. This patch > does add some overhead for parsing each test file, > which I measured to be about 1-2 sec on a directory which took 30s to run. I > think this slight slowdown is worthwhile (we could > probably eliminate it with some webkitpy optimizations).
Hm... that's ~3% overhead. > >> - Adding link elements itself may affect tests (all W3C tests are > >> required to have link elements at the moment) > >> > >> I haven't seen this be an issue. > > > > Another issue is that if you were to modify a test which happens to be > > also used as a reference or a mismatch result (worse) for some other > > test, then you may not notice that without inspecting every other test > > in existence. > > EWS will tell you. Also, generally a file won't act as both a test and a > reference; I don't see us deviating from > our current "-expected.html" naming conventions. BTW, you don't *have* to add > a <link> tag; we'll still fall > back to the current search behavior. If you have both, then webkitpy will > warn. I think we should enforce that in our tooling then. > >> - Hard to understand relationship between files. e.g. if we want to > >> figure out which tests use ref.html, we must look at all test files > >> > >> This is true, but I don't really see it being a problem in practice. > > > > This definitely is an issue. It's possible WPT has improved things but > > we've definitely had an experience where tests were used as reference > > for other tests, etc... and having to think about this issue every > > time I touch test drove me nuts. > > Do you have any examples? If this does happen in WPT, we should discourage it > (and can fix it via PRs). Oh yeah, it's discouraged but it still happens. If we're doing this in WebKit, run-webkit-tests should outright refuse to run such tests. > Generally references live in a resources/ or references/ subdirectory, or > have "-ref" in the name. We need to enforce that in tool. > >> What I have seen is us importing CSS 2.1 tests that have foo.html and > >> foo-ref.html, and treating foo-ref.html as a test so generating > >> foo-expected.txt and foo-ref-expected.txt. That seems worse. > > > > Seems like we can treat "-ref" as a special suffix like we already do > > with support directory and resources directory. > > All that works now. Cool. Now that I think about it, I may have added that LOL. > >> So now that WPT is heavily invested in <link rel=> I think we should > >> follow suite. It will simplify WPT import, and reduced the number of > >> cloned -expected.html files significantly. > > > > I really don't want to deal with tests being used as references for > > other tests. I'm okay with this approach if we forbid that. > > I'm OK with that (enforced by code review unless we see the need for tooling). I think we should make run-webkit-tests not allow tests that use other tests as reference files, or ones that don't follow our file naming convention. - R. Niwa _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
