Bug 217297

Summary: [Testing] Add support for per-directory overriding of testing options via new TestOptions.txt files
Product: WebKit Reporter: Sam Weinig <sam>
Component: Tools / TestsAssignee: 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 Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch none

Description Sam Weinig 2020-10-04 09:29:58 PDT
[Testing] Add support for per-directory overriding of testing options via new TestOptions.txt files
Comment 1 Sam Weinig 2020-10-04 10:30:16 PDT Comment hidden (obsolete)
Comment 2 Sam Weinig 2020-10-04 10:36:02 PDT Comment hidden (obsolete)
Comment 3 Sam Weinig 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.
Comment 4 Sam Weinig 2020-10-04 11:24:43 PDT Comment hidden (obsolete)
Comment 5 Sam Weinig 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.
Comment 6 Sam Weinig 2020-10-04 11:35:23 PDT Comment hidden (obsolete)
Comment 7 Sam Weinig 2020-10-04 12:17:36 PDT
Created attachment 410479 [details]
Patch
Comment 8 Alexey Proskuryakov 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.
Comment 9 Sam Weinig 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.
Comment 10 Sam Weinig 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.)
Comment 11 Sam Weinig 2020-10-04 14:07:01 PDT
Created attachment 410482 [details]
Patch
Comment 12 Sam Weinig 2020-10-06 08:50:21 PDT
Created attachment 410642 [details]
Patch
Comment 13 Sam Weinig 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.
Comment 14 Darin Adler 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.)
Comment 15 Sam Weinig 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.
Comment 16 Sam Weinig 2020-10-07 10:39:26 PDT Comment hidden (obsolete)
Comment 17 EWS Watchlist 2020-10-07 10:40:12 PDT Comment hidden (obsolete)
Comment 18 Sam Weinig 2020-10-07 10:58:27 PDT Comment hidden (obsolete)
Comment 19 Sam Weinig 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.
Comment 20 Sam Weinig 2020-10-07 11:05:08 PDT
Created attachment 410761 [details]
Patch
Comment 21 Radar WebKit Bug Importer 2020-10-11 09:30:16 PDT
<rdar://problem/70183206>
Comment 22 Sam Weinig 2020-10-11 11:48:00 PDT
Created attachment 411058 [details]
Patch