Bug 64115 – REGRESSION (r90564): webkitpy's Checkout class uses Executive inappropriately

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adam Roben (:aroben)]

REGRESSION (r90564): webkitpy's Checkout class uses Executive inappropriately

[Adam Roben (:aroben)]

Created attachment 100015 [details]
Make Checkout use SCM's Executive instead of conjuring up its own

[Adam Barth]

Comment on attachment 100015 [details]
Make Checkout use SCM's Executive instead of conjuring up its own

Thanks Adam.

[Adam Roben (:aroben)]

Committed r90584: <http://trac.webkit.org/changeset/90584>

[Eric Seidel (no email)]

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.