Bug 161578 – W3C test importer should generate the list of resource files

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Following on bug 161307, W3C test importer should generate the list of resource files.

[youenn fablet]

Created attachment 287918 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 287918 [details]
Patch

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

> Tools/Scripts/webkitpy/w3c/test_importer.py:473
> +                if not self._already_identified_as_resource_file(resource_file_path):
> +                    files.append(resource_file_path)
> +                    should_update_json_file = True

This would not catch the case when a resource file becomes a test.

> Tools/Scripts/webkitpy/w3c/test_importer.py:484
> +        for directory in self._test_resource_files["directories"]:
> +            if path.find(directory) != -1:
> +                return True
> +        return False

This should just be:
return any([path.find(directory) != -1 for directory in self._test_resource_files["directories"]])

[youenn fablet]

Created attachment 287936 [details]
Patch

[youenn fablet]

Thanks for the review.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287918&action=review
> 
> > Tools/Scripts/webkitpy/w3c/test_importer.py:473
> > +                if not self._already_identified_as_resource_file(resource_file_path):
> > +                    files.append(resource_file_path)
> > +                    should_update_json_file = True
> 
> This would not catch the case when a resource file becomes a test.

I added a FIXME for it.
In the meantime, the plan is that on each full resync, the list of resource files will be fully regenerated.

I also added generation of a warning for tests imported in "resources" folder.


> > Tools/Scripts/webkitpy/w3c/test_importer.py:484
> > +        for directory in self._test_resource_files["directories"]:
> > +            if path.find(directory) != -1:
> > +                return True
> > +        return False
> 
> This should just be:
> return any([path.find(directory) != -1 for directory in
> self._test_resource_files["directories"]])

Done.

[Ryosuke Niwa]

Comment on attachment 287936 [details]
Patch

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

> Tools/Scripts/webkitpy/w3c/test_importer.py:475
> +                resource_file_path = full_path.replace(self.source_directory, '')[1:]

Presumably you're stripping the trailing / with [1:] but this is not robust nor is semantically descriptive.
Use .rstrip('/') instead. An even better approach is to use self.filesystem.relpath.

[youenn fablet]

Created attachment 287944 [details]
Patch for landing

[youenn fablet]

(In reply to comment #5)
> Comment on attachment 287936 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287936&action=review
> 
> > Tools/Scripts/webkitpy/w3c/test_importer.py:475
> > +                resource_file_path = full_path.replace(self.source_directory, '')[1:]
> 
> Presumably you're stripping the trailing / with [1:] but this is not robust
> nor is semantically descriptive.
> Use .rstrip('/') instead. An even better approach is to use
> self.filesystem.relpath.

Done, thanks.

[WebKit Commit Bot]

Comment on attachment 287944 [details]
Patch for landing

Clearing flags on attachment: 287944

Committed r205447: <http://trac.webkit.org/changeset/205447>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

It broke a python test:

[1432/1462] webkitpy.w3c.test_importer_unittest.TestImporterTest.test_clean_directory_option erred:
  Traceback (most recent call last):
    File "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py", line 204, in test_clean_directory_option
      fs = self.import_downloaded_tests(['--no-fetch', '--import-all', '-d', 'w3c', '--clean-dest-dir'], FAKE_FILES)
    File "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py", line 132, in import_downloaded_tests
      importer = TestImporter(host, None, options)
    File "/home/slave/webkitgtk/gtk-linux-64-release-tests/build/Tools/Scripts/webkitpy/w3c/test_importer.py", line 184, in __init__
      self._test_resource_files["files"] = []
  TypeError: 'NoneType' object does not support item assignment

[youenn fablet]

This should be covered by bug 161604, let me know if you still see broken tests.