|Summary:||Tidy up Sheriffbot's Sheriffs command's unit tests|
|Product:||WebKit||Reporter:||Alan Cutter <alancutter>|
|Component:||Tools / Tests||Assignee:||Alan Cutter <alancutter>|
|Severity:||Normal||CC:||abarth, dpranke, eric, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Alan Cutter 2013-01-29 17:08:14 PST
From bug 105698. (In reply to comment #19) > (From update of attachment 181006 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181006&action=review > > > Tools/Scripts/webkitpy/tool/bot/irc_command.py:147 > > + sheriff_js = Web().get_binary(url, True) > > rather than creating a Web() object, you should've used the web member of the tool object passed to you in execute(). Then you could've used tests that had a mocked out web object, rather than fetching test files. Generally speaking, we prefer tests that are self-contained with inline data; using the test files means hitting the filesystem which is slow and a bit more cumbersome. > > > Tools/Scripts/webkitpy/tool/bot/testdata/webkit_sheriff_zero.js:1 > > +document.write(''); > > I don't this file is actually being used, is it?
Comment 2 Eric Seidel (no email) 2013-01-29 18:18:45 PST
Comment on attachment 185366 [details] Patch LGTM. I'm glad you've embraced the beauty of mocks. :)
Comment 3 WebKit Review Bot 2013-01-29 19:02:22 PST
Comment on attachment 185366 [details] Patch Clearing flags on attachment: 185366 Committed r141206: <http://trac.webkit.org/changeset/141206>
Comment 4 WebKit Review Bot 2013-01-29 19:02:26 PST
All reviewed patches have been landed. Closing bug.