Summary: | [Testing] Add support for per-directory overriding of testing options via new TestOptions.txt files | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||||||||||||
Component: | Tools / Tests | Assignee: | Sam Weinig <sam> | ||||||||||||||||||||||||
Status: | NEW --- | ||||||||||||||||||||||||||
Severity: | Normal | CC: | ap, clopez, darin, eric.carlson, ews-watchlist, gsnedders, hi, jbedard, simon.fraser, webkit-bug-importer, youennf | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Bug Depends on: | 217390 | ||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||
Attachments: |
|
Description
Sam Weinig
2020-10-04 09:29:58 PDT
Created attachment 410471 [details]
Patch
Created attachment 410472 [details]
Patch
Aw, bummer, looks like some of builders don't support <filesystem>. I guess I have two options. Add a <filesystem> shim like I have done in the past for <optional> and <variant> (among others), or just re-write the bit of this that uses <filesystem> to use something else. Going to probably try the shim. Created attachment 410474 [details]
Patch
(In reply to Sam Weinig from comment #4) > Created attachment 410474 [details] > Patch Proof of concept <filesystem> shim (should only ever be needed on macOS). If this works, will move it WTF in a separate change. Created attachment 410476 [details]
Patch
Created attachment 410479 [details]
Patch
Although I hate the general idea of special case directories, moving from C++ code in WebKitTestRunner to TestOptions.txt is an improvement. One thing to consider here is to relaunch WebKitTestRunner and DumpRenderTree when moving to a directory with different options. There is fragile code in WKTR for re-creating a view and relaunching WebContent process when options change, which is also largely incompatible with DRT. > // This file is dual licensed under the MIT and the University of Illinois Open > // Source Licenses. See LICENSE.TXT for details. Are these licenses allowed in our repo? They are not listed on the patch upload page: If you are sending in a new file for inclusion in WebKit (no code copied from another source), the preferred license is BSD, but LGPL 2.1 is an option as well. Please include your copyright (name and year) and license preference (BSD or LGPL 2.1). By clicking below you agree that your file is licensed under either the BSD license or LGPL 2.1, as indicated in your file. > return pathContains(pathOrURL, "viewport/") && !pathContains(pathOrURL, "visual-viewport/"); I'm surprised that fast/visual-viewport tests are passing without TestOptions > webkit-test-runner [ isSVGTest=true ] This needs a better name. SVG has nothing to do with the effect it has, and there are lots of SVG tests that don't need this option. (In reply to Alexey Proskuryakov from comment #8) > Although I hate the general idea of special case directories, moving from > C++ code in WebKitTestRunner to TestOptions.txt is an improvement. Yeah, I'm not a fan of more manifests either. We may want to limit the use to only imported test suites, since all others should probably be able to use the comment based test headers. > > One thing to consider here is to relaunch WebKitTestRunner and > DumpRenderTree when moving to a directory with different options. There is > fragile code in WKTR for re-creating a view and relaunching WebContent > process when options change, which is also largely incompatible with DRT. This change only currently changes WTR, and only allows view/context reuse if the TestOptions match. It's almost the same as the old code, but a little more pessamistic since it no longer considers no value and default value to be the same thing. That said, it doesn't actually result in any change of view/context reuse in actual tests we have. > > > // This file is dual licensed under the MIT and the University of Illinois Open > > // Source Licenses. See LICENSE.TXT for details. > > Are these licenses allowed in our repo? They are not listed on the patch > upload page: > > If you are sending in a new file for inclusion in WebKit (no code copied > from another source), the preferred license is BSD, but LGPL 2.1 is an > option as well. Please include your copyright (name and year) and license > preference (BSD or LGPL 2.1). By clicking below you agree that your file is > licensed under either the BSD license or LGPL 2.1, as indicated in your file. The patch is not up for review yet (hence no r?). Just testing something out. That said, the MIT license is usually considered to be comparable with BSD in this regard, and I believe we have historically allowed copying code in from projects with MIT licenses (though I may be mistaken in my memory). > > > return pathContains(pathOrURL, "viewport/") && !pathContains(pathOrURL, "visual-viewport/"); > > I'm surprised that fast/visual-viewport tests are passing without TestOptions Note the ! for visual-viewport/. > > > webkit-test-runner [ isSVGTest=true ] > > This needs a better name. SVG has nothing to do with the effect it has, and > there are lots of SVG tests that don't need this option. Yeah, it's a bad name. Should probably just add the ability to override width and height, which is what it actually wants. > > > // This file is dual licensed under the MIT and the University of Illinois Open
> > > // Source Licenses. See LICENSE.TXT for details.
> >
> > Are these licenses allowed in our repo? They are not listed on the patch
> > upload page:
> >
> > If you are sending in a new file for inclusion in WebKit (no code copied
> > from another source), the preferred license is BSD, but LGPL 2.1 is an
> > option as well. Please include your copyright (name and year) and license
> > preference (BSD or LGPL 2.1). By clicking below you agree that your file is
> > licensed under either the BSD license or LGPL 2.1, as indicated in your file.
>
> The patch is not up for review yet (hence no r?). Just testing something
> out. That said, the MIT license is usually considered to be comparable with
> BSD in this regard, and I believe we have historically allowed copying code
> in from projects with MIT licenses (though I may be mistaken in my memory).
Doing a quip grep of the source code looks like we have a quite a bit of MIT source (see WebInspectorUI/UserInterface/External/CodeMirror, ThirdParty/ANGLE, ThirdParty/libwebrtc, etc.)
Created attachment 410482 [details]
Patch
Created attachment 410642 [details]
Patch
Comment on attachment 410642 [details]
Patch
Most of this is a refactor, so one option would be to just land the refactor, and then discuss the per-directory TestOptions.txt files in a different bug.
(In reply to Sam Weinig from comment #13) > Most of this is a refactor, so one option would be to just land the > refactor, and then discuss the per-directory TestOptions.txt files in a > different bug. I like that idea. (FWIW, I won’t have a chance to review this until this evening.) (In reply to Darin Adler from comment #14) > (In reply to Sam Weinig from comment #13) > > Most of this is a refactor, so one option would be to just land the > > refactor, and then discuss the per-directory TestOptions.txt files in a > > different bug. > > I like that idea. (FWIW, I won’t have a chance to review this until this > evening.) Ok. Cool. I extracted most of it in a patch for https://bugs.webkit.org/show_bug.cgi?id=217390. No rush on a timeline. Created attachment 410759 [details]
Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess Created attachment 410760 [details]
Patch
Ok, the patch is now tiny (in comparison), so we can have a more concentrated conversation about the change. There are two goals here: 1) Remove hardcoded, path based, configuration of tests that lives in WebKitTestRunner. 2) Add a way to set options on a test without modifying the test itself, for imported test cases like those from WPT that we don't want to modify. Some of the changes for #1 we could do by modifying a bunch of tests (e.g. we could update all the tests in fast/viewports/ to specify useFlexibleViewport=true, and perhaps we should), but we couldn't do all of them, since the majority affect imported tests. Created attachment 410761 [details]
Patch
Created attachment 411058 [details]
Patch
|