[webkit-reviews] review denied: [Bug 54374] Add method to make BuildBot return test outputs : [Attachment 83604] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied  review:
Bug 54374: Add method to make BuildBot return test outputs
https://bugs.webkit.org/show_bug.cgi?id=54374

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83604&action=review

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:98
> +	   revision_build_pairs = self.revision_build_pairs_with_results()

I don't remember.  Isn't this very expensive to compute?

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:232
> +    def results_zip_url(self):

Please test this.

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:418
> +	   class MockBuilder(Builder):

No need to make this local to the test.  These are very often shared between
more than one test.

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:420
> +		   Builder.__init__(self, 'mock builder', BuildBot())

Mocks shouldn't inherit from real objets.  If you want to test hte real object,
just mock out the methods.

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:422
> +	       def _fetch_build(self, build_number):

I would have probably just replaced the methods on a real object.

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:425
> +		   else:

No else after return in WEbKIt.