[webkit-reviews] review denied: [Bug 47520] new-run-webkit-tests: operations that look at the tree should be isolated : [Attachment 70496] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 47520: new-run-webkit-tests: operations that look at the tree should be
isolated
https://bugs.webkit.org/show_bug.cgi?id=47520

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

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

I'm confused by the design here.

> WebKitTools/Scripts/webkitpy/layout_tests/port/tree.py:30
> +"""Wrapper objects for the source tree and utility routines."""

I'm confused how this is different from common/checkout/api.py

> WebKitTools/Scripts/webkitpy/layout_tests/port/tree.py:56
> +class MockExecutive(object):

Seems this belongs in the _unittest.py file, not here.

> WebKitTools/Scripts/webkitpy/layout_tests/port/tree.py:115
> +    def default_configuration(self):

it seems this class does 2 things.  1.	know how to call build-*, and 2.  know
how to deal with the WebKit configuration file.  Seems the configuration stuff
should just be a separate class, then I'm not sure if Tree needs to hold the
build_ stuff.

> WebKitTools/Scripts/webkitpy/layout_tests/port/tree.py:216
> +class MockTree(Tree):

Again, goes in _unittest.py or in mocktool.py (which is our common mocking
file).