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

Reply via email to