Bug 52691

Summary: new-run-webkit-tests: remove use of os.walk, use mock filesystem for better unit testing
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, koz, mihaip, ojan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
revise approach to files_under() so we can handle reftests properly
none
update w/ feedback from mihaip's review
none
pull strcats out of loop in MockFileSystem.files_under() none

Description Dirk Pranke 2011-01-18 17:34:41 PST
new-run-webkit-tests: remove use of os.walk, use mock filesystem for better unit testing
Comment 1 Dirk Pranke 2011-01-18 17:41:02 PST
Created attachment 79369 [details]
Patch
Comment 2 Dirk Pranke 2011-01-18 18:18:50 PST
Created attachment 79375 [details]
revise approach to files_under() so we can handle reftests properly
Comment 3 Mihai Parparita 2011-01-19 07:46:56 PST
Comment on attachment 79375 [details]
revise approach to files_under() so we can handle reftests properly

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

> Tools/Scripts/webkitpy/common/system/filesystem.py:76
> +        dirs_to_skip = dirs_to_skip or []

You can use [] as the default argument value to the function, since you're not modifying the list.

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:75
> +        dirs_to_skip = dirs_to_skip or []

Same here.

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:100
> +                    continue

This "continue" will only apply to the loop over dirs_to_skip, but I think you want to continue the iteration in the self.files loop.
Comment 4 Dirk Pranke 2011-01-19 12:12:08 PST
(In reply to comment #3)
> (From update of attachment 79375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79375&action=review
> 
> > Tools/Scripts/webkitpy/common/system/filesystem.py:76
> > +        dirs_to_skip = dirs_to_skip or []
> 
> You can use [] as the default argument value to the function, since you're not modifying the list.
>

Fair enough. Done.
 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:75
> > +        dirs_to_skip = dirs_to_skip or []
> 
> Same here.
>

Ditto. 
 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:100
> > +                    continue
> 
> This "continue" will only apply to the loop over dirs_to_skip, but I think you want to continue the iteration in the self.files loop.

Good catch. Fixed.
Comment 5 Dirk Pranke 2011-01-19 12:18:59 PST
Created attachment 79462 [details]
update w/ feedback from mihaip's review
Comment 6 Dirk Pranke 2011-01-19 12:50:57 PST
Created attachment 79470 [details]
pull strcats out of loop in MockFileSystem.files_under()
Comment 7 Dirk Pranke 2011-01-19 13:30:17 PST
Comment on attachment 79470 [details]
pull strcats out of loop in MockFileSystem.files_under()

Clearing flags on attachment: 79470

Committed r76155: <http://trac.webkit.org/changeset/76155>
Comment 8 Dirk Pranke 2011-01-19 13:30:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Eric Seidel (no email) 2011-01-19 13:51:00 PST
This broke tests.

======================================================================
ERROR: test_files_in_namelist (webkitpy.common.system.directoryfileset_unittest.DirectoryFileSetTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/directoryfileset_unittest.py", line 42, in test_files_in_namelist
    self.assertTrue('some-file' in self._fileset.namelist())
  File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/directoryfileset.py", line 61, in namelist
    return map(self._drop_directory_prefix, self._files_in_directory())
  File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/directoryfileset.py", line 52, in _files_in_directory
    return self._filesystem.files_under(self._path)
  File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/filesystem_mock.py", line 85, in files_under
    if self.basename(path) in dirs_to_skip:
TypeError: basename() takes exactly 1 argument (2 given)

----------------------------------------------------------------------
Comment 10 Dirk Pranke 2011-01-19 14:08:58 PST
(In reply to comment #9)
> This broke tests.
> 
> ======================================================================
> ERROR: test_files_in_namelist (webkitpy.common.system.directoryfileset_unittest.DirectoryFileSetTest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/directoryfileset_unittest.py", line 42, in test_files_in_namelist
>     self.assertTrue('some-file' in self._fileset.namelist())
>   File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/directoryfileset.py", line 61, in namelist
>     return map(self._drop_directory_prefix, self._files_in_directory())
>   File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/directoryfileset.py", line 52, in _files_in_directory
>     return self._filesystem.files_under(self._path)
>   File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/filesystem_mock.py", line 85, in files_under
>     if self.basename(path) in dirs_to_skip:
> TypeError: basename() takes exactly 1 argument (2 given)
> 
> ----------------------------------------------------------------------

Fixed in r76158.
Comment 11 WebKit Review Bot 2011-01-19 14:41:06 PST
http://trac.webkit.org/changeset/76155 might have broken GTK Linux 32-bit Release
The following tests are not passing:
fast/forms/input-text-scroll-left-on-blur.html
fast/forms/plaintext-mode-2.html