Bug 142738 – W3C test importer should generate the modules installed dynamically to run wpt tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[youenn fablet]

WPT submodules are downloaded and installed at test runtime based on a WPTModules file.
The test importer should generate that file based on wpt git information.

[youenn fablet]

Created attachment 248734 [details]
Patch

[youenn fablet]

Created attachment 248744 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 248744 [details]
Patch

I'm low on time at the moment, so just one comment right now: why not call it web-platform-tests-modules.json, since it's a JSON file?

[WebKit Commit Bot]

Attachment 248744 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:96:  [TestImporterTest.import_downloaded_tests.TestDownloaderMock] Undefined variable 'TestDownloader'  [pylint/E0602] [5]
ERROR: Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:98:  [TestImporterTest.import_downloaded_tests.TestDownloaderMock.__init__] Undefined variable 'TestDownloader'  [pylint/E0602] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 248925 [details]
Updating module file name and missing python import

[youenn fablet]

Created attachment 248928 [details]
Updating module file name and missing python import, rebased version

[Bem Jones-Bey]

Comment on attachment 248928 [details]
Updating module file name and missing python import, rebased version

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

> Tools/Scripts/webkitpy/w3c/test_downloader.py:153
> +        return self.git(repository_directory)._run_git(['submodule', 'status'])

Given how often you're finding the need to call _run_git in these patches, I wonder if we should reconsider the interface to the git helper module. Perhaps something as simple as wrapping _run_git with a "run" method, so that it's actually public API. That's definitely something for a different patch, though.

> Tools/Scripts/webkitpy/w3c/test_downloader.py:167
> +                    if not submodule['url'].startswith('https://github.com/'):
> +                        _log.warning('submodule %s not hosted on github' % submodule['path'])

It seems like the code assumes github url format below. If we do end up with an invalid URL in the file, will it cause errors later?

It's not clear to me why this is a problem and the cases where it would be ok to go on with the tests as opposed to just giving up on the import. Maybe if the error message made it clearer what one is expected to do about this?

> Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:137
> +        self.assertFalse(fs.exists('/mock-checkout/LayoutTests/w3c/resources/csswg-tests.modules'))

Shouldn't this be csswg-tests-modules.json ?

[youenn fablet]

Comment on attachment 248928 [details]
Updating module file name and missing python import, rebased version

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

>> Tools/Scripts/webkitpy/w3c/test_downloader.py:153
>> +        return self.git(repository_directory)._run_git(['submodule', 'status'])
> 
> Given how often you're finding the need to call _run_git in these patches, I wonder if we should reconsider the interface to the git helper module. Perhaps something as simple as wrapping _run_git with a "run" method, so that it's actually public API. That's definitely something for a different patch, though.

Agreed this would be useful.

>> Tools/Scripts/webkitpy/w3c/test_downloader.py:167
>> +                        _log.warning('submodule %s not hosted on github' % submodule['path'])
> 
> It seems like the code assumes github url format below. If we do end up with an invalid URL in the file, will it cause errors later?
> 
> It's not clear to me why this is a problem and the cases where it would be ok to go on with the tests as opposed to just giving up on the import. Maybe if the error message made it clearer what one is expected to do about this?

We end up with an URL that probably leads to a 404 response.
This would make rwt break when trying to launch wpt server.
We could raise an error but it may be just better to edit the modules description file after the import.

Beefing up the message makes sense, maybe something like:
'submodule %s not hosted on github. Please ensure that URL %s points to an archive of the submodule, or manually change the URL after the import is done'

>> Tools/Scripts/webkitpy/w3c/test_importer_unittest.py:137
>> +        self.assertFalse(fs.exists('/mock-checkout/LayoutTests/w3c/resources/csswg-tests.modules'))
> 
> Shouldn't this be csswg-tests-modules.json ?

Good catch, I will fit it.

[youenn fablet]

Created attachment 248955 [details]
Fixing test and beefing up warning

[Ryosuke Niwa]

Comment on attachment 248955 [details]
Fixing test and beefing up warning

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

> LayoutTests/imported/w3c/ChangeLog:9
> +        Renamed WPTModules to web-platform-test-modules.json
> +        Updated TestRepositories to ask the importer to generate web-platform-test-modules.json at import time.  

It's not clear to me what "modules" mean. It sounds like they're dependencies or shared resources used by WPT tests?
Or are you referring to a git submodule?

> LayoutTests/imported/w3c/resources/web-platform-tests-modules.json:8
> +        "url_subpath": "testharness.js-2a1da264f6718db04c3925a9956b635426827aef"

I don't understand why we need to create this "-" split pseudo path.
Can't we just store two fields one of which is the file/directory name and another is a git hash?

[youenn fablet]

Comment on attachment 248955 [details]
Fixing test and beefing up warning

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

>> LayoutTests/imported/w3c/ChangeLog:9
>> +        Updated TestRepositories to ask the importer to generate web-platform-test-modules.json at import time.  
> 
> It's not clear to me what "modules" mean. It sounds like they're dependencies or shared resources used by WPT tests?
> Or are you referring to a git submodule?

Yes, I am referring to git submodules.
This is the file used to fuel WebPlatformTestServer._install_modules called at WPT server start.

>> LayoutTests/imported/w3c/resources/web-platform-tests-modules.json:8
>> +        "url_subpath": "testharness.js-2a1da264f6718db04c3925a9956b635426827aef"
> 
> I don't understand why we need to create this "-" split pseudo path.
> Can't we just store two fields one of which is the file/directory name and another is a git hash?

url and url_subpath are parameters passed directly to AutoInstaller.install.
The current format is generic/consistent with webkitpy thirdparty.

[Ryosuke Niwa]

Comment on attachment 248955 [details]
Fixing test and beefing up warning

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

> Tools/Scripts/webkitpy/w3c/test_downloader.py:160
> +            for line in self._filesystem.read_text_file(self._filesystem.join(repository_directory, '.gitmodules')).splitlines():

Can we extract this into a function?  e.g. parse_gitmodules.

> Tools/Scripts/webkitpy/w3c/test_downloader.py:188
> +        submodule_status = self._submodule_status(repository_directory)
> +        submodule_status = [line[1:].split(' ') for line in self._submodule_status(repository_directory).splitlines()]
> +        for submodule in submodules:
> +            for status in submodule_status:
> +                if submodule['path'] == status[1]:
> +                    url = submodule['url'][:-4]
> +                    version = status[0]
> +                    repository_name = url.split('/').pop()
> +                    submodule['url'] = url + '/archive/' + version + '.tar.gz'
> +                    submodule['url_subpath'] = repository_name + '-' + version
> +            if '/' in submodule['path']:
> +                steps = submodule['path'].split('/')
> +                submodule['name'] = steps.pop()
> +                submodule['path'] = steps
> +            else:
> +                submodule['name'] = submodule['path']
> +                submodule['path'] = ['.']

Ditto about putting this into a separate function.  Also can we use filesystem abstraction or python's builtin urllib or os libraries to deal with . and / instead of manually spliting and parsing them?

[Ryosuke Niwa]

Comment on attachment 248955 [details]
Fixing test and beefing up warning

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

> Tools/Scripts/webkitpy/w3c/test_downloader.py:192
> +    def dump_submodules_description(self, test_repository, filepath):
> +        self._filesystem.write_text_file(filepath, json.dumps(self.submodules_description(test_repository), sort_keys=True, indent=4))

I don't think there's much value in sperating submodules_description from dump_submodules_description.
Also, I think we give it a more descriptive name such as generate_description_for_git_submodules.

> Tools/Scripts/webkitpy/w3c/test_importer.py:196
> +    def post_process_downloaded_tests(self):

post_process_downloaded_tests() sounds very generic and doesn't tell us what it does.
I think we should call it something like generate_git_submodule_descriptions_for_all_repositories.

[youenn fablet]

(In reply to comment #13)
> Comment on attachment 248955 [details]
> Fixing test and beefing up warning
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248955&action=review
> 
> > Tools/Scripts/webkitpy/w3c/test_downloader.py:192
> > +    def dump_submodules_description(self, test_repository, filepath):
> > +        self._filesystem.write_text_file(filepath, json.dumps(self.submodules_description(test_repository), sort_keys=True, indent=4))
> 
> I don't think there's much value in sperating submodules_description from
> dump_submodules_description.

submodules_description is reused in bug 142743 to generate .gitignore

> Also, I think we give it a more descriptive name such as
> generate_description_for_git_submodules.

ok

> 
> > Tools/Scripts/webkitpy/w3c/test_importer.py:196
> > +    def post_process_downloaded_tests(self):
> 
> post_process_downloaded_tests() sounds very generic and doesn't tell us what
> it does.
> I think we should call it something like
> generate_git_submodule_descriptions_for_all_repositories.

The idea is to include gitignore generation handling in the same loop (cf bug 142743).
I agree the post_process name is too vague.
I would rename it to process_test_repositories_import_options, as it is guided by the "import_options" field found in TestRepositories.

[youenn fablet]

Created attachment 249112 [details]
Fixing names

[Ryosuke Niwa]

Comment on attachment 249112 [details]
Fixing names

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

> Tools/Scripts/webkitpy/w3c/test_importer.py:196
> +    def process_test_repositories_import_options(self):

This name is still too vague.
Just call it generate_git_submodules_description_for_all_repositories.

[youenn fablet]

Created attachment 250196 [details]
Patch for landing

[youenn fablet]

Created attachment 250197 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 250197 [details]
Patch for landing

Clearing flags on attachment: 250197

Committed r182386: <http://trac.webkit.org/changeset/182386>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.