Created attachment 247514 [details] Patch

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

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

(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 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?

(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.

Created attachment 247753 [details] Adding unit tests

Comment on attachment 247753 [details] Adding unit tests Looks good. r=me.

Comment on attachment 247753 [details] Adding unit tests Clearing flags on attachment: 247753 Committed r181014: <http://trac.webkit.org/changeset/181014>

All reviewed patches have been landed. Closing bug.