From a418d0d8c34f14b6554259ab350540cece3ee405 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Sat, 5 Aug 2023 21:23:09 -0400 Subject: [PATCH] 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 | 130 ++++++++++++++++++++++++++++----- fabric/testing/fixtures.py | 17 +++++ sites/docs/getting-started.rst | 2 +- sites/www/changelog.rst | 16 ++++ tests/testing.py | 79 ++++++++++++++++++++ 5 files changed, 226 insertions(+), 18 deletions(-) create mode 100644 tests/testing.py diff --git a/fabric/testing/base.py b/fabric/testing/base.py index 0cf2566bf..2ecfdad2c 100644 --- a/fabric/testing/base.py +++ b/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 `). + 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. diff --git a/fabric/testing/fixtures.py b/fabric/testing/fixtures.py index 9fd57c734..29c1b2435 100644 --- a/fabric/testing/fixtures.py +++ b/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(): """ diff --git a/sites/docs/getting-started.rst b/sites/docs/getting-started.rst index 1eef6bf3f..51ba58958 100644 --- a/sites/docs/getting-started.rst +++ b/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 diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 3ee34b73b..b74281ea0 100644 --- a/sites/www/changelog.rst +++ b/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`). diff --git a/tests/testing.py b/tests/testing.py new file mode 100644 index 000000000..ec0214c36 --- /dev/null +++ b/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")