Bug 31769

Summary: AbstractQueue needs better unit testing
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: abarth
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Description Flags
Patch abarth: review-

Description Eric Seidel (no email) 2009-11-21 08:26:33 PST
AbstractQueue.run_bugzilla_tool throws an exception

It should use self.tool instead of tool.

Add a unit test to make sure this doesn't happen again.
Comment 1 Eric Seidel (no email) 2009-11-21 08:30:02 PST
Created attachment 43650 [details]
Comment 2 Adam Barth 2009-11-21 08:33:15 PST
Comment on attachment 43650 [details]

i think you should subclass the real MockBugzillaTool
and override the path method
we want MockBugzillaTool to grow to be able to mock all the features of the real bugzilla tool
Comment 3 Eric Seidel (no email) 2009-11-21 08:47:12 PST
I don' think it's right to add this to the MockBugzillaTool.  use of echo is kinda a hack.  I'm not sure we always want run_bugzilla_tool() to even return a value.  And if it returns a value, I'm not sure it should be the output.  So I'd rather keep this hack self contained instead of adding it to some "global mock".  Also this is better locality of code putting it here, making the test easier to read.
Comment 4 Adam Barth 2009-11-21 08:49:52 PST
I don't understand your argument.  I'd rather see a test that mocked out both WorkQueue and BugzillaTool and didn't change run_bugzilla_tool to return a value.  To test this bug, you don't actually need to validate that we created a subprocess.  You just need to validate that we don't throw a bogus exception.
Comment 5 Adam Barth 2009-11-21 08:53:05 PST
Also, how does your test run on Windows?  I think you'd rather use the real bugzilla-tool with --help
Comment 6 Eric Seidel (no email) 2009-11-21 13:10:09 PST
I'm going to land the 5 char fix to unblock the commit-queue and then we can debate the proper way to unit test this all in person.
Comment 7 Eric Seidel (no email) 2009-11-21 13:13:18 PST
Committed r51284: <http://trac.webkit.org/changeset/51284>
Comment 8 Eric Seidel (no email) 2009-11-21 13:14:03 PST
Leaving this open and changing the title to reflect that what's left to solve here is unit testing.