Bug 108262

Summary: Tidy up Sheriffbot's Sheriffs command's unit tests
Product: WebKit Reporter: Alan Cutter <alancutter>
Component: Tools / TestsAssignee: Alan Cutter <alancutter>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

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 1 Alan Cutter 2013-01-29 18:12:58 PST
Created attachment 185366 [details]
Patch
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.