Bug 217297 – [Testing] Add support for per-directory overriding of testing options via new TestOptions.txt files

• Component: Tools / Tests
• Status: NEW

[Sam Weinig]

[Testing] Add support for per-directory overriding of testing options via new TestOptions.txt files

[Sam Weinig]

Created attachment 410471 [details]
Patch

[Sam Weinig]

Created attachment 410472 [details]
Patch

[Sam Weinig]

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]

Created attachment 410474 [details]
Patch

[Sam Weinig]

(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]

Created attachment 410476 [details]
Patch

[Sam Weinig]

Created attachment 410479 [details]
Patch

[Alexey Proskuryakov]

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]

(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]

> > > // 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]

Created attachment 410482 [details]
Patch

[Sam Weinig]

Created attachment 410642 [details]
Patch

[Sam Weinig]

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]

(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]

(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]

Created attachment 410759 [details]
Patch

[EWS Watchlist]

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]

Created attachment 410760 [details]
Patch

[Sam Weinig]

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]

Created attachment 410761 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/70183206>

[Sam Weinig]

Created attachment 411058 [details]
Patch