Bug 142085 – W3C test importer should use filesystem instead of os.walk

• Component: Tools / Tests
• Status: RESOLVED FIXED

[youenn fablet]

W3C test importer uses os.walk, while it should use filesystem for that.

[youenn fablet]

Created attachment 247514 [details]
Patch

[Build Bot]

Comment on attachment 247514 [details]
Patch

Attachment 247514 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4770151262060544

New failing tests:
fast/css/object-fit/object-fit-canvas.html

[Build Bot]

Created attachment 247656 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[youenn fablet]

(In reply to comment #2)
> Comment on attachment 247514 [details]
> Patch
> 
> Attachment 247514 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/4770151262060544
> 
> New failing tests:
> fast/css/object-fit/object-fit-canvas.html

Putting r?/cq as this patch should have no impact on this particular test.

[Bem Jones-Bey]

Comment on attachment 247514 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/filesystem.py:80
> +    def dirs_under(self, path, dir_filter=None):

It would be nice to have a unit test for this.

> Tools/Scripts/webkitpy/common/system/filesystem.py:93
> +        for (dirpath, dirnames, filenames) in os.walk(path):

If you wrap os.walk in a method (like _os_walk), then you only have to mock that method instead of copying this entire method into the MockFileSystem. Also, you'll be able to write unit tests for this method.

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:179
> +    def getsize(self, path):
> +        if not self.isfile(path):
> +            raise OSError("%s is not a directory" % path)
> +        return len(self.files[path])

Why did you add this?

[youenn fablet]

(In reply to comment #5)
> Comment on attachment 247514 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247514&action=review
> 
> > Tools/Scripts/webkitpy/common/system/filesystem.py:80
> > +    def dirs_under(self, path, dir_filter=None):
> 
> It would be nice to have a unit test for this.

Ok

> > Tools/Scripts/webkitpy/common/system/filesystem.py:93
> > +        for (dirpath, dirnames, filenames) in os.walk(path):
> 
> If you wrap os.walk in a method (like _os_walk), then you only have to mock
> that method instead of copying this entire method into the MockFileSystem.
> Also, you'll be able to write unit tests for this method.

I am not sure to understand.
MockFileSystem is totally independent of FileSystem. We need to add MockFileSystem.dirs_under anyway.

> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:179
> > +    def getsize(self, path):
> > +        if not self.isfile(path):
> > +            raise OSError("%s is not a directory" % path)
> > +        return len(self.files[path])
> 
> Why did you add this?

It is needed to run the unit test as it is used in TestImporter.
Rereading it, there is obviously a typo in the exception description.

[youenn fablet]

Created attachment 247753 [details]
Adding unit tests

[Bem Jones-Bey]

Comment on attachment 247753 [details]
Adding unit tests

Looks good. r=me.

[WebKit Commit Bot]

Comment on attachment 247753 [details]
Adding unit tests

Clearing flags on attachment: 247753

Committed r181014: <http://trac.webkit.org/changeset/181014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.