AbstractQueue needs better unit testing

• Component: Tools / Tests
• Status: NEW ---

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 43650 [details]
Patch

[Adam Barth]

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)]

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]

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]

Also, how does your test run on Windows?  I think you'd rather use the real bugzilla-tool with --help

[Eric Seidel (no email)]

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)]

Committed r51284: <http://trac.webkit.org/changeset/51284>

[Eric Seidel (no email)]

Leaving this open and changing the title to reflect that what's left to solve here is unit testing.