Bug 134763 - W3C test importer should have an option to disable testharness.js/testharnessreport.js link conversion
Summary: W3C test importer should have an option to disable testharness.js/testharness...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-09 06:41 PDT by youenn fablet
Modified: 2014-12-24 16:54 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.02 KB, patch)
2014-07-09 06:47 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Added parameter to resources folder (3.27 KB, patch)
2014-07-16 11:28 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Updating webkitpy tests (5.60 KB, patch)
2014-09-16 01:37 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
fixing import test executable (7.30 KB, patch)
2014-09-16 13:42 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
fixing import test executable (7.30 KB, patch)
2014-09-16 13:43 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Updated changelog (7.34 KB, patch)
2014-09-23 01:27 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (5.65 KB, patch)
2014-12-23 06:58 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Updated new option name (5.31 KB, patch)
2014-12-23 15:20 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Updated new option name (5.33 KB, patch)
2014-12-23 15:23 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2014-07-09 06:41:49 PDT
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.
Comment 1 youenn fablet 2014-07-09 06:47:57 PDT
Created attachment 234632 [details]
Patch
Comment 2 youenn fablet 2014-07-16 11:28:00 PDT
Created attachment 235009 [details]
Added parameter to resources folder
Comment 3 youenn fablet 2014-09-16 01:37:45 PDT
Created attachment 238159 [details]
Updating webkitpy tests
Comment 4 youenn fablet 2014-09-16 13:42:56 PDT
Created attachment 238203 [details]
fixing import test executable
Comment 5 youenn fablet 2014-09-16 13:43:30 PDT
Created attachment 238204 [details]
fixing import test executable
Comment 6 Bem Jones-Bey 2014-09-22 10:39:12 PDT
(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?
Comment 7 youenn fablet 2014-09-22 12:47:22 PDT
(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.
Comment 8 youenn fablet 2014-09-23 01:27:01 PDT
Created attachment 238522 [details]
Updated changelog
Comment 9 Bem Jones-Bey 2014-09-23 09:31:34 PDT
(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?
Comment 10 youenn fablet 2014-09-23 12:19:10 PDT
> 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?
Comment 11 Bem Jones-Bey 2014-09-23 13:04:48 PDT
(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.
Comment 12 Bem Jones-Bey 2014-09-23 13:05:19 PDT
Comment on attachment 238522 [details]
Updated changelog

Just because I don't think we should go this path.
Comment 13 youenn fablet 2014-09-25 05:36:22 PDT
> > 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?
Comment 14 Bem Jones-Bey 2014-09-25 08:20:46 PDT
(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?
Comment 15 youenn fablet 2014-09-25 09:40:26 PDT
(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).
Comment 16 Bem Jones-Bey 2014-09-25 11:55:42 PDT
(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.
Comment 17 youenn fablet 2014-09-25 14:00:38 PDT
> 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.
Comment 18 Bem Jones-Bey 2014-09-25 15:09:36 PDT
(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.
Comment 19 youenn fablet 2014-12-23 06:58:42 PST
Created attachment 243672 [details]
Patch
Comment 20 Ryosuke Niwa 2014-12-23 11:34:47 PST
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.
Comment 21 youenn fablet 2014-12-23 15:20:40 PST
Created attachment 243698 [details]
Updated new option name
Comment 22 youenn fablet 2014-12-23 15:23:52 PST
Created attachment 243699 [details]
Updated new option name
Comment 23 WebKit Commit Bot 2014-12-24 16:54:16 PST
Comment on attachment 243699 [details]
Updated new option name

Clearing flags on attachment: 243699

Committed r177727: <http://trac.webkit.org/changeset/177727>
Comment 24 WebKit Commit Bot 2014-12-24 16:54:21 PST
All reviewed patches have been landed.  Closing bug.