review applied

src/aiida/transports/plugins/local.py

@@ -104,7 +104,7 @@ def chdir(self, path: TransportPath):
         :raise OSError: if the directory does not have read attributes.
         """
         warn_deprecation(
-            '`chdir()` is deprecated and will be removed in the next major version.',
+            '`chdir()` is deprecated and will be removed in the next major version. Use absolute paths instead.',
             version=3,
         )
         path = str(path)

src/aiida/transports/plugins/ssh.py

@@ -231,10 +231,6 @@ class SshTransport(BlockingTransport):
     # if too large commands are sent, clogging the outputs or logs
     _MAX_EXEC_COMMAND_LOG_SIZE = None
 
-    # NOTE: all the methods that start with _get_ are class methods that
-    # return a suggestion for the specific field. They are being used in
-    # a function called transport_option_default in transports/cli.py,
-    # during an interactive `verdi computer configure` command.
     @classmethod
     def _get_username_suggestion_string(cls, computer):
         """Return a suggestion for the specific field."""
@@ -596,7 +592,7 @@ def chdir(self, path: TransportPath):
         happens and the cwd is unchanged.
         """
         warn_deprecation(
-            '`chdir()` is deprecated and will be removed in the next major version.',
+            '`chdir()` is deprecated and will be removed in the next major version. Use absolute paths instead.',
             version=3,
         )
         from paramiko.sftp import SFTPError
@@ -676,7 +672,7 @@ def getcwd(self):
         so this should never happen within this class.
         """
         warn_deprecation(
-            '`chdir()` is deprecated and will be removed in the next major version.',
+            '`chdir()` is deprecated and will be removed in the next major version. Use absolute paths instead.',
             version=3,
         )
         return self.sftp.getcwd()
@@ -1378,6 +1374,7 @@ def rename(self, oldpath: TransportPath, newpath: TransportPath):
         # TODO: this seems to be a bug (?)
         # why to raise an OSError if the newpath does not exist?
         # ofcourse newpath shouldn't exist, that's why we are renaming it!
+        # issue opened here: https://github.com/aiidateam/aiida-core/issues/6725
         if not self.isfile(newpath):
             if not self.isdir(newpath):
                 raise OSError(f'Destination {newpath} does not exist')

src/aiida/transports/plugins/ssh_async.py

@@ -9,6 +9,7 @@
 """Plugin for transport over SSH asynchronously."""
 
 ## TODO: put & get methods could be simplified with the asyncssh.sftp.mget() & put() method or sftp.glob()
+## https://github.com/aiidateam/aiida-core/issues/6719
 import asyncio
 import glob
 import os
@@ -113,7 +114,7 @@ def _get_machine_suggestion_string(cls, computer):
         # Originally set as 'Hostname' during `verdi computer setup`
         # and is passed as `machine=computer.hostname` in the codebase
         # unfortunately, name of hostname and machine are used interchangeably in the aiida-core codebase
-        # TODO: open an issue to unify the naming
+        # TODO: an issue is open: https://github.com/aiidateam/aiida-core/issues/6726
         return computer.hostname
 
     def __init__(self, *args, **kwargs):
@@ -175,9 +176,6 @@ async def close_async(self):
         await self._conn.wait_closed()
         self._is_open = False
 
-    def chown(self, path, uid, gid):
-        raise NotImplementedError
-
     def __str__(self):
         return f"{'OPEN' if self._is_open else 'CLOSED'} [AsyncSshTransport]"
 
@@ -222,8 +220,6 @@ async def get_async(
         if not os.path.isabs(localpath):
             raise ValueError('The localpath must be an absolute path')
 
-        ## TODO: this whole glob part can be simplified via the asyncssh glob
-        ## or by using the asyncssh.sftp.mget() method
         if self.has_magic(remotepath):
             if self.has_magic(localpath):
                 raise ValueError('Pathname patterns are not allowed in the destination')
@@ -429,7 +425,6 @@ async def put_async(
         if not os.path.isabs(localpath):
             raise ValueError('The localpath must be an absolute path')
 
-        # TODO: this whole glob part can be simplified via the asyncssh glob
         if self.has_magic(localpath):
             if self.has_magic(remotepath):
                 raise ValueError('Pathname patterns are not allowed in the destination')
@@ -1027,6 +1022,7 @@ async def remove_async(self, path: TransportPath):
         path = str(path)
         # TODO: check if asyncssh does return SFTPFileIsADirectory in this case
         # if that's the case, we can get rid of the isfile check
+        # https://github.com/aiidateam/aiida-core/issues/6719
         if await self.isdir_async(path):
             raise OSError(f'The path {path} is a directory')
         else:

src/aiida/transports/transport.py

@@ -44,8 +44,16 @@ def validate_positive_number(ctx, param, value):
 
 
 class Transport(abc.ABC):
-    """Abstract class for a generic blocking transport.
-    A plugin inhereting from this class should implement the blocking methods, only."""
+    """Abstract class for a generic transport.
+    A plugin inheriting from this class should implement all the abstract methods.
+    In case your plugin is strictly asynchronous (blocking), you may want to inherit
+      from `AsyncTransport` (BlockingTransport) for easier adoption.
+
+    .. note:: All the methods that start with ``_get_`` are class methods that
+        return a suggestion for the specific field. They are being used in
+        a function called ``transport_option_default`` in ``transports/cli.py``,
+        during an interactive ``verdi computer configure`` command.
+    """
 
     # This will be used for ``Computer.get_minimum_job_poll_interval``
     DEFAULT_MINIMUM_JOB_POLL_INTERVAL = 10
@@ -1583,6 +1591,9 @@ def puttree(self, *args, **kwargs):
     def chmod(self, *args, **kwargs):
         return self.run_command_blocking(self.chmod_async, *args, **kwargs)
 
+    def chown(self, path, uid, gid):
+        return self.run_command_blocking(self.chown_async, path, uid, gid)
+
     def copy(self, *args, **kwargs):
         return self.run_command_blocking(self.copy_async, *args, **kwargs)
 

tests/cmdline/commands/test_computer.py

@@ -974,38 +974,22 @@ def time_use_login_shell(authinfo, auth_params, use_login_shell, iterations) ->
     assert 'computer is configured to use a login shell, which is slower compared to a normal shell' in result.output
 
 
-def test_computer_ssh_auto(run_cli_command, aiida_computer):
-    """Test setup of computer with ``core.ssh_auto`` entry point.
-
-    The configure step should only require the common shared options ``safe_interval`` and ``use_login_shell``.
-    """
-    computer = aiida_computer(transport_type='core.ssh_auto').store()
-    assert not computer.is_configured
-
-    # It is important that no other options (except for `--safe-interval`) have to be specified for this transport type.
-    options = ['core.ssh_auto', computer.uuid, '--non-interactive', '--safe-interval', '0']
-    run_cli_command(computer_configure, options, use_subprocess=False)
-    assert computer.is_configured
-
-
-def test_computer_ssh_async(run_cli_command, aiida_computer):
-    """Test setup of computer with ``core.ssh_async`` entry point.
+# comment on 'core.ssh_async':
+# It is important that 'ssh localhost' is functional in your test environment.
+# It should connect without asking for a password.
+# comment on 'core.ssh_auto':
+# It is important that no other options (except for `--safe-interval`) have to be specified for this transport type.
+@pytest.mark.parametrize(
+    'transport_type, config', [('core.ssh_auto', []), ('core.ssh_async', ['--machine-or-host', 'localhost'])]
+)
+def test_computer_setup_with_various_transport(run_cli_command, aiida_computer, transport_type, config):
+    """Test setup of computer with ``core.ssh_auto`` and ``core.ssh_async`` entry points.
 
-    The configure step should only require the common shared options ``safe_interval`` and ``use_login_shell``.
+    pass any config option the setup needs in the parameter section``.
     """
-    computer = aiida_computer(transport_type='core.ssh_async').store()
+    computer = aiida_computer(transport_type=transport_type).store()
     assert not computer.is_configured
 
-    # It is important that 'ssh localhost' is functional in your test environment.
-    # It should connect without asking for a password.
-    options = [
-        'core.ssh_async',
-        computer.uuid,
-        '--non-interactive',
-        '--safe-interval',
-        '0',
-        '--machine-or-host',
-        'localhost',
-    ]
+    options = [transport_type, computer.uuid] + config
     run_cli_command(computer_configure, options, use_subprocess=False)
     assert computer.is_configured

tests/manage/tests/test_pytest_fixtures.py

@@ -20,70 +20,33 @@ def test_aiida_localhost(aiida_localhost):
     assert aiida_localhost.label == 'localhost'
 
 
+@pytest.mark.parametrize(
+    'fixture_name, transport_cls, transport_type',
+    [
+        ('aiida_computer_ssh', BlockingTransport, 'core.ssh'),
+        ('aiida_computer_local', BlockingTransport, 'core.local'),
+        ('aiida_computer_ssh_async', AsyncTransport, 'core.ssh_async'),
+    ],
+)
 @pytest.mark.usefixtures('aiida_profile_clean')
-def test_aiida_computer_local(aiida_computer_local):
-    """Test the ``aiida_computer_local`` fixture."""
-    computer = aiida_computer_local()
+def test_aiida_computer_fixtures(fixture_name, transport_cls, transport_type, request):
+    """Test the computer fixtures."""
+    aiida_computer = request.getfixturevalue(fixture_name)
+    computer = aiida_computer()
     assert isinstance(computer, Computer)
     assert computer.is_configured
     assert computer.hostname == 'localhost'
-    assert computer.transport_type == 'core.local'
+    assert computer.transport_type == transport_type
 
     with computer.get_transport() as transport:
-        assert isinstance(transport, BlockingTransport)
+        assert isinstance(transport, transport_cls)
 
     # Calling it again with the same label should simply return the existing computer
-    computer_alt = aiida_computer_local(label=computer.label)
+    computer_alt = aiida_computer(label=computer.label)
     assert computer_alt.uuid == computer.uuid
 
-    computer_new = aiida_computer_local(label=str(uuid.uuid4()))
+    computer_new = aiida_computer(label=str(uuid.uuid4()))
     assert computer_new.uuid != computer.uuid
 
-    computer_unconfigured = aiida_computer_local(label=str(uuid.uuid4()), configure=False)
-    assert not computer_unconfigured.is_configured
-
-
-@pytest.mark.usefixtures('aiida_profile_clean')
-def test_aiida_computer_ssh(aiida_computer_ssh):
-    """Test the ``aiida_computer_ssh`` fixture."""
-    computer = aiida_computer_ssh()
-    assert isinstance(computer, Computer)
-    assert computer.is_configured
-    assert computer.hostname == 'localhost'
-    assert computer.transport_type == 'core.ssh'
-
-    with computer.get_transport() as transport:
-        assert isinstance(transport, BlockingTransport)
-
-    # Calling it again with the same label should simply return the existing computer
-    computer_alt = aiida_computer_ssh(label=computer.label)
-    assert computer_alt.uuid == computer.uuid
-
-    computer_new = aiida_computer_ssh(label=str(uuid.uuid4()))
-    assert computer_new.uuid != computer.uuid
-
-    computer_unconfigured = aiida_computer_ssh(label=str(uuid.uuid4()), configure=False)
-    assert not computer_unconfigured.is_configured
-
-
-@pytest.mark.usefixtures('aiida_profile_clean')
-def test_aiida_computer_ssh_async(aiida_computer_ssh_async):
-    """Test the ``aiida_computer_ssh_async`` fixture."""
-    computer = aiida_computer_ssh_async()
-    assert isinstance(computer, Computer)
-    assert computer.is_configured
-    assert computer.hostname == 'localhost'
-    assert computer.transport_type == 'core.ssh_async'
-
-    with computer.get_transport() as transport:
-        assert isinstance(transport, AsyncTransport)
-
-    # Calling it again with the same label should simply return the existing computer
-    computer_alt = aiida_computer_ssh_async(label=computer.label)
-    assert computer_alt.uuid == computer.uuid
-
-    computer_new = aiida_computer_ssh_async(label=str(uuid.uuid4()))
-    assert computer_new.uuid != computer.uuid
-
-    computer_unconfigured = aiida_computer_ssh_async(label=str(uuid.uuid4()), configure=False)
+    computer_unconfigured = aiida_computer(label=str(uuid.uuid4()), configure=False)
     assert not computer_unconfigured.is_configured