[webkit-reviews] review denied: [Bug 68991] watchlist: Add a way to load the watchlist from config. : [Attachment 108996] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied David Levin <levin at chromium.org>'s
request for review:
Bug 68991: watchlist: Add a way to load the watchlist from config.
https://bugs.webkit.org/show_bug.cgi?id=68991

Attachment 108996: Patch
https://bugs.webkit.org/attachment.cgi?id=108996&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=108996&action=review


> Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:31
> +from webkitpy.common.watchlist.watchlistparser import WatchListParser

We usually put a blank line between the system imports and the webkitpy
imports.

> Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:39
> +    def _attempt_file_load(self, path, watchlist_filename):
> +	   try:
> +	       return open(os.path.join(path, watchlist_filename), 'r').read()
> +	   except:
> +	       return None

Rather than talking directly to the file system, we use the filesystem
abstraction.  That lets us test this sort of code without talking to the real
filesystem via dependency injection.  This class should take a filesystem
object as a parameter, which we'll eventually get from the tool (aka host)
object given to the command.

> Tools/Scripts/webkitpy/common/watchlist/watchlistloader_unittest.py:37
> +def _return_none(*args):
> +    return None

You can write this inline using  lambda.

> Tools/Scripts/webkitpy/common/watchlist/watchlistloader_unittest.py:46
> +    def _verifyException(self, regex_message, callable, *args):

Rather than copy/pasting this code, can we create a subclass of
unittest.TestCase that adds this functionality and then derive our test classes
from it?

> Tools/Scripts/webkitpy/common/watchlist/watchlistloader_unittest.py:58
> +    def test_watch_list_not_found(self):
> +	   loader = WatchListLoader()
> +	   loader._attempt_file_load = _return_none
> +	   self.assertRaisesRegexp(r'Watch list file
\(webkitpy/common/config/watchlist\) not found in the python search path\.',
> +				   loader.load)

Here you'll want to use a MockFilesystem, which you can populate with test
data.