Bug 151751

Summary:

Add support to import w3c tests from a repository with a different root that the main repo dir

Product:

WebKit

Reporter:

Xabier Rodríguez Calvar <calvaris>

Component:

New Bugs

Assignee:

Xabier Rodríguez Calvar <calvaris>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bjonesbe, cdumez, commit-queue, darin, glenn, rniwa, youennf

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none
Patch for landing	none
Patch for landing	none

Xabier Rodríguez Calvar

Reported 2015-12-02 10:02:03 PST

[Streams API] Add support to import web platform tests directly from the spec reference

Attachments
Patch (18.30 KB, patch) 2015-12-02 10:21 PST, Xabier Rodríguez Calvar	no flags	Details Formatted Diff Diff
Patch (18.18 KB, patch) 2015-12-03 00:56 PST, Xabier Rodríguez Calvar	no flags	Details Formatted Diff Diff
Patch (8.81 KB, patch) 2015-12-03 09:10 PST, Xabier Rodríguez Calvar	no flags	Details Formatted Diff Diff
Patch (6.37 KB, patch) 2015-12-07 09:20 PST, Xabier Rodríguez Calvar	no flags	Details Formatted Diff Diff
Patch for landing (6.37 KB, patch) 2015-12-07 09:42 PST, Xabier Rodríguez Calvar	no flags	Details Formatted Diff Diff
Patch for landing (6.37 KB, patch) 2015-12-07 10:00 PST, Xabier Rodríguez Calvar	no flags	Details Formatted Diff Diff
Patch for landing (6.39 KB, patch) 2015-12-07 10:05 PST, Xabier Rodríguez Calvar	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Xabier Rodríguez Calvar

Comment 1 2015-12-02 10:21:10 PST

Created attachment 266451 [details] Patch

youenn fablet

Comment 2 2015-12-02 12:48:15 PST

Comment on attachment 266451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266451&action=review > Tools/ChangeLog:3 > + [Streams API] Add support to import web platform tests directly from the spec reference I think this change is more general than streams API. I would rename the bug to something like "Enable importing of tests from a repository sub-directory" In the future, we might also consider changing these scripts so as to support importing of non W3C repositories in LayoutTests/imported, like chromium or Mozilla for instance. > Tools/ChangeLog:16 > + the test converter to remove any script tags containing workers code. That issue is specific to streams repository. In general, it is best to not modify any test if possible. It might be simpler to: - Import streams-api tests as a sub-directory of imported/w3c/web-platform-tests (this would require changes in the import script). - Modify if needed service-worker resource test scripts so as to disable the service-worker flavoured tests until service-worker gets supported in WebKit. > Tools/Scripts/webkitpy/w3c/test_downloader.py:111 > + files = self._filesystem.listdir(complete_directory) files is just used once, maybe we do not need it. > Tools/Scripts/webkitpy/w3c/test_downloader.py:115 > + paths = (original_path, self._filesystem.join(webkit_path, name)) paths is just used once, maybe we do not need it. > Tools/Scripts/webkitpy/w3c/test_downloader.py:138 > + tests_directory = '' Could you rewrite it like: test_directory = test_... if(...) else ''? > Tools/Scripts/webkitpy/w3c/test_downloader.py:146 > + path, destination_path = paths Why not using paths[0] and paths[1] in code below. They appear to be used just once. This way, destination_path would not need to be changed to destination_path. > Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:118 > + '/mock-checkout/WebKitBuild/w3c-tests/streams-web-platform-tests/reference-implementation/web-platform-tests/test.html': '<!doctype html><script src="/resources/testharness.js"></script><script src="/resources/testharnessreport.js"></script>', I am not sure what this change is for? Should there be som assert added afterwards? > Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:134 > + '/mock-checkout/WebKitBuild/w3c-tests/streams-web-platform-tests/reference-implementation/web-platform-tests/test.html': '<!doctype html><script src="/resources/testharness.js"></script><script src="/resources/testharnessreport.js"></script>', Ditto. > LayoutTests/imported/w3c/resources/TestRepositories:36 > + "name": "streams-web-platform-tests", I would use streams-api as folder name. > LayoutTests/imported/w3c/resources/TestRepositories:40 > + "paths_to_skip": [], If we import these tests in web-platform-tests folder, it will be no longer needed to convert test-harness links. Unrelated note, we might want in a later patch to get ridd of paths_to_skip. And probably not require to set parameter values if they are empty.

Xabier Rodríguez Calvar

Comment 3 2015-12-03 00:56:51 PST

Created attachment 266519 [details] Patch

Xabier Rodríguez Calvar

Comment 4 2015-12-03 02:38:58 PST

(In reply to comment #2) > > Tools/ChangeLog:3 > > + [Streams API] Add support to import web platform tests directly from the spec reference > > I think this change is more general than streams API. > I would rename the bug to something like "Enable importing of tests from a > repository sub-directory" I thought of that, but considering that I am adding the the streams repository I don't think it is a good idea to make this generic description as the change is not generic. > > Tools/ChangeLog:16 > > + the test converter to remove any script tags containing workers code. > > That issue is specific to streams repository. > In general, it is best to not modify any test if possible. > It might be simpler to: > - Import streams-api tests as a sub-directory of > imported/w3c/web-platform-tests (this would require changes in the import > script). > - Modify if needed service-worker resource test scripts so as to disable the > service-worker flavoured tests until service-worker gets supported in WebKit. I have quite clear the being different repositories they should end up in different directories, hence I wouldn't add it to the web-platform-tests and therefore the code needed for htat. As the filter for the workers, I think it is a filter that can be activated and detactivated. Currently we only use it for this repository but we could use it for more things. > > Tools/Scripts/webkitpy/w3c/test_downloader.py:111 > > + files = self._filesystem.listdir(complete_directory) > > files is just used once, maybe we do not need it. > > > Tools/Scripts/webkitpy/w3c/test_downloader.py:115 > > + paths = (original_path, self._filesystem.join(webkit_path, name)) > > paths is just used once, maybe we do not need it. > > > Tools/Scripts/webkitpy/w3c/test_downloader.py:138 > > + tests_directory = '' > > Could you rewrite it like: test_directory = test_... if(...) else ''? > > > Tools/Scripts/webkitpy/w3c/test_downloader.py:146 > > + path, destination_path = paths > > Why not using paths[0] and paths[1] in code below. They appear to be used > just once. > This way, destination_path would not need to be changed to destination_path. All this done. > > Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:118 > > + '/mock-checkout/WebKitBuild/w3c-tests/streams-web-platform-tests/reference-implementation/web-platform-tests/test.html': '<!doctype html><script src="/resources/testharness.js"></script><script src="/resources/testharnessreport.js"></script>', > > I am not sure what this change is for? > Should there be som assert added afterwards? This change is motivated because that test is importing all repostitories and it was failing because the fake streams repository was empty, therefore I added the fake file. It is true though that we could check for the output as the repository has the test harness option and that could be checked, so I added a couple of assertions. > > Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:134 > > + '/mock-checkout/WebKitBuild/w3c-tests/streams-web-platform-tests/reference-implementation/web-platform-tests/test.html': '<!doctype html><script src="/resources/testharness.js"></script><script src="/resources/testharnessreport.js"></script>', > > Ditto. In this case we also have the issue that all repositories are added, so I added a fake file so that the test doesn't fail when retrieving, but the difference with the other case is that streams tests does not have any submodules that we need to care about so that fake file is purely phony. > > LayoutTests/imported/w3c/resources/TestRepositories:36 > > + "name": "streams-web-platform-tests", > > I would use streams-api as folder name. Done. > Unrelated note, we might want in a later patch to get ridd of paths_to_skip. > And probably not require to set parameter values if they are empty. It would be interesting to make the parameter optional. I didn't touch it because it was out of the scope of the patch. What I did was making the parameter that I added optional.

Xabier Rodríguez Calvar

Comment 5 2015-12-03 09:10:17 PST

Created attachment 266531 [details] Patch

Xabier Rodríguez Calvar

Comment 6 2015-12-03 09:13:59 PST

After discussing with Youenn, we think it might be better to submit only the part of importing the tests from a specific directory and then consider a different strategy to deal with the changes in the tests that are necessary to get them running in WebKit. There's a discussion in GitHub and we might postpone any further actions until that is solved.

Ryosuke Niwa

Comment 7 2015-12-03 22:59:29 PST

Comment on attachment 266531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266531&action=review > Tools/Scripts/webkitpy/w3c/test_downloader.py:134 > + test_repository['tests_directory'] if ('tests_directory' in test_repository) else '') Nit: wrong indentation. test_repository['tests_directory'] should appear exactly 4 spaces to the right of self._add_test_suite_paths.

Xabier Rodríguez Calvar

Comment 8 2015-12-07 09:20:25 PST

Created attachment 266780 [details] Patch

Xabier Rodríguez Calvar

Comment 9 2015-12-07 09:32:34 PST

(In reply to comment #8) > Created attachment 266780 [details] > Patch This patch has some subtantial changes with the other one, like removing the repository, adding the new test, leaving the other untouched...

youenn fablet

Comment 10 2015-12-07 09:34:14 PST

Comment on attachment 266780 [details] Patch r=me, patch is rebased according bug 151938

Xabier Rodríguez Calvar

Comment 11 2015-12-07 09:42:03 PST

Created attachment 266782 [details] Patch for landing

Xabier Rodríguez Calvar

Comment 12 2015-12-07 10:00:11 PST

Created attachment 266784 [details] Patch for landing

Xabier Rodríguez Calvar

Comment 13 2015-12-07 10:05:18 PST

Created attachment 266785 [details] Patch for landing

WebKit Commit Bot

Comment 14 2015-12-07 10:50:19 PST

Comment on attachment 266785 [details] Patch for landing Clearing flags on attachment: 266785 Committed r193638: <http://trac.webkit.org/changeset/193638>

WebKit Commit Bot

Comment 15 2015-12-07 10:50:23 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.