WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Patch
(101.30 KB, patch)
2020-10-04 10:36 PDT
,
Sam Weinig
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(267.35 KB, patch)
2020-10-04 11:24 PDT
,
Sam Weinig
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(267.41 KB, patch)
2020-10-04 11:35 PDT
,
Sam Weinig
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(258.01 KB, patch)
2020-10-04 12:17 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(269.66 KB, patch)
2020-10-04 14:07 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(104.12 KB, patch)
2020-10-06 08:50 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(38.60 KB, patch)
2020-10-07 10:39 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(18.17 KB, patch)
2020-10-07 10:58 PDT
,
Sam Weinig
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(18.23 KB, patch)
2020-10-07 11:05 PDT
,
Sam Weinig
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(16.77 KB, patch)
2020-10-11 11:48 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Show Obsolete
(11)
View All
Add attachment
proposed patch, testcase, etc.
Sam Weinig
Comment 1
2020-10-04 10:30:16 PDT
Comment hidden (obsolete)
Created
attachment 410471
[details]
Patch
Sam Weinig
Comment 2
2020-10-04 10:36:02 PDT
Comment hidden (obsolete)
Created
attachment 410472
[details]
Patch
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)
Created
attachment 410474
[details]
Patch
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)
Created
attachment 410476
[details]
Patch
Sam Weinig
Comment 7
2020-10-04 12:17:36 PDT
Created
attachment 410479
[details]
Patch
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
Created
attachment 410482
[details]
Patch
Sam Weinig
Comment 12
2020-10-06 08:50:21 PDT
Created
attachment 410642
[details]
Patch
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)
Created
attachment 410759
[details]
Patch
EWS Watchlist
Comment 17
2020-10-07 10:40:12 PDT
Comment hidden (obsolete)
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
Sam Weinig
Comment 18
2020-10-07 10:58:27 PDT
Comment hidden (obsolete)
Created
attachment 410760
[details]
Patch
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
Created
attachment 410761
[details]
Patch
Radar WebKit Bug Importer
Comment 21
2020-10-11 09:30:16 PDT
<
rdar://problem/70183206
>
Sam Weinig
Comment 22
2020-10-11 11:48:00 PDT
Created
attachment 411058
[details]
Patch
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug