Enhance MockRemote and friends to cover SFTP too

Backwards compatible emphasis. "Good enough for now" emphasis as well.

Surprised how overcomplicated and poorly-fit-for-purpose this module is
5 years later, though in my defense it was really written for the "our
own doctests" use case which looks a LOT different from regular unit
tests.

Likely we want to really just rewrite the entire top level API in 4.0,
do away with any concept of multiple sessions, etc.

fabric/testing/base.py

@@ -24,6 +24,10 @@
 from deprecated.classic import deprecated as deprecated_no_docstring
 
 
+# TODO 4.0: reorganize harder (eg building blocks in one module, central
+# classes in another?)
+
+
 class Command:
     """
     Data record specifying params of a command execution to mock/expect.
@@ -141,7 +145,17 @@ class Session:
             Giving ``cmd``, ``out`` etc alongside explicit ``commands`` is not
             allowed and will result in an error.
 
+    :param bool enable_sftp: Whether to enable basic SFTP mocking support.
+
+    :param transfers:
+        None if no transfers to expect; otherwise, should be a list of dicts of
+        the form ``{"method": "get|put", **kwargs}`` where ``**kwargs`` are the
+        kwargs expected in the relevant `~paramiko.sftp_client.SFTPClient`
+        method. (eg: ``{"method": "put", "localpath": "/some/path"}``)
+
     .. versionadded:: 2.1
+    .. versionchanged:: 3.2
+        Added the ``enable_sftp`` and ``transfers`` parameters.
     """
 
     def __init__(
@@ -156,6 +170,8 @@ def __init__(
         err=None,
         exit=None,
         waits=None,
+        enable_sftp=False,
+        transfers=None,
     ):
         # Safety check
         params = cmd or out or err or exit or waits
@@ -188,6 +204,8 @@ def __init__(
             self.commands = [Command(**kwargs)]
         if not self.commands:
             self.commands = [Command()]
+        self._enable_sftp = enable_sftp
+        self.transfers = transfers
 
     def generate_mocks(self):
         """
@@ -243,12 +261,44 @@ def generate_mocks(self):
         # open_session() is called.
         transport.open_session.side_effect = channels
 
+        # SFTP, if enabled
+        if self._enable_sftp:
+            self._start_sftp(client)
+
         self.client = client
         self.channels = channels
 
+    def _start_sftp(self, client):
+        # Patch os module for local stat and similar
+        self.os_patcher = patch("fabric.transfer.os")
+        mock_os = self.os_patcher.start()
+        # Patch Path class inside transfer.py to prevent real fs touchery
+        self.path_patcher = patch("fabric.transfer.Path")
+        self.path_patcher.start()
+        self.sftp = sftp = client.open_sftp.return_value
+
+        # Handle common filepath massage actions; tests will assume these.
+        def fake_abspath(path):
+            # Run normpath to avoid tests not seeing abspath wrinkles (like
+            # trailing slash chomping)
+            return "/local/{}".format(os.path.normpath(path))
+
+        mock_os.path.abspath.side_effect = fake_abspath
+        sftp.getcwd.return_value = "/remote"
+        # Ensure stat st_mode is a real number; Python 3's stat.S_IMODE doesn't
+        # like just being handed a MagicMock?
+        fake_mode = 0o644  # arbitrary real-ish mode
+        sftp.stat.return_value.st_mode = fake_mode
+        mock_os.stat.return_value.st_mode = fake_mode
+        # Not super clear to me why the 'wraps' functionality in mock isn't
+        # working for this :( reinstate a bunch of os(.path) so it still works
+        mock_os.sep = os.sep
+        for name in ("basename", "split", "join", "normpath"):
+            getattr(mock_os.path, name).side_effect = getattr(os.path, name)
+
     @deprecated_no_docstring(
         version="3.2",
-        reason="This method has been renamed to `safety_check` & will be removed in 4.0",
+        reason="This method has been renamed to `safety_check` & will be removed in 4.0",  # noqa
     )
     def sanity_check(self):
         return self.safety_check()
@@ -282,29 +332,55 @@ def safety_check(self):
         calls = transport.return_value.open_session.call_args_list
         assert calls == session_opens
 
+        # SFTP transfers
+        for transfer in self.transfers or []:
+            method_name = transfer.pop("method")
+            method = getattr(self.sftp, method_name)
+            method.assert_any_call(**transfer)
+
+    def stop(self):
+        """
+        Stop any internal per-session mocks.
+
+        .. versionadded:: 3.2
+        """
+        if hasattr(self, "os_patcher"):
+            self.os_patcher.stop()
+        if hasattr(self, "path_patcher"):
+            self.path_patcher.stop()
+
 
 class MockRemote:
     """
-    Class representing mocked remote state.
+    Class representing mocked remote SSH/SFTP state.
 
-    By default this class is set up for start/stop style patching as opposed to
-    the more common context-manager or decorator approach; this is so it can be
-    used in situations requiring setup/teardown semantics (such as inside a
-    `pytest fixture <fabric.testing.fixtures>`).
+    It supports stop/start style patching (useful for doctests) but then wraps
+    that in a more convenient/common contextmanager pattern (useful in most
+    other situations). The latter is also leveraged by the
+    `fabric.testing.fixtures` module, recommended if you're using pytest.
 
-    By default, this class creates a single anonymous/internal `Session`, for
+    Note that the `expect` and `expect_sessions` methods automatically call
+    `start`, so you won't normally need to do so by hand.
+
+    By default, a single anonymous/internal `Session` is created, for
     convenience (eg mocking out SSH functionality as a safety measure). Users
     requiring detailed remote session expectations can call methods like
     `expect` or `expect_sessions`, which wipe that anonymous Session & set up a
     new one instead.
 
     .. versionadded:: 2.1
+    .. versionchanged:: 3.2
+        Added the ``enable_sftp`` init kwarg to enable mocking both SSH and
+        SFTP at the same time.
+    .. versionchanged:: 3.2
+        Added contextmanager semantics to the class, so you don't have to
+        remember to call `safety`/`stop`.
     """
 
-    def __init__(self):
-        self.expect_sessions(Session())
-
-    # TODO: make it easier to assume single session w/ >1 command?
+    # TODO 4.0: delete enable_sftp and make its behavior default
+    def __init__(self, enable_sftp=False):
+        self._enable_sftp = enable_sftp
+        self.expect_sessions(Session(enable_sftp=enable_sftp))
 
     def expect(self, *args, **kwargs):
         """
@@ -314,6 +390,7 @@ def expect(self, *args, **kwargs):
 
         .. versionadded:: 2.1
         """
+        kwargs.setdefault("enable_sftp", self._enable_sftp)
         return self.expect_sessions(Session(*args, **kwargs))[0]
 
     def expect_sessions(self, *sessions):
@@ -332,6 +409,8 @@ def expect_sessions(self, *sessions):
         # And start patching again, returning mocked channels
         return self.start()
 
+    # TODO 4.0: definitely clean this up once the SFTP bit isn't opt-in, doing
+    # that backwards compatibly was real gross
     def start(self):
         """
         Start patching SSHClient with the stored sessions, returning channels.
@@ -348,10 +427,13 @@ def start(self):
             session.generate_mocks()
             clients.append(session.client)
         # Each time the mocked SSHClient class is instantiated, it will
-        # yield one of our mocked clients (w/ mocked transport & channel)
-        # generated above.
+        # yield one of our mocked clients (w/ mocked transport & channel, and
+        # optionally SFTP subclient) generated above.
         SSHClient.side_effect = clients
-        return list(chain.from_iterable(x.channels for x in self.sessions))
+        sessions = list(chain.from_iterable(x.channels for x in self.sessions))
+        # TODO: in future we _may_ want to change this so it returns SFTP file
+        # data as well?
+        return sessions
 
     def stop(self):
         """
@@ -364,10 +446,13 @@ def stop(self):
             return
         # Stop patching SSHClient
         self.patcher.stop()
+        # Also ask all sessions to stop any of their self-owned mocks
+        for session in self.sessions:
+            session.stop()
 
     @deprecated(
         version="3.2",
-        reason="This method has been renamed to `safety` & will be removed in 4.0",
+        reason="This method has been renamed to `safety` & will be removed in 4.0",  # noqa
     )
     def sanity(self):
         """
@@ -386,9 +471,20 @@ def safety(self):
         for session in self.sessions:
             session.safety_check()
 
+    def __enter__(self):
+        return self
+
+    def __exit__(self, *exc):
+        try:
+            self.safety()
+        finally:
+            self.stop()
+
 
-# TODO: unify with the stuff in paramiko itself (now in its tests/conftest.py),
-# they're quite distinct and really shouldn't be.
+@deprecated(
+    version="3.2",
+    reason="This class has been merged with `MockRemote` which can now handle SFTP mocking too. Please switch to it!",  # noqa
+)
 class MockSFTP:
     """
     Class managing mocked SFTP remote state.

fabric/testing/fixtures.py

@@ -68,6 +68,23 @@ def connection():
 cxn = connection
 
 
+# TODO 4.0: remove old remote() and make this the new remote()
+@fixture
+def remote_with_sftp():
+    """
+    Like `remote`, but with ``enable_sftp=True``.
+
+    To access the internal mocked SFTP client (eg for asserting SFTP
+    functionality was called), note that the returned `MockRemote` object has a
+    ``.sftp`` attribute when created in this mode.
+    """
+    # NOTE: recall that by default an instantiated MockRemote has a single
+    # internal anonymous session; so these fixtures are useful for autouse
+    # guardrails.
+    with MockRemote(enable_sftp=True) as remote:
+        yield remote
+
+
 @fixture
 def remote():
     """

sites/docs/getting-started.rst

@@ -239,7 +239,7 @@ For example, say you had an archive file you wanted to upload:
 
 .. testsetup:: transfers
 
-    mock = MockSFTP()
+    mock = MockRemote(enable_sftp=True)
 
 .. testcleanup:: transfers
 

sites/www/changelog.rst

@@ -13,6 +13,22 @@ Changelog
     names in this paragraph to visit their changelogs and see what you might get
     if you upgrade your dependencies.
 
+- :feature:`-` Enhanced `fabric.testing` in ways large and small:
+
+    - Backwards-compatibly merged the functionality of
+      `~fabric.testing.base.MockSFTP` into `~fabric.testing.base.MockRemote`
+      (may be opted-into by instantiating the latter with ``enable_sftp=True``)
+      so you can mock out both SSH *and* SFTP functionality in the same test,
+      which was previously impossible. It also means you can use this in a
+      Pytest autouse fixture to prevent any tests from accidentally hitting the
+      network!
+    - A new pytest fixture, `~fabric.testing.fixtures.remote_with_sftp`, has
+      been added which leverages the previous bullet point (an all-in-one
+      fixture suitable for, eg, preventing any incidental ssh/sftp attempts
+      during test execution).
+    - A pile of documentation and test enhancements (yes, testing our testing
+      helpers is a thing).
+
 - :support:`-` Language update: applied `s/sanity/safety/g` to the codebase
   (with the few actual API members using the term now marked deprecated & new
   ones added in the meantime, mostly in `fabric.testing`).

tests/testing.py

@@ -0,0 +1,79 @@
+"""
+Tests testing the publicly exposed test helper API.
+
+Most of the testing module is tested via use in our own test suite, but
+considering it's publicly-exposed code, we should have some dedicated tests to
+it!
+
+And thank goodness we're not using vanilla unittest/pytest or the word "test"
+would be in here more than you can shake a buffalo at (see:
+https://en.wikipedia.org/wiki/Buffalo_buffalo_Buffalo_buffalo_buffalo_buffalo_Buffalo_buffalo)
+"""
+
+from unittest.mock import Mock
+
+from fabric import Connection
+from fabric.testing.base import MockRemote
+from pytest import raises
+
+
+class MockRemote_:
+    class contextmanager_behavior:
+        def calls_safety_and_stop_on_exit_with_try_finally(self):
+            mr = MockRemote()
+            # Stop now, before we overwrite, lest we leak the automatic mocking
+            # from init time.
+            mr.stop()
+            mr.stop = Mock()
+            mr.safety = Mock(side_effect=Exception("onoz"))
+            with raises(Exception, match="onoz"):
+                with mr:
+                    pass
+            # assert exit behavior
+            mr.safety.assert_called_once_with()
+            mr.stop.assert_called_once_with()
+
+    class enable_sftp:
+        def does_not_break_ssh_mocking(self):
+            with MockRemote(enable_sftp=True) as mr:
+                mr.expect(cmd="whoami")
+                cxn = Connection(host="whatevs")
+                cxn.run("whoami", in_stream=False)
+                # Safety: can call sftp stuff w/o expect()ing it, should noop
+                # instead of exploding
+                cxn.put("whatevs")
+
+        def enables_per_session_sftp_mocks(self):
+            with MockRemote(enable_sftp=True) as mr:
+                mr.expect(
+                    cmd="rm file",
+                    transfers=[
+                        dict(
+                            method="put",
+                            localpath="/local/whatevs",
+                            remotepath="/remote/whatevs",
+                        )
+                    ],
+                )
+                cxn = Connection(host="host")
+                cxn.run("rm file", in_stream=False)
+                cxn.put("whatevs")
+
+        def safety_checks_work(self):
+            with raises(AssertionError, match=r"put(.*whatevs)"):
+                with MockRemote(enable_sftp=True) as mr:
+                    mr.expect(
+                        transfers=[
+                            dict(
+                                method="put",
+                                localpath="/local/whatevs",
+                                remotepath="/remote/whatevs",
+                            )
+                        ],
+                    )
+                    cxn = Connection(host="host")
+                    # Satisfy the less rigorous default expectations for
+                    # commands
+                    cxn.run("rm file", in_stream=False)
+                    # Oh no! The wrong put()!
+                    cxn.put("onoz")