Bug 64115 - REGRESSION (r90564): webkitpy's Checkout class uses Executive inappropriately
Summary: REGRESSION (r90564): webkitpy's Checkout class uses Executive inappropriately
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Roben (:aroben)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-07 12:00 PDT by Adam Roben (:aroben)
Modified: 2011-07-07 12:15 PDT (History)
2 users (show)

See Also:


Attachments
Make Checkout use SCM's Executive instead of conjuring up its own (4.08 KB, patch)
2011-07-07 12:08 PDT, Adam Roben (:aroben)
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-07-07 12:00:45 PDT
REGRESSION (r90564): webkitpy's Checkout class uses Executive inappropriately
Comment 1 Adam Roben (:aroben) 2011-07-07 12:08:14 PDT
Created attachment 100015 [details]
Make Checkout use SCM's Executive instead of conjuring up its own
Comment 2 Adam Barth 2011-07-07 12:10:42 PDT
Comment on attachment 100015 [details]
Make Checkout use SCM's Executive instead of conjuring up its own

Thanks Adam.
Comment 3 Adam Roben (:aroben) 2011-07-07 12:14:14 PDT
Committed r90584: <http://trac.webkit.org/changeset/90584>
Comment 4 Eric Seidel (no email) 2011-07-07 12:15:32 PDT
Comment on attachment 100015 [details]
Make Checkout use SCM's Executive instead of conjuring up its own

View in context: https://bugs.webkit.org/attachment.cgi?id=100015&action=review

> Tools/Scripts/webkitpy/common/checkout/checkout.py:123
> +        message_text = self._scm.run([self._scm.script_path('commit-log-editor'), '--print-log'] + changelog_paths, return_stderr=False)

SCM's run may do more than you want... like setting the cwd.  But should be OK.  I would have probably used self._scm._executive directly (even though grabbing at _ members is ugly too).

Part of the trouble here is that scm.py is some of the vey very oldest code in webkitpy, and is poorly tested and factored.  Checkout is pretty old itself.  We've gotten better at this whole mocking thing since then.

Thanks again for the fix.

> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:128
> +        def mock_run(*args, **kwargs):
> +            # Note that we use a real Executive here, not a MockExecutive, so we can test that we're
> +            # invoking commit-log-editor correctly.
> +            return Executive().run_command(*args, **kwargs)
> +

I suspect what we wanted to do was make Checkout hold an self.executive pointer.  But we can do that later when we go clean up the rest of Checkout too.