From 2466819ed1744501bcc0a9008c72ced1aa0aed6c Mon Sep 17 00:00:00 2001 From: Aleksei Stepanov Date: Thu, 29 Oct 2026 18:15:33 +0100 Subject: [PATCH 1/3] Port the skipped SFTP tests to pytest mkdir, rm_rf, upload and download were tested only by skipped tests whose mocks no longer matched the code (os.path and os.walk instead of pathlib). - Rewrite them as pytest functions on the shared conftest fixtures. - Use a temporary local tree instead of mocking the local filesystem. - Cover upload into a remote directory and download of a missing path and of a directory. --- test/test_sftp.py | 250 ++++++++++++++++++++-------------------------- 1 file changed, 107 insertions(+), 143 deletions(-) diff --git a/test/test_sftp.py b/test/test_sftp.py index 9b5fa51..eae9b79 100644 --- a/test/test_sftp.py +++ b/test/test_sftp.py @@ -18,13 +18,13 @@ from __future__ import annotations -import os -import posixpath +import pathlib import stat import unittest from unittest import mock import paramiko +import pytest import exec_helpers @@ -194,54 +194,6 @@ def __init__(self, mode): self.assertFalse(result) lstat.assert_called_once_with(dst) - @unittest.skip("Need to port to pytest: too huge chain of mocks and setup") - @mock.patch("exec_helpers.ssh.SSHClient.exists") - @mock.patch("exec_helpers.ssh.SSHClient.execute") - def test_mkdir(self, execute, exists, *args): - exists.side_effect = [False, True] - - dst = "~/tst dir" - escaped_dst = r"~/tst\ dir" - - # noinspection PyTypeChecker - ssh = exec_helpers.SSHClient( - host=host, - port=port, - auth=exec_helpers.SSHAuth(username=username, password=password), - ) - - # Path not exists - # noinspection PyTypeChecker - ssh.mkdir(dst) - exists.assert_called_once_with(dst) - execute.assert_called_once_with(f"mkdir -p {escaped_dst}\n") - - # Path exists - exists.reset_mock() - execute.reset_mock() - - # noinspection PyTypeChecker - ssh.mkdir(dst) - exists.assert_called_once_with(dst) - execute.assert_not_called() - - @unittest.skip("Need to port to pytest: too huge chain of mocks and setup") - @mock.patch("exec_helpers.ssh.SSHClient.execute") - def test_rm_rf(self, execute, *args): - dst = "~/tst" - - # noinspection PyTypeChecker - ssh = exec_helpers.SSHClient( - host=host, - port=port, - auth=exec_helpers.SSHAuth(username=username, password=password), - ) - - # Path not exists - # noinspection PyTypeChecker - ssh.rm_rf(dst) - execute.assert_called_once_with(f"rm -rf {dst}") - def test_open(self, client, *args): ssh, sftp = self.prepare_sftp_file_tests(client) fopen = mock.Mock(return_value=True) @@ -254,97 +206,109 @@ def test_open(self, client, *args): fopen.assert_called_once_with(dst, mode) self.assertTrue(result) - @unittest.skip("Need to port to pytest: too huge chain of mocks and setup") - @mock.patch("exec_helpers.ssh.SSHClient.exists") - @mock.patch("os.path.exists", autospec=True) - @mock.patch("exec_helpers.ssh.SSHClient.isdir") - @mock.patch("os.path.isdir", autospec=True) - def test_download( - self, - isdir, - remote_isdir, - exists, - remote_exists, - client, - policy, - _logger, - ): - ssh, sftp = self.prepare_sftp_file_tests(client) - isdir.return_value = True - exists.side_effect = [True, False, False] - remote_isdir.side_effect = [False, False, True] - remote_exists.side_effect = [True, False, False] - dst = "/etc/environment" - target = "/tmp/environment" - # noinspection PyTypeChecker - result = ssh.download(destination=dst, target=target) - self.assertTrue(result) - isdir.assert_called_once_with(target) - exists.assert_called_once_with(posixpath.join(target, os.path.basename(dst))) - remote_isdir.assert_called_once_with(dst) - remote_exists.assert_called_once_with(dst) - sftp.assert_has_calls( - mock.call.get( - dst, - posixpath.join(target, os.path.basename(dst)), - ) - ) - - # Negative scenarios - # noinspection PyTypeChecker - result = ssh.download(destination=dst, target=target) - self.assertFalse(result) - - # noinspection PyTypeChecker - ssh.download(destination=dst, target=target) - - @unittest.skip("Need to port to pytest: too huge chain of mocks and setup") - @mock.patch("exec_helpers.ssh.SSHClient.isdir") - @mock.patch("os.path.isdir", autospec=True) - def test_upload_file(self, isdir, remote_isdir, client, *args): - ssh, sftp = self.prepare_sftp_file_tests(client) - isdir.return_value = False - remote_isdir.return_value = False - target = "/etc/environment" - source = "/tmp/environment" - - # noinspection PyTypeChecker - ssh.upload(source=source, target=target) - isdir.assert_called_once_with(source) - remote_isdir.assert_called_once_with(target) - sftp.assert_has_calls((mock.call.put(source, target),)) - - @unittest.skip("Need to port to pytest: too huge chain of mocks and setup") - @mock.patch("exec_helpers.ssh.SSHClient.exists") - @mock.patch("exec_helpers.ssh.SSHClient.mkdir") - @mock.patch("os.walk") - @mock.patch("exec_helpers.ssh.SSHClient.isdir") - @mock.patch("os.path.isdir", autospec=True) - def test_upload_dir(self, isdir, remote_isdir, walk, mkdir, exists, client, *args): - ssh, sftp = self.prepare_sftp_file_tests(client) - isdir.return_value = True - remote_isdir.return_value = True - exists.return_value = True - target = "/etc" - source = "/tmp/bash" - filename = "bashrc" - walk.return_value = ((source, "", [filename]),) - expected_path = posixpath.join(target, os.path.basename(source)) - expected_file = posixpath.join(expected_path, filename) - - # noinspection PyTypeChecker - ssh.upload(source=source, target=target) - isdir.assert_called_once_with(source) - remote_isdir.assert_called_once_with(target) - mkdir.assert_called_once_with(expected_path) - exists.assert_called_once_with(expected_file) - sftp.assert_has_calls( - ( - mock.call.unlink(expected_file), - mock.call.put( - os.path.normpath(os.path.join(source, filename)), - expected_file, - ), - ) - ) +@pytest.fixture +def sftp_client(paramiko_ssh_client, ssh_auth_logger, get_logger) -> tuple[exec_helpers.SSHClient, mock.Mock]: + """Create the client and the SFTP session mock it opens.""" + sftp = mock.Mock() + transport = mock.Mock(open_sftp_client=mock.Mock(return_value=sftp)) + paramiko_ssh_client.return_value = mock.Mock(get_transport=mock.Mock(return_value=transport)) + ssh = exec_helpers.SSHClient( + host=host, + port=port, + auth=exec_helpers.SSHAuth(username=username, password=password), + ) + return ssh, sftp + + +def test_mkdir(mocker, sftp_client) -> None: + """The directory is created by a command with the escaped path, only when it is missing.""" + ssh, _ = sftp_client + exists = mocker.patch.object(exec_helpers.SSHClient, "exists", side_effect=[False, True]) + execute = mocker.patch.object(exec_helpers.SSHClient, "execute") + + ssh.mkdir("~/tst dir") + exists.assert_called_once_with("~/tst dir") + execute.assert_called_once_with("mkdir -p ~/tst\\ dir\n") + + exists.reset_mock() + execute.reset_mock() + ssh.mkdir("~/tst dir") + exists.assert_called_once_with("~/tst dir") + execute.assert_not_called() + + +def test_rm_rf(mocker, sftp_client) -> None: + """The path is removed by a command with the escaped path.""" + ssh, _ = sftp_client + execute = mocker.patch.object(exec_helpers.SSHClient, "execute") + + ssh.rm_rf("~/tst dir") + execute.assert_called_once_with("rm -rf ~/tst\\ dir") + + +@pytest.mark.parametrize( + ("remote_isdir", "expected_target"), + [ + pytest.param(False, "/etc/environment", id="to_file"), + pytest.param(True, "/etc/environment/environment", id="into_dir"), + ], +) +def test_upload_file(mocker, tmp_path, sftp_client, remote_isdir, expected_target) -> None: + """A file is put to the target, or into it when the target is a directory.""" + ssh, sftp = sftp_client + source = tmp_path / "environment" + source.write_text("A=1\n") + isdir = mocker.patch.object(exec_helpers.SSHClient, "isdir", return_value=remote_isdir) + + ssh.upload(source=source, target="/etc/environment") + isdir.assert_called_once_with("/etc/environment") + sftp.put.assert_called_once_with(source.resolve().as_posix(), expected_target, confirm=True) + + +def test_upload_dir(mocker, tmp_path, sftp_client) -> None: + """A directory is copied recursively: subdirectories are created and existing remote files are replaced.""" + ssh, sftp = sftp_client + source = tmp_path / "bash" + (source / "completions").mkdir(parents=True) + (source / "bashrc").write_text("") + (source / "completions" / "git").write_text("") + mocker.patch.object(exec_helpers.SSHClient, "isdir", return_value=True) + mkdir = mocker.patch.object(exec_helpers.SSHClient, "mkdir") + remote_files = {"/etc/bash/bashrc"} + mocker.patch.object(exec_helpers.SSHClient, "exists", side_effect=remote_files.__contains__) + + ssh.upload(source=source, target="/etc") + mkdir.assert_any_call("/etc/bash/completions") + sftp.unlink.assert_called_once_with("/etc/bash/bashrc") + sftp.put.assert_has_calls( + [ + mock.call((source / "bashrc").resolve().as_posix(), "/etc/bash/bashrc", confirm=True), + mock.call((source / "completions" / "git").resolve().as_posix(), "/etc/bash/completions/git", confirm=True), + ], + any_order=True, + ) + assert sftp.put.call_count == 2 + + +@pytest.mark.parametrize( + ("remote_isdir", "remote_exists", "downloaded"), + [ + pytest.param(False, True, True, id="file"), + pytest.param(False, False, False, id="missing"), + pytest.param(True, True, False, id="directory"), + ], +) +def test_download(mocker, tmp_path, sftp_client, remote_isdir, remote_exists, downloaded) -> None: + """A remote file is got into a local directory; a missing path or a directory is not downloaded.""" + ssh, sftp = sftp_client + mocker.patch.object(exec_helpers.SSHClient, "isdir", return_value=remote_isdir) + mocker.patch.object(exec_helpers.SSHClient, "exists", return_value=remote_exists) + target = tmp_path.resolve() / "environment" + sftp.get.side_effect = lambda _remote, local: pathlib.Path(local).write_text("A=1\n") + + assert ssh.download(destination="/etc/environment", target=tmp_path) is downloaded + if downloaded: + sftp.get.assert_called_once_with("/etc/environment", target.as_posix()) + else: + sftp.get.assert_not_called() From 81195e40f40896990a924911344ebaad23b5c91d Mon Sep 17 00:00:00 2001 From: Aleksei Stepanov Date: Thu, 29 Oct 2026 18:16:21 +0100 Subject: [PATCH 2/3] Raise a failed reconnect from the SFTP path checks Since 8.2.1 a closed client reconnects before opening SFTP, but exists, isfile, isdir and islink did that inside the try that maps OSError to False. A reconnect that failed with a socket error (connection refused, host unreachable) was therefore reported as a missing path, and a caller skipped a cleanup or an upload without any error. - Open the session before the try, so only an error from lstat becomes False. - Document the raised exceptions in the docstrings and in SSHClient.rst. --- doc/source/SSHClient.rst | 20 +++++++++++++++++ exec_helpers/_ssh_base.py | 32 ++++++++++++++++++++++++---- test/test_ssh_client_init_special.py | 21 ++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/doc/source/SSHClient.rst b/doc/source/SSHClient.rst index 1d291a8..30a0aee 100644 --- a/doc/source/SSHClient.rst +++ b/doc/source/SSHClient.rst @@ -447,6 +447,11 @@ API: SSHClient and SSHAuth. :type path: ``str`` :return: path is valid (object exists) :rtype: ``bool`` + :raises ConnectionError: SSH session was closed and the reconnect failed + :raises OSError: SSH session was closed and the reconnect failed with a socket error + :raises paramiko.SSHException: SFTP connection failed, or the reconnect failed with a paramiko error + + .. versionchanged:: 8.2.2 a failed reconnect is raised instead of being reported as a missing path .. py:method:: stat(path) @@ -477,6 +482,11 @@ API: SSHClient and SSHAuth. :type path: ``str`` :return: path is file :rtype: ``bool`` + :raises ConnectionError: SSH session was closed and the reconnect failed + :raises OSError: SSH session was closed and the reconnect failed with a socket error + :raises paramiko.SSHException: SFTP connection failed, or the reconnect failed with a paramiko error + + .. versionchanged:: 8.2.2 a failed reconnect is raised instead of being reported as a missing path .. py:method:: isdir(path) @@ -486,6 +496,11 @@ API: SSHClient and SSHAuth. :type path: ``str`` :return: path is directory :rtype: ``bool`` + :raises ConnectionError: SSH session was closed and the reconnect failed + :raises OSError: SSH session was closed and the reconnect failed with a socket error + :raises paramiko.SSHException: SFTP connection failed, or the reconnect failed with a paramiko error + + .. versionchanged:: 8.2.2 a failed reconnect is raised instead of being reported as a missing path .. py:method:: islink(path) @@ -495,6 +510,11 @@ API: SSHClient and SSHAuth. :type path: ``str`` :return: path is symlink :rtype: ``bool`` + :raises ConnectionError: SSH session was closed and the reconnect failed + :raises OSError: SSH session was closed and the reconnect failed with a socket error + :raises paramiko.SSHException: SFTP connection failed, or the reconnect failed with a paramiko error + + .. versionchanged:: 8.2.2 a failed reconnect is raised instead of being reported as a missing path .. py:method:: symlink(source, dest) diff --git a/exec_helpers/_ssh_base.py b/exec_helpers/_ssh_base.py index 67adbc0..90fe14b 100644 --- a/exec_helpers/_ssh_base.py +++ b/exec_helpers/_ssh_base.py @@ -1905,9 +1905,15 @@ def exists(self, path: SupportPathT) -> bool: :type path: str | pathlib.PurePath :return: path is valid (an object exists). :rtype: bool + :raises ConnectionError: SSH session was closed and the reconnect failed. + :raises OSError: SSH session was closed and the reconnect failed with a socket error. + :raises paramiko.SSHException: SFTP connection failed, or the reconnect failed with a paramiko error. + + .. versionchanged:: 8.2.2 A failed reconnect is raised instead of being reported as a missing path. """ + sftp = self._sftp # a failed reconnect is raised, not reported as a missing path try: - self._sftp.lstat(pathlib.PurePath(path).as_posix()) + sftp.lstat(pathlib.PurePath(path).as_posix()) except OSError: return False @@ -1942,9 +1948,15 @@ def isfile(self, path: SupportPathT) -> bool: :type path: str | pathlib.PurePath :return: path is file. :rtype: bool + :raises ConnectionError: SSH session was closed and the reconnect failed. + :raises OSError: SSH session was closed and the reconnect failed with a socket error. + :raises paramiko.SSHException: SFTP connection failed, or the reconnect failed with a paramiko error. + + .. versionchanged:: 8.2.2 A failed reconnect is raised instead of being reported as a missing path. """ + sftp = self._sftp # a failed reconnect is raised, not reported as a missing path try: - attrs: paramiko.sftp_attr.SFTPAttributes = self._sftp.lstat(pathlib.PurePath(path).as_posix()) + attrs: paramiko.sftp_attr.SFTPAttributes = sftp.lstat(pathlib.PurePath(path).as_posix()) if attrs.st_mode is None: return False return stat.S_ISREG(attrs.st_mode) @@ -1958,9 +1970,15 @@ def isdir(self, path: SupportPathT) -> bool: :type path: str | pathlib.PurePath :return: path is directory. :rtype: bool + :raises ConnectionError: SSH session was closed and the reconnect failed. + :raises OSError: SSH session was closed and the reconnect failed with a socket error. + :raises paramiko.SSHException: SFTP connection failed, or the reconnect failed with a paramiko error. + + .. versionchanged:: 8.2.2 A failed reconnect is raised instead of being reported as a missing path. """ + sftp = self._sftp # a failed reconnect is raised, not reported as a missing path try: - attrs: paramiko.sftp_attr.SFTPAttributes = self._sftp.lstat(pathlib.PurePath(path).as_posix()) + attrs: paramiko.sftp_attr.SFTPAttributes = sftp.lstat(pathlib.PurePath(path).as_posix()) if attrs.st_mode is None: return False return stat.S_ISDIR(attrs.st_mode) @@ -1974,9 +1992,15 @@ def islink(self, path: SupportPathT) -> bool: :type path: str | pathlib.PurePath :return: path is symlink. :rtype: bool + :raises ConnectionError: SSH session was closed and the reconnect failed. + :raises OSError: SSH session was closed and the reconnect failed with a socket error. + :raises paramiko.SSHException: SFTP connection failed, or the reconnect failed with a paramiko error. + + .. versionchanged:: 8.2.2 A failed reconnect is raised instead of being reported as a missing path. """ + sftp = self._sftp # a failed reconnect is raised, not reported as a missing path try: - attrs: paramiko.sftp_attr.SFTPAttributes = self._sftp.lstat(pathlib.PurePath(path).as_posix()) + attrs: paramiko.sftp_attr.SFTPAttributes = sftp.lstat(pathlib.PurePath(path).as_posix()) if attrs.st_mode is None: return False return stat.S_ISLNK(attrs.st_mode) diff --git a/test/test_ssh_client_init_special.py b/test/test_ssh_client_init_special.py index 0bed7d1..4e0376b 100644 --- a/test/test_ssh_client_init_special.py +++ b/test/test_ssh_client_init_special.py @@ -574,3 +574,24 @@ def test_016_sftp_after_close_reconnect_failed( ssh._sftp # pylint: disable=pointless-statement # noqa: B018 fake.transport.open_sftp_client.assert_not_called() + + +@pytest.mark.parametrize("check", ["exists", "isfile", "isdir", "islink"]) +def test_017_sftp_path_check_after_close_reconnect_failed( + check: str, + paramiko_ssh_client, + ssh_auth_logger, + get_logger, +): + """A failed reconnect is raised by a path check, not reported as a missing path.""" + fake = ClosableSSHClient() + paramiko_ssh_client.return_value = fake + + ssh = exec_helpers.SSHClient(host=host, auth=exec_helpers.SSHAuth(password=password)) + ssh.close() + fake.connect.side_effect = ConnectionRefusedError("connect failed") + + with pytest.raises(ConnectionRefusedError, match="connect failed"): + getattr(ssh, check)("/path") + + fake.sftp.lstat.assert_not_called() From 99e1bc9962a702093a2d93a2f3e7c78f018f8dcf Mon Sep 17 00:00:00 2001 From: Aleksei Stepanov Date: Thu, 29 Oct 2026 18:18:09 +0100 Subject: [PATCH 3/3] Create the remote directory when uploading a directory upload() copied the contents of a local directory but never created the remote directory they go into. A target that did not exist yet was created only as a side effect of mkdir -p for a subdirectory, so the upload failed when a file came first, and an empty directory produced nothing. - Create the target directory before copying what is inside the source. - Test upload into an existing remote directory and to a new path. --- doc/source/SSHClient.rst | 2 ++ exec_helpers/ssh.py | 3 +++ test/test_sftp.py | 18 +++++++++++++----- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/doc/source/SSHClient.rst b/doc/source/SSHClient.rst index 30a0aee..0020c5a 100644 --- a/doc/source/SSHClient.rst +++ b/doc/source/SSHClient.rst @@ -555,6 +555,8 @@ API: SSHClient and SSHAuth. :type source: ``str`` :type target: ``str`` + .. versionchanged:: 8.2.2 uploading a directory creates the remote directory it is copied to + .. py:method:: download(destination, target) Download file(s) to target from destination. diff --git a/exec_helpers/ssh.py b/exec_helpers/ssh.py index f720816..9107af2 100644 --- a/exec_helpers/ssh.py +++ b/exec_helpers/ssh.py @@ -73,6 +73,8 @@ def upload(self, source: SupportPathT, target: SupportPathT) -> None: :type source: str | pathlib.PurePath :param target: Remote path. :type target: str | pathlib.PurePath + + .. versionchanged:: 8.2.2 Uploading a directory creates the remote directory it is copied to. """ self.logger.debug(f"Copying '{source}' -> '{target}'") @@ -87,6 +89,7 @@ def upload(self, source: SupportPathT, target: SupportPathT) -> None: self._sftp.put(src.as_posix(), tgt.as_posix(), confirm=True) return + self.mkdir(tgt.as_posix()) # glob below yields only what is inside src, never src itself for pth in src.glob("**/*"): relative = pth.relative_to(src).as_posix() destination: str = os.path.normpath(tgt.joinpath(relative).as_posix()).replace("\\", "/") diff --git a/test/test_sftp.py b/test/test_sftp.py index eae9b79..01f79dc 100644 --- a/test/test_sftp.py +++ b/test/test_sftp.py @@ -266,20 +266,28 @@ def test_upload_file(mocker, tmp_path, sftp_client, remote_isdir, expected_targe sftp.put.assert_called_once_with(source.resolve().as_posix(), expected_target, confirm=True) -def test_upload_dir(mocker, tmp_path, sftp_client) -> None: - """A directory is copied recursively: subdirectories are created and existing remote files are replaced.""" +@pytest.mark.parametrize( + ("target", "remote_isdir"), + [ + pytest.param("/etc", True, id="into_dir"), + pytest.param("/etc/bash", False, id="as_target"), + ], +) +def test_upload_dir(mocker, tmp_path, sftp_client, target, remote_isdir) -> None: + """A directory is copied recursively: its directories are created and existing remote files are replaced.""" ssh, sftp = sftp_client source = tmp_path / "bash" (source / "completions").mkdir(parents=True) (source / "bashrc").write_text("") (source / "completions" / "git").write_text("") - mocker.patch.object(exec_helpers.SSHClient, "isdir", return_value=True) + mocker.patch.object(exec_helpers.SSHClient, "isdir", return_value=remote_isdir) mkdir = mocker.patch.object(exec_helpers.SSHClient, "mkdir") remote_files = {"/etc/bash/bashrc"} mocker.patch.object(exec_helpers.SSHClient, "exists", side_effect=remote_files.__contains__) - ssh.upload(source=source, target="/etc") - mkdir.assert_any_call("/etc/bash/completions") + ssh.upload(source=source, target=target) + # The root first: the files and directories below it are created inside it. + assert mkdir.call_args_list == [mock.call("/etc/bash"), mock.call("/etc/bash/completions")] sftp.unlink.assert_called_once_with("/etc/bash/bashrc") sftp.put.assert_has_calls( [