Bug 134764 – WebKit test infrastructure should automate the process of cloning W3C test suite and importing tests from it

• Component: Tools / Tests
• Status: RESOLVED FIXED

[youenn fablet]

To import W3C tests, the first step is to clone the W3C test suite.
To ease test import, it would be convenient to have a tool that automates the process of cloning/refreshing the W3C test suite repo then importing parts or all tests

[youenn fablet]

Created attachment 234633 [details]
Patch

[youenn fablet]

Created attachment 235011 [details]
Handlingof resources path

[youenn fablet]

Created attachment 238161 [details]
Added file version and skipping .git dir import

[youenn fablet]

Created attachment 238175 [details]
Rebasing according TestImporter change

[Bem Jones-Bey]

Comment on attachment 238175 [details]
Rebasing according TestImporter change

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

I'm all for importing the WPT tests!

How is this importer going to be invoked? Are you planning to write a wrapper script?

Also, it would be nice to seem some unit tests for this code.

> Tools/ChangeLog:9
> +        * Scripts/webkitpy/w3c/factory.py: Added (w3c test suite argument parser).
> +        (w3c_create_argument_parser):

This function isn't used by any of your code, and factory.py isn't a very good filename. Is this actually needed? If so what are you using if for?

> Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:97
> +    def wptsha(self):

wpt_version would be a better name.

> Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:108
> +    def git_clone_arguments(self):

Maybe name this git_clone_tests, and have it actually do the call to Executive().run_command? It doesn't seem that useful for it just to return the list that has to be passed ton run_command.

> Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:111
> +    def git_pull_arguments(self):

Ditto, "git_pull_tests"

> Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:114
> +    def git_checkout_arguments(self, version):

Ditto, "git_checkout_test_version"

[youenn fablet]

(In reply to comment #5)
> (From update of attachment 238175 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238175&action=review
> 
> I'm all for importing the WPT tests!
> 
> How is this importer going to be invoked? Are you planning to write a wrapper script?

bug 134766 and 134767 are using it.
A wrapper script may still be handy though.

> Also, it would be nice to seem some unit tests for this code.

Right, I will add some.

> 
> > Tools/ChangeLog:9
> > +        * Scripts/webkitpy/w3c/factory.py: Added (w3c test suite argument parser).
> > +        (w3c_create_argument_parser):
> 
> This function isn't used by any of your code, and factory.py isn't a very good filename. Is this actually needed? If so what are you using if for?

It is used elsewhere, I will rationalize this.
> 
> > Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:97
> > +    def wptsha(self):
> 
> wpt_version would be a better name.

OK

> 
> > Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:108
> > +    def git_clone_arguments(self):
> 
> Maybe name this git_clone_tests, and have it actually do the call to Executive().run_command? It doesn't seem that useful for it just to return the list that has to be passed ton run_command.
> 
> > Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:111
> > +    def git_pull_arguments(self):
> 
> Ditto, "git_pull_tests"
> 
> > Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:114
> > +    def git_checkout_arguments(self, version):
> 
> Ditto, "git_checkout_test_version"

Thanks for the feedback, I will update the patch accordingly

[youenn fablet]

Created attachment 243673 [details]
Updated according review

[youenn fablet]

(In reply to comment #5)
> Comment on attachment 238175 [details]
> Rebasing according TestImporter change
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=238175&action=review
> 
> I'm all for importing the WPT tests!
> 
> How is this importer going to be invoked? Are you planning to write a
> wrapper script?
> 
> Also, it would be nice to seem some unit tests for this code.

It seems difficult to write meaningful tests without making network calls or doing executive cmd.
If you have some ideas on tests to add, I will be happy to add them in a following patch.(In reply to comment #5)
> Comment on attachment 238175 [details]
> Rebasing according TestImporter change
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=238175&action=review
> 
> I'm all for importing the WPT tests!
> 
> How is this importer going to be invoked? Are you planning to write a
> wrapper script?
> 
> Also, it would be nice to seem some unit tests for this code.

It seems difficult to write meaningful tests without making network calls or doing executive cmd.
If you have some ideas on tests to add, I will be happy to add them in a following patch.

[Ryosuke Niwa]

Comment on attachment 243673 [details]
Updated according review

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

> Tools/ChangeLog:9
> +        WebPlatformTestImporter imports W3C tests into Webkit from the official W3C repository (https://github.com/w3c/web-platform-tests.git).
> +        The W3C test suite repository is cloned and tests are then imported using test_import.py.

We already have http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/w3c/test_importer.py
How is this version different from that?

> Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:57
> +        # Excluding some directories
> +        self._exclude_dir = ['conformance-checkers', 'old-tests', '.git']
> +        # Including test infrastructure directories (use of test importer)
> +        self._always_import_dirs = ('common', 'fonts', 'images', 'media', 'resources', 'tools')
> +        # Including test infrastructure files (no use of test importer, direct copy)
> +        self._always_import_files = ['config.default.json', 'serve.py']

These comments repeat what the code says.  Please remove them.
Also, rename _always_import_dirs to a more descriptive name like _test_infrastructure_dirs
and _always_import_files to _test_infrastrcutre_files.

> Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:64
> +        if self._options.__dict__['w3c_verbose'] is False:

What kind of an object is options?  This code looks funky and superfluous.

> Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:87
> +    def always_imported_files(self):
> +        return self._always_import_files
> +
> +    def always_imported_directories(self):
> +        return self._always_import_dirs

Ditto about renames.  But these methods seem to be never called anywhere.

Could we not have them until we need them?

[youenn fablet]

(In reply to comment #9)
> Comment on attachment 243673 [details]
> Updated according review
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243673&action=review
> 
> > Tools/ChangeLog:9
> > +        WebPlatformTestImporter imports W3C tests into Webkit from the official W3C repository (https://github.com/w3c/web-platform-tests.git).
> > +        The W3C test suite repository is cloned and tests are then imported using test_import.py.
> 
> We already have
> http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/w3c/
> test_importer.py
> How is this version different from that?

Two test infrastructure coexist in the W3C: WPT and CSS 
web_platform_test_importer.py is dedicated to WPT (git cloning and conversion).
test_importer.py is used to convert both CSS and WPT tests.

> 
> > Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:57
> > +        # Excluding some directories
> > +        self._exclude_dir = ['conformance-checkers', 'old-tests', '.git']
> > +        # Including test infrastructure directories (use of test importer)
> > +        self._always_import_dirs = ('common', 'fonts', 'images', 'media', 'resources', 'tools')
> > +        # Including test infrastructure files (no use of test importer, direct copy)
> > +        self._always_import_files = ['config.default.json', 'serve.py']
> 
> These comments repeat what the code says.  Please remove them.
> Also, rename _always_import_dirs to a more descriptive name like
> _test_infrastructure_dirs
> and _always_import_files to _test_infrastrcutre_files.

OK

> > Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:64
> > +        if self._options.__dict__['w3c_verbose'] is False:
> 
> What kind of an object is options?  This code looks funky and superfluous.
> 
> > Tools/Scripts/webkitpy/w3c/web_platform_test_importer.py:87
> > +    def always_imported_files(self):
> > +        return self._always_import_files
> > +
> > +    def always_imported_directories(self):
> > +        return self._always_import_dirs
> 
> Ditto about renames.  But these methods seem to be never called anywhere.
> 
> Could we not have them until we need them?

Let's remove these methods and make the classs fields public, as they are used by the wpt baseline generator.

[youenn fablet]

Created attachment 243700 [details]
Updated according review

[Ryosuke Niwa]

(In reply to comment #10)
> (In reply to comment #9)
> > Comment on attachment 243673 [details]
> > Updated according review
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=243673&action=review
> > 
> > > Tools/ChangeLog:9
> > > +        WebPlatformTestImporter imports W3C tests into Webkit from the official W3C repository (https://github.com/w3c/web-platform-tests.git).
> > > +        The W3C test suite repository is cloned and tests are then imported using test_import.py.
> > 
> > We already have
> > http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/w3c/
> > test_importer.py
> > How is this version different from that?
> 
> Two test infrastructure coexist in the W3C: WPT and CSS 
> web_platform_test_importer.py is dedicated to WPT (git cloning and
> conversion).
> test_importer.py is used to convert both CSS and WPT tests.

If that's the case, it appears to me that test_importer.py is the one that should be using web_platform_test_importer.py, not the other way around as it's currently done in this patch.

[youenn fablet]

(In reply to comment #12)
> > Two test infrastructure coexist in the W3C: WPT and CSS 
> > web_platform_test_importer.py is dedicated to WPT (git cloning and
> > conversion).
> > test_importer.py is used to convert both CSS and WPT tests.
> 
> If that's the case, it appears to me that test_importer.py is the one that
> should be using web_platform_test_importer.py, not the other way around as
> it's currently done in this patch.

test_importer.py should be split in two parts:
- one specific (css_test_suite_importer.py) to CSS test suite
- one generic (w3c_test_importer.py) used by the dedicated CSS and WPT test suite importers 

web_platform_test_importer.py could be renamed in web_platform_test_suite_importer.py

[youenn fablet]

Created attachment 243807 [details]
Rebasing according bug 134763

[youenn fablet]

Created attachment 244512 [details]
Adding CSS tests import

[youenn fablet]

(In reply to comment #15)
> Created attachment 244512 [details]
> Adding CSS tests import

As part of this additional support, TestImporter stays the main entry point. If no source directory is provided, CSS and WPT repositories are cloned and filtered before being actually imported.

[youenn fablet]

> If that's the case, it appears to me that test_importer.py is the one that
> should be using web_platform_test_importer.py, not the other way around as
> it's currently done in this patch.

Done as part of the latest patch, test_downloader being responsible of downloading both wpt and css tests.

[Bem Jones-Bey]

Comment on attachment 244512 [details]
Adding CSS tests import

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

In general, I like this patch. Some comments and questions, though.

> Tools/ChangeLog:27
> +        Replaced shutil calls by filesystem calls.
> +        Migrated from optparse to argparse to ease conformance scripts.

It seems like these would be good in their own patch, since this one is pretty big as is.

> Tools/Scripts/webkitpy/w3c/test_downloader.py:120
> +    def checkout_web_platform_tests(self):
> +        version = self.tests_version('WPT')
> +        if not self._filesystem.exists(self.web_platform_tests_directory):
> +            _log.info('Cloning web-platform-tests repository...')
> +            self.git_clone(self.WPT_URL, self.web_platform_tests_directory)
> +        elif self._options.fetch is True:
> +            _log.info('Fetching web-platform-tests repository...')
> +            self.git_fetch(self.web_platform_tests_directory)
> +        _log.info('Checking out web_platform_tests repository branch ' + version)
> +        self.git_checkout(self.web_platform_tests_directory, version)
> +
> +    def checkout_csswg_tests(self):
> +        version = self.tests_version('CSS')
> +        if not self._filesystem.exists(self.csswg_tests_directory):
> +            _log.info('Cloning csswg-test repository...')
> +            self.git_clone(self.CSS_URL, self.csswg_tests_directory)
> +        elif self._options.fetch is True:
> +            _log.info('Fetching csswg-test repository...')
> +            self.git_fetch(self.csswg_tests_directory)
> +        _log.info('Checking out csswg-test repository branch ' + version)
> +        self.git_checkout(self.csswg_tests_directory, version)

Nit: These two methods are pretty much identical. It would be nice to remove the duplication.

> Tools/Scripts/webkitpy/w3c/test_downloader.py:134
> +            elif 'PASS' in line.expectations:
> +                self.infrastructure_paths.append(line.name)

It seems like infrastructure_paths is a misnomer here, since these paths you are adding aren't only infrastructure paths, it includes test paths, right? Maybe name it paths_to_import or something like that?

> Tools/Scripts/webkitpy/w3c/test_downloader.py:152
> +        for name in self._filesystem.listdir(self.web_platform_tests_directory):
> +            relative_path = self._filesystem.join(self.WPT_NAME, name)
> +            if self._should_copy(relative_path, name):
> +                test_paths.append(relative_path)
> +        for name in self._filesystem.listdir(self.csswg_tests_directory):
> +            relative_path = self._filesystem.join(self.CSS_NAME, name)
> +            if self._should_copy(relative_path, name):
> +                test_paths.append(relative_path)

Nit: It would be nice to fix the duplication here as well.

> Tools/Scripts/webkitpy/w3c/test_importer.py:57
> +            This should be applied to CSS tests but not to WPT tests.

Nit: "This is applied"

> Tools/Scripts/webkitpy/w3c/test_importer.py:129
> +    parser.add_argument('-l', '--no-links-conversion', dest='convert_test_harness_links', action='store_false', default=True,
> +        help='Do not change links (testharness js or css e.g.). By default, links are converted to point to WebKit testharness files if a w3c test directory to import is provided')

This help should be updated to reflect that links are only converted by default for CSS WG tests, this flag has no effect for WPT tests.

> Tools/Scripts/webkitpy/w3c/test_importer.py:426
> +        import_log.write('Instead, push changes to the corresponding CSS repository:\n')
>          import_log.write('\thttp://hg.csswg.org/test\n')

We could simplify this message and remove the mention of the HG repo. I think most people working in the WebKit source will be more inclined to go the Git route.

> LayoutTests/imported/w3c/resources/ImportExpectations:1
> +csswg-tests/compositing-1 [ Skip ]

From my reading of the code, if a new directory is added, it will be imported. I think it would be better if the default was to Skip, and this acted as a whitelist so that tests won't be accidentally imported.

[youenn fablet]

Comment on attachment 244512 [details]
Adding CSS tests import

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

>> Tools/ChangeLog:27
>> +        Migrated from optparse to argparse to ease conformance scripts.
> 
> It seems like these would be good in their own patch, since this one is pretty big as is.

Thanks a lot for the review.
I will update the patch according your suggestions, except for the last item which may need more discussion (see below).

>> Tools/Scripts/webkitpy/w3c/test_downloader.py:120
>> +        self.git_checkout(self.csswg_tests_directory, version)
> 
> Nit: These two methods are pretty much identical. It would be nice to remove the duplication.

OK

>> Tools/Scripts/webkitpy/w3c/test_downloader.py:134
>> +                self.infrastructure_paths.append(line.name)
> 
> It seems like infrastructure_paths is a misnomer here, since these paths you are adding aren't only infrastructure paths, it includes test paths, right? Maybe name it paths_to_import or something like that?

OK

>> Tools/Scripts/webkitpy/w3c/test_downloader.py:152
>> +                test_paths.append(relative_path)
> 
> Nit: It would be nice to fix the duplication here as well.

OK

>> Tools/Scripts/webkitpy/w3c/test_importer.py:57
>> +            This should be applied to CSS tests but not to WPT tests.
> 
> Nit: "This is applied"

OK

>> Tools/Scripts/webkitpy/w3c/test_importer.py:129
>> +        help='Do not change links (testharness js or css e.g.). By default, links are converted to point to WebKit testharness files if a w3c test directory to import is provided')
> 
> This help should be updated to reflect that links are only converted by default for CSS WG tests, this flag has no effect for WPT tests.

OK

>> Tools/Scripts/webkitpy/w3c/test_importer.py:426
>>          import_log.write('\thttp://hg.csswg.org/test\n')
> 
> We could simplify this message and remove the mention of the HG repo. I think most people working in the WebKit source will be more inclined to go the Git route.

OK

>> LayoutTests/imported/w3c/resources/ImportExpectations:1
>> +csswg-tests/compositing-1 [ Skip ]
> 
> From my reading of the code, if a new directory is added, it will be imported. I think it would be better if the default was to Skip, and this acted as a whitelist so that tests won't be accidentally imported.

Right, pass is the default currently, mirroring the test expectations files.
It makes things a bit simpler as this file contains only Skip lines (test suites or individual tests) and comments.
This gives the incentive to make the file smaller if not empty.

The issue you mention happens when bumping the version of the repo.
As it may also be fixed at that level, I would tend to revisit this issue when we get some more practice on bumping version.

[Bem Jones-Bey]

(In reply to comment #19)
> Comment on attachment 244512 [details]
> Adding CSS tests import
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244512&action=review
> 
> >> LayoutTests/imported/w3c/resources/ImportExpectations:1
> >> +csswg-tests/compositing-1 [ Skip ]
> > 
> > From my reading of the code, if a new directory is added, it will be imported. I think it would be better if the default was to Skip, and this acted as a whitelist so that tests won't be accidentally imported.
> 
> Right, pass is the default currently, mirroring the test expectations files.
> It makes things a bit simpler as this file contains only Skip lines (test
> suites or individual tests) and comments.
> This gives the incentive to make the file smaller if not empty.
> 
> The issue you mention happens when bumping the version of the repo.
> As it may also be fixed at that level, I would tend to revisit this issue
> when we get some more practice on bumping version.

You have a good point there. Having it work like existing expectations files is valuable. It would be nice if we could have the script notify on version updates if a new path is getting imported so that something intelligent could be done about it. But that's something for the future.

[youenn fablet]

Created attachment 245166 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 245166 [details]
Patch

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

> Tools/Scripts/webkitpy/w3c/test_downloader.py:72
> +        self._init_paths_from_expectations(self._filesystem.join(self.layout_tests_w3c_resources_path, 'InfrastructurePaths'))

I don't think we should be using TestExpectations format for InfrastructurePaths.
That would be very confusion because web-platform-tests/config.default.json, for example, is never ran as a test.
We should just use a JSON format indicating whether a file should be imported or not.
r- because of this.

> Tools/Scripts/webkitpy/w3c/test_downloader.py:91
> +    def _run_command(self, arguments):
> +        Executive().run_command(arguments, return_stderr=False)
> +
> +    def git_clone(self, url, directory):
> +        arguments = ['git', 'clone', '--recursive', url, directory]
> +        self._run_command(arguments)
> +
> +    def git_fetch(self, directory):
> +        arguments = ['git', '-C', directory, 'pull']
> +        self._run_command(arguments)
> +
> +    def git_checkout(self, directory, version):
> +        arguments = ['git', '-C', directory, 'checkout', version]
> +        if not self._options.verbose:
> +            arguments += ['-q']
> +        self._run_command(arguments)

Why are we not using scm/git.py instead?

> Tools/Scripts/webkitpy/w3c/test_downloader.py:93
> +    def tests_version(self, name):

Is "version" some W3C test specific terminology?  If not, why don't we call it "branch" to match git terminology?

> Tools/Scripts/webkitpy/w3c/test_downloader.py:100
> +    def checkout_tests(self, version, url, directory):

I would call this checkout_test_repository instead.

> Tools/Scripts/webkitpy/w3c/test_downloader.py:129
> +    def _should_copy(self, relative_path, name):
> +        if name == '.git':
> +            return False
> +        if relative_path in self.paths_to_skip:
> +            return False
> +        return True

I don't think it's helpful to have a separate method here.  I'd prefer seeing this logic inside _add_test_suite_paths instead.

> Tools/Scripts/webkitpy/w3c/test_importer.py:183
> +        if len(self.options.test_paths) == 0 or self._importing_downloaded_tests:

While we're at it, we should change the first condition to "not self.options.test_paths" to match the WebKit style guide.

[youenn fablet]

Comment on attachment 245166 [details]
Patch

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

>> Tools/Scripts/webkitpy/w3c/test_downloader.py:72
>> +        self._init_paths_from_expectations(self._filesystem.join(self.layout_tests_w3c_resources_path, 'InfrastructurePaths'))
> 
> I don't think we should be using TestExpectations format for InfrastructurePaths.
> That would be very confusion because web-platform-tests/config.default.json, for example, is never ran as a test.
> We should just use a JSON format indicating whether a file should be imported or not.
> r- because of this.

Sounds good.

>> Tools/Scripts/webkitpy/w3c/test_downloader.py:91
>> +        self._run_command(arguments)
> 
> Why are we not using scm/git.py instead?

I was not aware of it.
Checking it now, I will use _run_git method.

>> Tools/Scripts/webkitpy/w3c/test_downloader.py:93
>> +    def tests_version(self, name):
> 
> Is "version" some W3C test specific terminology?  If not, why don't we call it "branch" to match git terminology?

"revision" sounds good.

>> Tools/Scripts/webkitpy/w3c/test_downloader.py:100
>> +    def checkout_tests(self, version, url, directory):
> 
> I would call this checkout_test_repository instead.

OK

>> Tools/Scripts/webkitpy/w3c/test_downloader.py:129
>> +        return True
> 
> I don't think it's helpful to have a separate method here.  I'd prefer seeing this logic inside _add_test_suite_paths instead.

OK

>> Tools/Scripts/webkitpy/w3c/test_importer.py:183
>> +        if len(self.options.test_paths) == 0 or self._importing_downloaded_tests:
> 
> While we're at it, we should change the first condition to "not self.options.test_paths" to match the WebKit style guide.

OK

[youenn fablet]

Created attachment 245348 [details]
Using scm git, changing to JSON for TestInfrastructurePaths and repository revisions

[youenn fablet]

(In reply to comment #24)
> Created attachment 245348 [details]
> Using scm git, changing to JSON for TestInfrastructurePaths and repository
> revisions

I made some small additional modifications:
- Added TestsRevision file into imported/w3c/resources
- If not able to get a proper revision, the script will fail(previously we were returning master)
- Removed verbose option
- No copy of top files starting with '.' and not only '.git'

One small downsides of using _run_git is that no status update is displayed when cloning a repo.

[youenn fablet]

The following features are missing:
- Making git clone not recursive (submodules are not checked in WebKit repo).
- Automatically generate repository submodules decription (imported/w3c/resources/WPTModules)
- Better handling of ImportExpectations. In particular Skip should be working for test files and not only top-level directories.

I can integrate those changes in the current patch.
Since it seems patch is converging, I would tend to file these as separate bugs.

Any review?

[Ryosuke Niwa]

Comment on attachment 245348 [details]
Using scm git, changing to JSON for TestInfrastructurePaths and repository revisions

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

> Tools/Scripts/webkitpy/w3c/test_downloader.py:47
> +    CSS_NAME = 'csswg-tests'
> +    CSS_URL = 'https://github.com/w3c/csswg-test.git'
> +
> +    WPT_NAME = 'web-platform-tests'
> +    WPT_URL = 'https://github.com/w3c/web-platform-tests.git'

Can we use an array/dictionary of name/url pairs instead?

> Tools/Scripts/webkitpy/w3c/test_importer.py:100
> +    import_dir = args[0] if len(args) > 0 else None

if args else None is sufficient. In fact, that's the preferred style in WebKit per our coding guideline.

> LayoutTests/imported/w3c/ChangeLog:16
> +        * resources/ImportExpectations: Added.
> +        * resources/InfrastructurePaths: Added.
> +        * resources/TestRevisions: Added.

It annoys me that there are there different files specifying revisions, files to import, and then a list of files to skip after importing.
I think we should at least combine the first two into one file.

Also, why can't we simply avoid importing without having ImportExpectations?

[youenn fablet]

Created attachment 247225 [details]
Refactoring repository description and download

[youenn fablet]

(In reply to comment #27)
> Comment on attachment 245348 [details]
> Using scm git, changing to JSON for TestInfrastructurePaths and repository
> revisions
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245348&action=review
> 
> > Tools/Scripts/webkitpy/w3c/test_downloader.py:47
> > +    CSS_NAME = 'csswg-tests'
> > +    CSS_URL = 'https://github.com/w3c/csswg-test.git'
> > +
> > +    WPT_NAME = 'web-platform-tests'
> > +    WPT_URL = 'https://github.com/w3c/web-platform-tests.git'
> 
> Can we use an array/dictionary of name/url pairs instead?

I refactored this to put all information in resources/TestRepositories, which also merge resources/InfrastructurePaths and resources/TestRevisions.

> > Tools/Scripts/webkitpy/w3c/test_importer.py:100
> > +    import_dir = args[0] if len(args) > 0 else None
> 
> if args else None is sufficient. In fact, that's the preferred style in
> WebKit per our coding guideline.

OK

> > LayoutTests/imported/w3c/ChangeLog:16
> > +        * resources/ImportExpectations: Added.
> > +        * resources/InfrastructurePaths: Added.
> > +        * resources/TestRevisions: Added.
> 
> It annoys me that there are there different files specifying revisions,
> files to import, and then a list of files to skip after importing.
> I think we should at least combine the first two into one file.

Latest patch merges the last two files in one.

ImportExpectations contain test not to import.
InfrastructurePaths contain folders that need to be copied but should be skipped in the TestExpectations.

> Also, why can't we simply avoid importing without having ImportExpectations?

Initial patches were importing every test in a given folder.
The failing/timeout/crash tests were then marked as skip in the TestExpectations (see bug 134766).

Bem suggested to introduce the ImportExpectations file to capture this list of failing tests. I think it makes sense to do so, there is no real point in importing test that do not assert anything.

[youenn fablet]

Created attachment 247674 [details]
Rebasing and adding already imported tests in ImportExpectations

[youenn fablet]

(In reply to comment #30)
> Created attachment 247674 [details]
> Rebasing and adding already imported tests in ImportExpectations

Patch from bug 142163 (https://bugs.webkit.org/attachment.cgi?id=247676) illustrates how would be used ImportExpectations to progressively import and triage W3C tests.

[WebKit Commit Bot]

Comment on attachment 247674 [details]
Rebasing and adding already imported tests in ImportExpectations

Clearing flags on attachment: 247674

Committed r181479: <http://trac.webkit.org/changeset/181479>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.