Bug 134763 – W3C test importer should have an option to disable testharness.js/testharnessreport.js link conversion

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[youenn fablet]

Some W3C tests are using functions present in W3C testharness.js, but not in WebKit version.
It may just be simpler to import the W3C version and update the test converter to point to that version.

The same may be done for testharnessreport.js, but WebKit version should overwrite the W3C version.
This would allow the WPT server to correctly send testharness.js and testharnessreport.js.

[youenn fablet]

Created attachment 234632 [details]
Patch

[youenn fablet]

Created attachment 235009 [details]
Added parameter to resources folder

[youenn fablet]

Created attachment 238159 [details]
Updating webkitpy tests

[youenn fablet]

Created attachment 238203 [details]
fixing import test executable

[youenn fablet]

Created attachment 238204 [details]
fixing import test executable

[Bem Jones-Bey]

(In reply to comment #0)
> Some W3C tests are using functions present in W3C testharness.js, but not in WebKit version.
> It may just be simpler to import the W3C version and update the test converter to point to that version.

Why don't we just update the webkit version with the newest upstream changes from the W3C?

> The same may be done for testharnessreport.js, but WebKit version should overwrite the W3C version.

Are there features in the W3C version of testharnessreport.js that the WPT tests need that aren't in the WebKit version?

> This would allow the WPT server to correctly send testharness.js and testharnessreport.js.

If we updated the WebKit versions testharness.js and testharnessreport.js, the WPT server could send those instead of having multiple copies in the tree, right?

[youenn fablet]

(In reply to comment #6)
> (In reply to comment #0)
> > Some W3C tests are using functions present in W3C testharness.js, but not in WebKit version.
> > It may just be simpler to import the W3C version and update the test converter to point to that version.
> 
> Why don't we just update the webkit version with the newest upstream changes from the W3C?

That is a possibility, but there is always a risk that legacy WebKit tests break.

> > The same may be done for testharnessreport.js, but WebKit version should overwrite the W3C version.
> 
> Are there features in the W3C version of testharnessreport.js that the WPT tests need that aren't in the WebKit version?

My explanation was not clear.
I did not see any issue when using WebKit version.
We cannot use W3C version for WebKit test runner to output meaningful results.

> > This would allow the WPT server to correctly send testharness.js and testharnessreport.js.
> 
> If we updated the WebKit versions testharness.js and testharnessreport.js, the WPT server could send those instead of having multiple copies in the tree, right?

Not directly, the WPT server is implemented to only serve files in his doc_root, which is set to the imported WPT base folder.

[youenn fablet]

Created attachment 238522 [details]
Updated changelog

[Bem Jones-Bey]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #0)
> > > Some W3C tests are using functions present in W3C testharness.js, but not in WebKit version.
> > > It may just be simpler to import the W3C version and update the test converter to point to that version.
> > 
> > Why don't we just update the webkit version with the newest upstream changes from the W3C?
> 
> That is a possibility, but there is always a risk that legacy WebKit tests break.

I think we should update our copy of testharness.js and fix any tests that break. I don't think that having multiple copies of testharness.js in the tree would be a good thing.

> > > This would allow the WPT server to correctly send testharness.js and testharnessreport.js.
> > 
> > If we updated the WebKit versions testharness.js and testharnessreport.js, the WPT server could send those instead of having multiple copies in the tree, right?
> 
> Not directly, the WPT server is implemented to only serve files in his doc_root, which is set to the imported WPT base folder.

We could make it possible to serve common resources through the WPT server even if they are outside of the default root, correct?

[youenn fablet]

> I think we should update our copy of testharness.js and fix any tests that break. I don't think that having multiple copies of testharness.js in the tree would be a good thing.

Doing so adds burden each time one wants to bump WPT test version.
This is manageable as long as done manually.
We can try it this way and revisit it later on.

> We could make it possible to serve common resources through the WPT server even if they are outside of the default root, correct?

Not without hacking the wpt server.
I dit in the first version of bug 127094 and would prefer avoiding that.

How about copying WebKit testharness.js/testharnessreport.js to WPT resources sub-folder when starting WPT tests run, and to clean that folder at the end of the run?

[Bem Jones-Bey]

(In reply to comment #10)
> > I think we should update our copy of testharness.js and fix any tests that break. I don't think that having multiple copies of testharness.js in the tree would be a good thing.
> 
> Doing so adds burden each time one wants to bump WPT test version.
> This is manageable as long as done manually.
> We can try it this way and revisit it later on.

How many times do updates to testharness.js have backwards incompatible changes? I'd think they'd like to minimize those, because of the sheer number of tests that use it. So hopefully updating it regularly won't be hard.

> 
> > We could make it possible to serve common resources through the WPT server even if they are outside of the default root, correct?
> 
> Not without hacking the wpt server.
> I dit in the first version of bug 127094 and would prefer avoiding that.
> 
> How about copying WebKit testharness.js/testharnessreport.js to WPT resources sub-folder when starting WPT tests run, and to clean that folder at the end of the run?

That sounds like a perfectly good plan to me.

[Bem Jones-Bey]

Comment on attachment 238522 [details]
Updated changelog

Just because I don't think we should go this path.

[youenn fablet]

> > How about copying WebKit testharness.js/testharnessreport.js to WPT resources sub-folder when starting WPT tests run, and to clean that folder at the end of the run?
> 
> That sounds like a perfectly good plan to me.

But test importer is still converting links to map to WebKit resources folder.
How about adding an option to disallow testharness link conversion?

[Bem Jones-Bey]

(In reply to comment #13)
> > > How about copying WebKit testharness.js/testharnessreport.js to WPT resources sub-folder when starting WPT tests run, and to clean that folder at the end of the run?
> > 
> > That sounds like a perfectly good plan to me.
> 
> But test importer is still converting links to map to WebKit resources folder.
> How about adding an option to disallow testharness link conversion?

Is there any way to determine from the test itself if it should be converted or not? Do all of the Web Platform Tests need to be served by the HTTP server?

[youenn fablet]

(In reply to comment #14)
> (In reply to comment #13)
> > > > How about copying WebKit testharness.js/testharnessreport.js to WPT resources sub-folder when starting WPT tests run, and to clean that folder at the end of the run?
> > > 
> > > That sounds like a perfectly good plan to me.
> > 
> > But test importer is still converting links to map to WebKit resources folder.
> > How about adding an option to disallow testharness link conversion?
> 
> Is there any way to determine from the test itself if it should be converted or not? Do all of the Web Platform Tests need to be served by the HTTP server?

Not from the test itself.
We could specify rules using the filename but we would need to keep them in sync with the testname2uri mapping...

The original plan was to run all tests through WPT server.
If it was taking too much time to run, we could restrict the use of WPT server to some specific folders.

That is why the initial idea behind this patch is to convert the absolute testharness URLs as relative URLs (working with both file:// and http:// urls).

[Bem Jones-Bey]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > > > How about copying WebKit testharness.js/testharnessreport.js to WPT resources sub-folder when starting WPT tests run, and to clean that folder at the end of the run?
> > > > 
> > > > That sounds like a perfectly good plan to me.
> > > 
> > > But test importer is still converting links to map to WebKit resources folder.
> > > How about adding an option to disallow testharness link conversion?
> > 
> > Is there any way to determine from the test itself if it should be converted or not? Do all of the Web Platform Tests need to be served by the HTTP server?
> 
> Not from the test itself.
> We could specify rules using the filename but we would need to keep them in sync with the testname2uri mapping...

Is the testname2uri mapping not stored in the WPT repo? If there's a file there that says which tests need to run using a web server, it seems to me like we could use that during the import process to decide if we should rewrite or not. (And we could use that to configure the WebKit test scripts to only run those tests via the WPT server) The CSSWG's tests have metadata in the test header, for example, that say if the test needs to run on a web server or not.

> 
> The original plan was to run all tests through WPT server.
> If it was taking too much time to run, we could restrict the use of WPT server to some specific folders.

Are most of the WPT tests intended to be run using the WPT server? How do others run the test suite? To me, aside from any performance concerns, running tests via a server adds another potential point of failure, so I'd like to have it be as simple as possible to run the tests.

> 
> That is why the initial idea behind this patch is to convert the absolute testharness URLs as relative URLs (working with both file:// and http:// urls).

If there is no reasonable way to tell which tests need an HTTP server and which don't, then I'm fine with adding a flag to indicate that HTTP server tests are being imported, and use that to not rewrite the testharness URLs. But I feel that's a last resort.

[youenn fablet]

> Is the testname2uri mapping not stored in the WPT repo? If there's a file there that says which tests need to run using a web server, it seems to me like we could use that during the import process to decide if we should rewrite or not. (And we could use that to configure the WebKit test scripts to only run those tests via the WPT server) The CSSWG's tests have metadata in the test header, for example, that say if the test needs to run on a web server or not.

I am not aware of any such metadata in WPT.
Tests using python scripts URLs and/or XHR seem scattered in various folders (html, XMLHttpRequest, workers...)

As of testname2uri mapping, I was meaning the one implemented in WebKit driver.py.
Currently, the mapping generates http:// URLs for LayoutTests/http tests and file:// URLs otherwise.
As part of patch for bug 127094, another rule would be added for WPT tests.

> Are most of the WPT tests intended to be run using the WPT server? How do others run the test suite? To me, aside from any performance concerns, running tests via a server adds another potential point of failure, so I'd like to have it be as simple as possible to run the tests.

Firefox seems to run all WPT tests through the WPT server.

Blink is not running any WPT test that requires a web server (see http://www.chromium.org/blink/importing-the-w3c-tests)
https://code.google.com/p/chromium/issues/detail?id=360762 presents different alternatives.

At the end, using python wptserver seems the way to go. WPT tests are designed and tested with that server in mind.

[Bem Jones-Bey]

(In reply to comment #17)
> > Is the testname2uri mapping not stored in the WPT repo? If there's a file there that says which tests need to run using a web server, it seems to me like we could use that during the import process to decide if we should rewrite or not. (And we could use that to configure the WebKit test scripts to only run those tests via the WPT server) The CSSWG's tests have metadata in the test header, for example, that say if the test needs to run on a web server or not.
> 
> I am not aware of any such metadata in WPT.
> Tests using python scripts URLs and/or XHR seem scattered in various folders (html, XMLHttpRequest, workers...)
> 
> As of testname2uri mapping, I was meaning the one implemented in WebKit driver.py.
> Currently, the mapping generates http:// URLs for LayoutTests/http tests and file:// URLs otherwise.
> As part of patch for bug 127094, another rule would be added for WPT tests.

Ok, sounds like we should add the flag for now. Depending on how the performance is, maybe we could consider moving imported CSSWG tests to use the wpt server as well, and never rewriting paths to testharness.

[youenn fablet]

Created attachment 243672 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 243672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243672&action=review

> Tools/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description but after the bug URL.

> Tools/Scripts/webkitpy/w3c/test_importer.py:127
> +    parser.add_option('-l', '--no-links-conversion', dest='no_links_conversion', action='store_true', default=False,
> +        help='Do not change links (testharness js or css). By default, links are converted to point to WebKit testharness files.')

I'd prefer storing convert_test_harness_links and set the default to True instead.

[youenn fablet]

Created attachment 243698 [details]
Updated new option name

[youenn fablet]

Created attachment 243699 [details]
Updated new option name

[WebKit Commit Bot]

Comment on attachment 243699 [details]
Updated new option name

Clearing flags on attachment: 243699

Committed r177727: <http://trac.webkit.org/changeset/177727>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.