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   
Attachments:
Description Flags
Patch abarth: review-

Eric Seidel (no email)
Reported 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.
Attachments
Patch (5.49 KB, patch)
2009-11-21 08:30 PST, Eric Seidel (no email)
abarth: review-
Eric Seidel (no email)
Comment 1 2009-11-21 08:30:02 PST
Adam Barth
Comment 2 2009-11-21 08:33:15 PST
Comment on attachment 43650 [details] Patch 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
Eric Seidel (no email)
Comment 3 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.
Adam Barth
Comment 4 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.
Adam Barth
Comment 5 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
Eric Seidel (no email)
Comment 6 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.
Eric Seidel (no email)
Comment 7 2009-11-21 13:13:18 PST
Eric Seidel (no email)
Comment 8 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.
Note You need to log in before you can comment on or make changes to this bug.