NEW 217297
[Testing] Add support for per-directory overriding of testing options via new TestOptions.txt files
https://bugs.webkit.org/show_bug.cgi?id=217297
Summary [Testing] Add support for per-directory overriding of testing options via new...
Sam Weinig
Reported 2020-10-04 09:29:58 PDT
[Testing] Add support for per-directory overriding of testing options via new TestOptions.txt files
Attachments
Patch (101.32 KB, patch)
2020-10-04 10:30 PDT, Sam Weinig
ews-feeder: commit-queue-
Patch (101.30 KB, patch)
2020-10-04 10:36 PDT, Sam Weinig
ews-feeder: commit-queue-
Patch (267.35 KB, patch)
2020-10-04 11:24 PDT, Sam Weinig
ews-feeder: commit-queue-
Patch (267.41 KB, patch)
2020-10-04 11:35 PDT, Sam Weinig
ews-feeder: commit-queue-
Patch (258.01 KB, patch)
2020-10-04 12:17 PDT, Sam Weinig
no flags
Patch (269.66 KB, patch)
2020-10-04 14:07 PDT, Sam Weinig
no flags
Patch (104.12 KB, patch)
2020-10-06 08:50 PDT, Sam Weinig
no flags
Patch (38.60 KB, patch)
2020-10-07 10:39 PDT, Sam Weinig
no flags
Patch (18.17 KB, patch)
2020-10-07 10:58 PDT, Sam Weinig
ews-feeder: commit-queue-
Patch (18.23 KB, patch)
2020-10-07 11:05 PDT, Sam Weinig
ews-feeder: commit-queue-
Patch (16.77 KB, patch)
2020-10-11 11:48 PDT, Sam Weinig
no flags
Sam Weinig
Comment 1 2020-10-04 10:30:16 PDT Comment hidden (obsolete)
Sam Weinig
Comment 2 2020-10-04 10:36:02 PDT Comment hidden (obsolete)
Sam Weinig
Comment 3 2020-10-04 10:58:59 PDT
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.
Sam Weinig
Comment 4 2020-10-04 11:24:43 PDT Comment hidden (obsolete)
Sam Weinig
Comment 5 2020-10-04 11:25:47 PDT
(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.
Sam Weinig
Comment 6 2020-10-04 11:35:23 PDT Comment hidden (obsolete)
Sam Weinig
Comment 7 2020-10-04 12:17:36 PDT
Alexey Proskuryakov
Comment 8 2020-10-04 13:11:08 PDT
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.
Sam Weinig
Comment 9 2020-10-04 13:45:21 PDT
(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.
Sam Weinig
Comment 10 2020-10-04 13:52:54 PDT
> > > // 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.)
Sam Weinig
Comment 11 2020-10-04 14:07:01 PDT
Sam Weinig
Comment 12 2020-10-06 08:50:21 PDT
Sam Weinig
Comment 13 2020-10-06 09:14:12 PDT
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.
Darin Adler
Comment 14 2020-10-06 09:29:22 PDT
(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.)
Sam Weinig
Comment 15 2020-10-06 09:52:49 PDT
(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.
Sam Weinig
Comment 16 2020-10-07 10:39:26 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 17 2020-10-07 10:40:12 PDT Comment hidden (obsolete)
Sam Weinig
Comment 18 2020-10-07 10:58:27 PDT Comment hidden (obsolete)
Sam Weinig
Comment 19 2020-10-07 11:02:52 PDT
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.
Sam Weinig
Comment 20 2020-10-07 11:05:08 PDT
Radar WebKit Bug Importer
Comment 21 2020-10-11 09:30:16 PDT
Sam Weinig
Comment 22 2020-10-11 11:48:00 PDT
Note You need to log in before you can comment on or make changes to this bug.