Bug 142085

Summary: W3C test importer should use filesystem instead of os.walk
Product: WebKit Reporter: youenn fablet <youennf>
Component: Tools / TestsAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: bjonesbe, buildbot, commit-queue, glenn, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 142012, 142084    
Bug Blocks: 134764    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
Adding unit tests none

Description youenn fablet 2015-02-27 07:16:56 PST
W3C test importer uses os.walk, while it should use filesystem for that.
Comment 1 youenn fablet 2015-02-27 07:45:09 PST
Created attachment 247514 [details]
Patch
Comment 2 Build Bot 2015-03-02 01:10:47 PST
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
Comment 3 Build Bot 2015-03-02 01:10:49 PST
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
Comment 4 youenn fablet 2015-03-02 04:43:32 PST
(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.
Comment 5 Bem Jones-Bey 2015-03-02 10:43:01 PST
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?
Comment 6 youenn fablet 2015-03-02 11:46:10 PST
(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.
Comment 7 youenn fablet 2015-03-03 00:31:30 PST
Created attachment 247753 [details]
Adding unit tests
Comment 8 Bem Jones-Bey 2015-03-04 12:10:01 PST
Comment on attachment 247753 [details]
Adding unit tests

Looks good. r=me.
Comment 9 WebKit Commit Bot 2015-03-04 13:02:57 PST
Comment on attachment 247753 [details]
Adding unit tests

Clearing flags on attachment: 247753

Committed r181014: <http://trac.webkit.org/changeset/181014>
Comment 10 WebKit Commit Bot 2015-03-04 13:03:02 PST
All reviewed patches have been landed.  Closing bug.