[webkit-reviews] review denied: [Bug 31923] [bzt] Unit test download commands : [Attachment 43938] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 31923: [bzt] Unit test download commands
https://bugs.webkit.org/show_bug.cgi?id=31923

Attachment 43938: Patch
https://bugs.webkit.org/attachment.cgi?id=43938&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I don't understand why you're adding "return tool":
 43	    return tool

Why?
232		 bugs_to_patches[bug_id] = bugs_to_patches.get(bug_id,
[]).append(patch)
 234		 bugs_to_patches[bug_id] = bugs_to_patches.get(bug_id, []) +
[patch]

Does .append() return none?  Was it not working before?

I'm not sure I understand:
 36	def _default_options(self):
either, but it's nice to list them all.

Why does MockBugzilla inherit from Mock?
 34 class MockBugzilla(Mock):

I'm not saying it shouldn't, just not sure what that adds.

I would think that we would make:
 35	patch1 = {
into an array eventually.  THen the fetch_attachment lookup is super-simple.

Is this gonna be a problem?
 93	    self.checkout_root = os.getcwd()
Does it ever touch the disk?  Will it end up leaving turd files as a result of
the test, or fail if you run-webkit-tests from a non-scm directory?

Otherwise looks OK.  The return and .append change are at least un-explained. 
The return tool change might be "wrong".