Bug 68991

Created attachment 108996 [details] Patch

Comment on attachment 108996 [details] Patch 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.

Created attachment 109049 [details] Patch

Created attachment 109051 [details] Patch

All fixed.

Comment on attachment 109051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109051&action=review > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:30 > +import os > +import sys You should rarely, if ever need eithe rof these. FielSystem, Executive and User modules should provide this all in a cross-platform, mockable way. > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:40 > + current_filename = os.path.join(path, watchlist_filename) self._filesystem.join > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:42 > + return self._filesystem.read_text_file(current_filename) You could also pair this with an exists() instead, if you're just tryign to catch the "does not exist" case. > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:47 > + watchlist_filename = os.path.join('webkitpy', 'common', 'config', 'watchlist') You're assuming we're being run from Scripts... which isn't always true. You probably want to use filesystem.path_to_module(self) > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:48 > + for path in sys.path: I see. We don't have this abstracted anywhere, so you have to use sys for sys.path. But I'm confused why you'd look in sys.path? That's the python lookup path for modules? > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:51 > + contents = self._attempt_file_load(path, watchlist_filename) > + if contents is not None: > + return contents Seems you just want to pair exists with the read here, and remove _attempt_file_load. > Tools/Scripts/webkitpy/common/watchlist/watchlistloader_unittest.py:40 > + loader = WatchListLoader(filesystem_mock.MockFileSystem()) > + self.assertRaisesRegexp(r'Watch list file \(webkitpy/common/config/watchlist\) not found in the python search path\.', You can test that it's not found, and then test what happens when it is found, both with a MockFileSystem. > Tools/Scripts/webkitpy/common/watchlist/watchlistloader_unittest.py:45 > + WatchListLoader(filesystem.FileSystem()).load() It's rare that you want to use a real filesystem in a unittest.

Comment on attachment 109051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109051&action=review This is looking good. Let do one more iteration. > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:40 > + current_filename = os.path.join(path, watchlist_filename) self._filesystem.join <-- makes unit testing easier on Mac and Windows. > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:47 > + watchlist_filename = os.path.join('webkitpy', 'common', 'config', 'watchlist') Does this assume something about the current working directory? You probably want something relative to the current source directory, which is something the Checkout class knows about. Another alternative is to find the file relative to the currently executing python. These are different, for example, if you run the webkit-patch script from one webkit checkout while the current working directory is in another. I'd expect this to work like commiters.py, which is to say relative to the executing python (as if loaded as a module). You can use: self._filesystem.path_to_module(WatchListLoader) Going through self._filesystem makes this easy to mock during testing. > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:52 > + raise Exception('Watch list file (%s) not found in the python search path.' % watchlist_filename) I'm not sure "python search path" is what you mean here. Why not just say that a given path was not found?

I enjoy that Eric said the same things I did, plus he caught a bunch more. :)

(In reply to comment #6) > (From update of attachment 109051 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109051&action=review > > Tools/Scripts/webkitpy/common/watchlist/watchlistloader_unittest.py:45 > > + WatchListLoader(filesystem.FileSystem()).load() > > It's rare that you want to use a real filesystem in a unittest. I wanted to load the actual checked in watchlist to verify that there are no mistakes in it. Because it is bad news, if there are. The watchlist bot will completely break.

Created attachment 109067 [details] Patch

I fixed everything except I still access the real filesystem to load the checked in watchlist as a test. I not only want to verify that I can actually load it but I want the integrity checks that will happen as a part of that load, so that any mistakes made in that file are caught quickly (by the bots running python tests if nothing else).

Comment on attachment 109067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109067&action=review Seems OK. Please look at the nits I posted. > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:36 > + def _get_watch_list_contents(self): We don't normally use _get_ in WebKit method names. You could use _read_watch_list_contents. Except I still don't think you want this method. You don't need it for unittesting anymore, now that you're using Filesystem. > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:42 > + except: > + raise Exception('Watch list file (%s) not found.' % watch_list_full_path) This isn't true. You're converting any exception into a "not found" custom exception. I think you should instead remove the exception handling and guard your read in self._filesystem.exists() instead, returning None if it doesn't exist. Then if there si some real read problem you'll see that as a real exception, instead of mutated to your custom one. > Tools/Scripts/webkitpy/common/watchlist/watchlistloader_unittest.py:41 > + self.assertRaisesRegexp(r'Watch list file \(.*/watchlist\) not found\.', > + loader.load) We don't match PEP8 wrapping. You're welcome to wrap wherever you feel is most readable. > Tools/Scripts/webkitpy/common/watchlist/watchlistloader_unittest.py:45 > + def test_watch_list_load(self): > + # Just verifying that loading the checked in watch list works. > + WatchListLoader(filesystem.FileSystem()).load() Ok, so it wasn't immediately clear to me. This is a parsing test for the checked-in watch list. > Tools/Scripts/webkitpy/common/webkitunittest.py:41 > + def _verifyException(self, regex_message, callable, *args): I might name this _assertRaisesRegexp. :) I guess one might worry that could conflict with some other TestCase method...

(In reply to comment #12) > (From update of attachment 109067 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109067&action=review > > Seems OK. Please look at the nits I posted. > > > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:36 > > + def _get_watch_list_contents(self): > > We don't normally use _get_ in WebKit method names. You could use _read_watch_list_contents. Except I still don't think you want this method. You don't need it for unittesting anymore, now that you're using Filesystem. I'll get rid of it :) > > > Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:42 > > + except: > > + raise Exception('Watch list file (%s) not found.' % watch_list_full_path) > > This isn't true. You're converting any exception into a "not found" custom exception. I think you should instead remove the exception handling and guard your read in self._filesystem.exists() instead, returning None if it doesn't exist. Then if there si some real read problem you'll see that as a real exception, instead of mutated to your custom one. You said this before and I messed up on the follow through. Thanks for pointing it out again. I'll address everything you said and check in. Thanks again!

Committed as http://trac.webkit.org/changeset/96263