[v7,11/21] dts: remote session docstring update

  Format according to the Google format and PEP257, with slight
deviations.

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
 dts/framework/remote_session/__init__.py      |  39 +++++-
 .../remote_session/remote_session.py          | 128 +++++++++++++-----
 dts/framework/remote_session/ssh_session.py   |  16 +--
 3 files changed, 135 insertions(+), 48 deletions(-)
  

On 11/15/23 13:09, Juraj Linkeš wrote:
> Format according to the Google format and PEP257, with slight
> deviations.
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
>   dts/framework/remote_session/__init__.py      |  39 +++++-
>   .../remote_session/remote_session.py          | 128 +++++++++++++-----
>   dts/framework/remote_session/ssh_session.py   |  16 +--
>   3 files changed, 135 insertions(+), 48 deletions(-)
> 
> diff --git a/dts/framework/remote_session/__init__.py b/dts/framework/remote_session/__init__.py
> index 5e7ddb2b05..51a01d6b5e 100644
> --- a/dts/framework/remote_session/__init__.py
> +++ b/dts/framework/remote_session/__init__.py
> @@ -2,12 +2,14 @@
>   # Copyright(c) 2023 PANTHEON.tech s.r.o.
>   # Copyright(c) 2023 University of New Hampshire
>   
> -"""
> -The package provides modules for managing remote connections to a remote host (node),
> -differentiated by OS.
> -The package provides a factory function, create_session, that returns the appropriate
> -remote connection based on the passed configuration. The differences are in the
> -underlying transport protocol (e.g. SSH) and remote OS (e.g. Linux).
> +"""Remote interactive and non-interactive sessions.
> +
> +This package provides modules for managing remote connections to a remote host (node).
> +
> +The non-interactive sessions send commands and return their output and exit code.
> +
> +The interactive sessions open an interactive shell which is continuously open,
> +allowing it to send and receive data within that particular shell.
>   """
>   
>   # pylama:ignore=W0611
> @@ -26,10 +28,35 @@
>   def create_remote_session(
>       node_config: NodeConfiguration, name: str, logger: DTSLOG
>   ) -> RemoteSession:
> +    """Factory for non-interactive remote sessions.
> +
> +    The function returns an SSH session, but will be extended if support
> +    for other protocols is added.
> +
> +    Args:
> +        node_config: The test run configuration of the node to connect to.
> +        name: The name of the session.
> +        logger: The logger instance this session will use.
> +
> +    Returns:
> +        The SSH remote session.
> +    """
>       return SSHSession(node_config, name, logger)
>   
>   
>   def create_interactive_session(
>       node_config: NodeConfiguration, logger: DTSLOG
>   ) -> InteractiveRemoteSession:
> +    """Factory for interactive remote sessions.
> +
> +    The function returns an interactive SSH session, but will be extended if support
> +    for other protocols is added.
> +
> +    Args:
> +        node_config: The test run configuration of the node to connect to.
> +        logger: The logger instance this session will use.
> +
> +    Returns:
> +        The interactive SSH remote session.
> +    """
>       return InteractiveRemoteSession(node_config, logger)
> diff --git a/dts/framework/remote_session/remote_session.py b/dts/framework/remote_session/remote_session.py
> index 0647d93de4..629c2d7b9c 100644
> --- a/dts/framework/remote_session/remote_session.py
> +++ b/dts/framework/remote_session/remote_session.py
> @@ -3,6 +3,13 @@
>   # Copyright(c) 2022-2023 PANTHEON.tech s.r.o.
>   # Copyright(c) 2022-2023 University of New Hampshire
>   
> +"""Base remote session.
> +
> +This module contains the abstract base class for remote sessions and defines
> +the structure of the result of a command execution.
> +"""
> +
> +
>   import dataclasses
>   from abc import ABC, abstractmethod
>   from pathlib import PurePath
> @@ -15,8 +22,14 @@
>   
>   @dataclasses.dataclass(slots=True, frozen=True)
>   class CommandResult:
> -    """
> -    The result of remote execution of a command.
> +    """The result of remote execution of a command.
> +
> +    Attributes:
> +        name: The name of the session that executed the command.
> +        command: The executed command.
> +        stdout: The standard output the command produced.
> +        stderr: The standard error output the command produced.
> +        return_code: The return code the command exited with.
>       """
>   
>       name: str
> @@ -26,6 +39,7 @@ class CommandResult:
>       return_code: int
>   
>       def __str__(self) -> str:
> +        """Format the command outputs."""
>           return (
>               f"stdout: '{self.stdout}'\n"
>               f"stderr: '{self.stderr}'\n"
> @@ -34,13 +48,24 @@ def __str__(self) -> str:
>   
>   
>   class RemoteSession(ABC):
> -    """
> -    The base class for defining which methods must be implemented in order to connect
> -    to a remote host (node) and maintain a remote session. The derived classes are
> -    supposed to implement/use some underlying transport protocol (e.g. SSH) to
> -    implement the methods. On top of that, it provides some basic services common to
> -    all derived classes, such as keeping history and logging what's being executed
> -    on the remote node.
> +    """Non-interactive remote session.
> +
> +    The abstract methods must be implemented in order to connect to a remote host (node)
> +    and maintain a remote session.
> +    The subclasses must use (or implement) some underlying transport protocol (e.g. SSH)
> +    to implement the methods. On top of that, it provides some basic services common to all
> +    subclasses, such as keeping history and logging what's being executed on the remote node.
> +
> +    Attributes:
> +        name: The name of the session.
> +        hostname: The node's hostname. Could be an IP (possibly with port, separated by a colon)
> +            or a domain name.
> +        ip: The IP address of the node or a domain name, whichever was used in `hostname`.
> +        port: The port of the node, if given in `hostname`.
> +        username: The username used in the connection.
> +        password: The password used in the connection. Most frequently empty,
> +            as the use of passwords is discouraged.
> +        history: The executed commands during this session.
>       """
>   
>       name: str
> @@ -59,6 +84,16 @@ def __init__(
>           session_name: str,
>           logger: DTSLOG,
>       ):
> +        """Connect to the node during initialization.
> +
> +        Args:
> +            node_config: The test run configuration of the node to connect to.
> +            session_name: The name of the session.
> +            logger: The logger instance this session will use.
> +
> +        Raises:
> +            SSHConnectionError: If the connection to the node was not successful.
> +        """
>           self._node_config = node_config
>   
>           self.name = session_name
> @@ -79,8 +114,13 @@ def __init__(
>   
>       @abstractmethod
>       def _connect(self) -> None:
> -        """
> -        Create connection to assigned node.
> +        """Create a connection to the node.
> +
> +        The implementation must assign the established session to self.session.
> +
> +        The implementation must except all exceptions and convert them to an SSHConnectionError.
> +
> +        The implementation may optionally implement retry attempts.
>           """
>   
>       def send_command(
> @@ -90,11 +130,24 @@ def send_command(
>           verify: bool = False,
>           env: dict | None = None,
>       ) -> CommandResult:
> -        """
> -        Send a command to the connected node using optional env vars
> -        and return CommandResult.
> -        If verify is True, check the return code of the executed command
> -        and raise a RemoteCommandExecutionError if the command failed.
> +        """Send `command` to the connected node.
> +
> +        The :option:`--timeout` command line argument and the :envvar:`DTS_TIMEOUT`
> +        environment variable configure the timeout of command execution.
> +
> +        Args:
> +            command: The command to execute.
> +            timeout: Wait at most this long in seconds to execute `command`.
> +            verify: If :data:`True`, will check the exit code of `command`.
> +            env: A dictionary with environment variables to be used with `command` execution.
> +
> +        Raises:
> +            SSHSessionDeadError: If the session isn't alive when sending `command`.
> +            SSHTimeoutError: If `command` execution timed out.
> +            RemoteCommandExecutionError: If verify is :data:`True` and `command` execution failed.
> +
> +        Returns:
> +            The output of the command along with the return code.
>           """
>           self._logger.info(
>               f"Sending: '{command}'" + (f" with env vars: '{env}'" if env else "")
> @@ -115,29 +168,36 @@ def send_command(
>       def _send_command(
>           self, command: str, timeout: float, env: dict | None
>       ) -> CommandResult:
> -        """
> -        Use the underlying protocol to execute the command using optional env vars
> -        and return CommandResult.
> +        """Send a command to the connected node.
> +
> +        The implementation must execute the command remotely with `env` environment variables
> +        and return the result.
> +
> +        The implementation must except all exceptions and raise an SSHSessionDeadError if
> +        the session is not alive and an SSHTimeoutError if the command execution times out.

3 way "and". Needs comas or splitting the sentence.

>           """
>   
>       def close(self, force: bool = False) -> None:
> -        """
> -        Close the remote session and free all used resources.
> +        """Close the remote session and free all used resources.
> +
> +        Args:
> +            force: Force the closure of the connection. This may not clean up all resources.
>           """
>           self._logger.logger_exit()
>           self._close(force)
>   
>       @abstractmethod
>       def _close(self, force: bool = False) -> None:
> -        """
> -        Execute protocol specific steps needed to close the session properly.
> +        """Protocol specific steps needed to close the session properly.
> +
> +        Args:
> +            force: Force the closure of the connection. This may not clean up all resources.
> +                This doesn't have to be implemented in the overloaded method.
>           """
>   
>       @abstractmethod
>       def is_alive(self) -> bool:
> -        """
> -        Check whether the remote session is still responding.
> -        """
> +        """Check whether the remote session is still responding."""
>   
>       @abstractmethod
>       def copy_from(
> @@ -147,12 +207,12 @@ def copy_from(
>       ) -> None:
>           """Copy a file from the remote Node to the local filesystem.
>   
> -        Copy source_file from the remote Node associated with this remote
> -        session to destination_file on the local filesystem.
> +        Copy `source_file` from the remote Node associated with this remote session
> +        to `destination_file` on the local filesystem.
>   
>           Args:
> -            source_file: the file on the remote Node.
> -            destination_file: a file or directory path on the local filesystem.
> +            source_file: The file on the remote Node.
> +            destination_file: A file or directory path on the local filesystem.
>           """
>   
>       @abstractmethod
> @@ -163,10 +223,10 @@ def copy_to(
>       ) -> None:
>           """Copy a file from local filesystem to the remote Node.
>   
> -        Copy source_file from local filesystem to destination_file
> -        on the remote Node associated with this remote session.
> +        Copy `source_file` from local filesystem to `destination_file` on the remote Node
> +        associated with this remote session.
>   
>           Args:
> -            source_file: the file on the local filesystem.
> -            destination_file: a file or directory path on the remote Node.
> +            source_file: The file on the local filesystem.
> +            destination_file: A file or directory path on the remote Node.
>           """
> diff --git a/dts/framework/remote_session/ssh_session.py b/dts/framework/remote_session/ssh_session.py
> index cee11d14d6..7186490a9a 100644
> --- a/dts/framework/remote_session/ssh_session.py
> +++ b/dts/framework/remote_session/ssh_session.py
> @@ -1,6 +1,8 @@
>   # SPDX-License-Identifier: BSD-3-Clause
>   # Copyright(c) 2023 PANTHEON.tech s.r.o.
>   
> +"""SSH session remote session."""

Is the double "session" intended?

> +
>   import socket
>   import traceback
>   from pathlib import PurePath
> @@ -26,13 +28,8 @@
>   class SSHSession(RemoteSession):
>       """A persistent SSH connection to a remote Node.
>   
> -    The connection is implemented with the Fabric Python library.
> -
> -    Args:
> -        node_config: The configuration of the Node to connect to.
> -        session_name: The name of the session.
> -        logger: The logger used for logging.
> -            This should be passed from the parent OSSession.
> +    The connection is implemented with
> +    `the Fabric Python library <https://docs.fabfile.org/en/latest/>`_.
>   
>       Attributes:
>           session: The underlying Fabric SSH connection.
> @@ -80,6 +77,7 @@ def _connect(self) -> None:
>               raise SSHConnectionError(self.hostname, errors)
>   
>       def is_alive(self) -> bool:
> +        """Overrides :meth:`~.remote_session.RemoteSession.is_alive`."""
>           return self.session.is_connected
>   
>       def _send_command(
> @@ -89,7 +87,7 @@ def _send_command(
>   
>           Args:
>               command: The command to execute.
> -            timeout: Wait at most this many seconds for the execution to complete.
> +            timeout: Wait at most this long in seconds to execute the command.

Is the timeout actually to start running the command and not to wait for 
it to be completed?

>               env: Extra environment variables that will be used in command execution.
>   
>           Raises:
> @@ -118,6 +116,7 @@ def copy_from(
>           source_file: str | PurePath,
>           destination_file: str | PurePath,
>       ) -> None:
> +        """Overrides :meth:`~.remote_session.RemoteSession.copy_from`."""
>           self.session.get(str(destination_file), str(source_file))
>   
>       def copy_to(
> @@ -125,6 +124,7 @@ def copy_to(
>           source_file: str | PurePath,
>           destination_file: str | PurePath,
>       ) -> None:
> +        """Overrides :meth:`~.remote_session.RemoteSession.copy_to`."""
>           self.session.put(str(source_file), str(destination_file))
>   
>       def _close(self, force: bool = False) -> None:
  

On Tue, Nov 21, 2023 at 4:36 PM Yoan Picchi <yoan.picchi@foss.arm.com> wrote:
>
> On 11/15/23 13:09, Juraj Linkeš wrote:
> > Format according to the Google format and PEP257, with slight
> > deviations.
> >
> > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > ---
> >   dts/framework/remote_session/__init__.py      |  39 +++++-
> >   .../remote_session/remote_session.py          | 128 +++++++++++++-----
> >   dts/framework/remote_session/ssh_session.py   |  16 +--
> >   3 files changed, 135 insertions(+), 48 deletions(-)
> >
> > diff --git a/dts/framework/remote_session/__init__.py b/dts/framework/remote_session/__init__.py
> > index 5e7ddb2b05..51a01d6b5e 100644
> > --- a/dts/framework/remote_session/__init__.py
> > +++ b/dts/framework/remote_session/__init__.py
> > @@ -2,12 +2,14 @@
> >   # Copyright(c) 2023 PANTHEON.tech s.r.o.
> >   # Copyright(c) 2023 University of New Hampshire
> >
> > -"""
> > -The package provides modules for managing remote connections to a remote host (node),
> > -differentiated by OS.
> > -The package provides a factory function, create_session, that returns the appropriate
> > -remote connection based on the passed configuration. The differences are in the
> > -underlying transport protocol (e.g. SSH) and remote OS (e.g. Linux).
> > +"""Remote interactive and non-interactive sessions.
> > +
> > +This package provides modules for managing remote connections to a remote host (node).
> > +
> > +The non-interactive sessions send commands and return their output and exit code.
> > +
> > +The interactive sessions open an interactive shell which is continuously open,
> > +allowing it to send and receive data within that particular shell.
> >   """
> >
> >   # pylama:ignore=W0611
> > @@ -26,10 +28,35 @@
> >   def create_remote_session(
> >       node_config: NodeConfiguration, name: str, logger: DTSLOG
> >   ) -> RemoteSession:
> > +    """Factory for non-interactive remote sessions.
> > +
> > +    The function returns an SSH session, but will be extended if support
> > +    for other protocols is added.
> > +
> > +    Args:
> > +        node_config: The test run configuration of the node to connect to.
> > +        name: The name of the session.
> > +        logger: The logger instance this session will use.
> > +
> > +    Returns:
> > +        The SSH remote session.
> > +    """
> >       return SSHSession(node_config, name, logger)
> >
> >
> >   def create_interactive_session(
> >       node_config: NodeConfiguration, logger: DTSLOG
> >   ) -> InteractiveRemoteSession:
> > +    """Factory for interactive remote sessions.
> > +
> > +    The function returns an interactive SSH session, but will be extended if support
> > +    for other protocols is added.
> > +
> > +    Args:
> > +        node_config: The test run configuration of the node to connect to.
> > +        logger: The logger instance this session will use.
> > +
> > +    Returns:
> > +        The interactive SSH remote session.
> > +    """
> >       return InteractiveRemoteSession(node_config, logger)
> > diff --git a/dts/framework/remote_session/remote_session.py b/dts/framework/remote_session/remote_session.py
> > index 0647d93de4..629c2d7b9c 100644
> > --- a/dts/framework/remote_session/remote_session.py
> > +++ b/dts/framework/remote_session/remote_session.py
> > @@ -3,6 +3,13 @@
> >   # Copyright(c) 2022-2023 PANTHEON.tech s.r.o.
> >   # Copyright(c) 2022-2023 University of New Hampshire
> >
> > +"""Base remote session.
> > +
> > +This module contains the abstract base class for remote sessions and defines
> > +the structure of the result of a command execution.
> > +"""
> > +
> > +
> >   import dataclasses
> >   from abc import ABC, abstractmethod
> >   from pathlib import PurePath
> > @@ -15,8 +22,14 @@
> >
> >   @dataclasses.dataclass(slots=True, frozen=True)
> >   class CommandResult:
> > -    """
> > -    The result of remote execution of a command.
> > +    """The result of remote execution of a command.
> > +
> > +    Attributes:
> > +        name: The name of the session that executed the command.
> > +        command: The executed command.
> > +        stdout: The standard output the command produced.
> > +        stderr: The standard error output the command produced.
> > +        return_code: The return code the command exited with.
> >       """
> >
> >       name: str
> > @@ -26,6 +39,7 @@ class CommandResult:
> >       return_code: int
> >
> >       def __str__(self) -> str:
> > +        """Format the command outputs."""
> >           return (
> >               f"stdout: '{self.stdout}'\n"
> >               f"stderr: '{self.stderr}'\n"
> > @@ -34,13 +48,24 @@ def __str__(self) -> str:
> >
> >
> >   class RemoteSession(ABC):
> > -    """
> > -    The base class for defining which methods must be implemented in order to connect
> > -    to a remote host (node) and maintain a remote session. The derived classes are
> > -    supposed to implement/use some underlying transport protocol (e.g. SSH) to
> > -    implement the methods. On top of that, it provides some basic services common to
> > -    all derived classes, such as keeping history and logging what's being executed
> > -    on the remote node.
> > +    """Non-interactive remote session.
> > +
> > +    The abstract methods must be implemented in order to connect to a remote host (node)
> > +    and maintain a remote session.
> > +    The subclasses must use (or implement) some underlying transport protocol (e.g. SSH)
> > +    to implement the methods. On top of that, it provides some basic services common to all
> > +    subclasses, such as keeping history and logging what's being executed on the remote node.
> > +
> > +    Attributes:
> > +        name: The name of the session.
> > +        hostname: The node's hostname. Could be an IP (possibly with port, separated by a colon)
> > +            or a domain name.
> > +        ip: The IP address of the node or a domain name, whichever was used in `hostname`.
> > +        port: The port of the node, if given in `hostname`.
> > +        username: The username used in the connection.
> > +        password: The password used in the connection. Most frequently empty,
> > +            as the use of passwords is discouraged.
> > +        history: The executed commands during this session.
> >       """
> >
> >       name: str
> > @@ -59,6 +84,16 @@ def __init__(
> >           session_name: str,
> >           logger: DTSLOG,
> >       ):
> > +        """Connect to the node during initialization.
> > +
> > +        Args:
> > +            node_config: The test run configuration of the node to connect to.
> > +            session_name: The name of the session.
> > +            logger: The logger instance this session will use.
> > +
> > +        Raises:
> > +            SSHConnectionError: If the connection to the node was not successful.
> > +        """
> >           self._node_config = node_config
> >
> >           self.name = session_name
> > @@ -79,8 +114,13 @@ def __init__(
> >
> >       @abstractmethod
> >       def _connect(self) -> None:
> > -        """
> > -        Create connection to assigned node.
> > +        """Create a connection to the node.
> > +
> > +        The implementation must assign the established session to self.session.
> > +
> > +        The implementation must except all exceptions and convert them to an SSHConnectionError.
> > +
> > +        The implementation may optionally implement retry attempts.
> >           """
> >
> >       def send_command(
> > @@ -90,11 +130,24 @@ def send_command(
> >           verify: bool = False,
> >           env: dict | None = None,
> >       ) -> CommandResult:
> > -        """
> > -        Send a command to the connected node using optional env vars
> > -        and return CommandResult.
> > -        If verify is True, check the return code of the executed command
> > -        and raise a RemoteCommandExecutionError if the command failed.
> > +        """Send `command` to the connected node.
> > +
> > +        The :option:`--timeout` command line argument and the :envvar:`DTS_TIMEOUT`
> > +        environment variable configure the timeout of command execution.
> > +
> > +        Args:
> > +            command: The command to execute.
> > +            timeout: Wait at most this long in seconds to execute `command`.
> > +            verify: If :data:`True`, will check the exit code of `command`.
> > +            env: A dictionary with environment variables to be used with `command` execution.
> > +
> > +        Raises:
> > +            SSHSessionDeadError: If the session isn't alive when sending `command`.
> > +            SSHTimeoutError: If `command` execution timed out.
> > +            RemoteCommandExecutionError: If verify is :data:`True` and `command` execution failed.
> > +
> > +        Returns:
> > +            The output of the command along with the return code.
> >           """
> >           self._logger.info(
> >               f"Sending: '{command}'" + (f" with env vars: '{env}'" if env else "")
> > @@ -115,29 +168,36 @@ def send_command(
> >       def _send_command(
> >           self, command: str, timeout: float, env: dict | None
> >       ) -> CommandResult:
> > -        """
> > -        Use the underlying protocol to execute the command using optional env vars
> > -        and return CommandResult.
> > +        """Send a command to the connected node.
> > +
> > +        The implementation must execute the command remotely with `env` environment variables
> > +        and return the result.
> > +
> > +        The implementation must except all exceptions and raise an SSHSessionDeadError if
> > +        the session is not alive and an SSHTimeoutError if the command execution times out.
>
> 3 way "and". Needs comas or splitting the sentence.
>

What about this?

The implementation must except all exceptions and raise:

    * SSHSessionDeadError if the session is not alive,
    * SSHTimeoutError if the command execution times out.


> >           """
> >
> >       def close(self, force: bool = False) -> None:
> > -        """
> > -        Close the remote session and free all used resources.
> > +        """Close the remote session and free all used resources.
> > +
> > +        Args:
> > +            force: Force the closure of the connection. This may not clean up all resources.
> >           """
> >           self._logger.logger_exit()
> >           self._close(force)
> >
> >       @abstractmethod
> >       def _close(self, force: bool = False) -> None:
> > -        """
> > -        Execute protocol specific steps needed to close the session properly.
> > +        """Protocol specific steps needed to close the session properly.
> > +
> > +        Args:
> > +            force: Force the closure of the connection. This may not clean up all resources.
> > +                This doesn't have to be implemented in the overloaded method.
> >           """
> >
> >       @abstractmethod
> >       def is_alive(self) -> bool:
> > -        """
> > -        Check whether the remote session is still responding.
> > -        """
> > +        """Check whether the remote session is still responding."""
> >
> >       @abstractmethod
> >       def copy_from(
> > @@ -147,12 +207,12 @@ def copy_from(
> >       ) -> None:
> >           """Copy a file from the remote Node to the local filesystem.
> >
> > -        Copy source_file from the remote Node associated with this remote
> > -        session to destination_file on the local filesystem.
> > +        Copy `source_file` from the remote Node associated with this remote session
> > +        to `destination_file` on the local filesystem.
> >
> >           Args:
> > -            source_file: the file on the remote Node.
> > -            destination_file: a file or directory path on the local filesystem.
> > +            source_file: The file on the remote Node.
> > +            destination_file: A file or directory path on the local filesystem.
> >           """
> >
> >       @abstractmethod
> > @@ -163,10 +223,10 @@ def copy_to(
> >       ) -> None:
> >           """Copy a file from local filesystem to the remote Node.
> >
> > -        Copy source_file from local filesystem to destination_file
> > -        on the remote Node associated with this remote session.
> > +        Copy `source_file` from local filesystem to `destination_file` on the remote Node
> > +        associated with this remote session.
> >
> >           Args:
> > -            source_file: the file on the local filesystem.
> > -            destination_file: a file or directory path on the remote Node.
> > +            source_file: The file on the local filesystem.
> > +            destination_file: A file or directory path on the remote Node.
> >           """
> > diff --git a/dts/framework/remote_session/ssh_session.py b/dts/framework/remote_session/ssh_session.py
> > index cee11d14d6..7186490a9a 100644
> > --- a/dts/framework/remote_session/ssh_session.py
> > +++ b/dts/framework/remote_session/ssh_session.py
> > @@ -1,6 +1,8 @@
> >   # SPDX-License-Identifier: BSD-3-Clause
> >   # Copyright(c) 2023 PANTHEON.tech s.r.o.
> >
> > +"""SSH session remote session."""
>
> Is the double "session" intended?
>

Not really, I'll remove the first occurence.

> > +
> >   import socket
> >   import traceback
> >   from pathlib import PurePath
> > @@ -26,13 +28,8 @@
> >   class SSHSession(RemoteSession):
> >       """A persistent SSH connection to a remote Node.
> >
> > -    The connection is implemented with the Fabric Python library.
> > -
> > -    Args:
> > -        node_config: The configuration of the Node to connect to.
> > -        session_name: The name of the session.
> > -        logger: The logger used for logging.
> > -            This should be passed from the parent OSSession.
> > +    The connection is implemented with
> > +    `the Fabric Python library <https://docs.fabfile.org/en/latest/>`_.
> >
> >       Attributes:
> >           session: The underlying Fabric SSH connection.
> > @@ -80,6 +77,7 @@ def _connect(self) -> None:
> >               raise SSHConnectionError(self.hostname, errors)
> >
> >       def is_alive(self) -> bool:
> > +        """Overrides :meth:`~.remote_session.RemoteSession.is_alive`."""
> >           return self.session.is_connected
> >
> >       def _send_command(
> > @@ -89,7 +87,7 @@ def _send_command(
> >
> >           Args:
> >               command: The command to execute.
> > -            timeout: Wait at most this many seconds for the execution to complete.
> > +            timeout: Wait at most this long in seconds to execute the command.
>
> Is the timeout actually to start running the command and not to wait for
> it to be completed?
>

It is to wait for it to be completed. The wording is a bit confusing,
what about:

Wait at most this long in seconds for the command execution to complete.

I'll change this in all places where timeout is documented.

> >               env: Extra environment variables that will be used in command execution.
> >
> >           Raises:
> > @@ -118,6 +116,7 @@ def copy_from(
> >           source_file: str | PurePath,
> >           destination_file: str | PurePath,
> >       ) -> None:
> > +        """Overrides :meth:`~.remote_session.RemoteSession.copy_from`."""
> >           self.session.get(str(destination_file), str(source_file))
> >
> >       def copy_to(
> > @@ -125,6 +124,7 @@ def copy_to(
> >           source_file: str | PurePath,
> >           destination_file: str | PurePath,
> >       ) -> None:
> > +        """Overrides :meth:`~.remote_session.RemoteSession.copy_to`."""
> >           self.session.put(str(source_file), str(destination_file))
> >
> >       def _close(self, force: bool = False) -> None:
>
  

On 11/22/23 11:13, Juraj Linkeš wrote:
> On Tue, Nov 21, 2023 at 4:36 PM Yoan Picchi <yoan.picchi@foss.arm.com> wrote:
>>
>> On 11/15/23 13:09, Juraj Linkeš wrote:
>>> Format according to the Google format and PEP257, with slight
>>> deviations.
>>>
>>> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
>>> ---
>>>    dts/framework/remote_session/__init__.py      |  39 +++++-
>>>    .../remote_session/remote_session.py          | 128 +++++++++++++-----
>>>    dts/framework/remote_session/ssh_session.py   |  16 +--
>>>    3 files changed, 135 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/dts/framework/remote_session/__init__.py b/dts/framework/remote_session/__init__.py
>>> index 5e7ddb2b05..51a01d6b5e 100644
>>> --- a/dts/framework/remote_session/__init__.py
>>> +++ b/dts/framework/remote_session/__init__.py
>>> @@ -2,12 +2,14 @@
>>>    # Copyright(c) 2023 PANTHEON.tech s.r.o.
>>>    # Copyright(c) 2023 University of New Hampshire
>>>
>>> -"""
>>> -The package provides modules for managing remote connections to a remote host (node),
>>> -differentiated by OS.
>>> -The package provides a factory function, create_session, that returns the appropriate
>>> -remote connection based on the passed configuration. The differences are in the
>>> -underlying transport protocol (e.g. SSH) and remote OS (e.g. Linux).
>>> +"""Remote interactive and non-interactive sessions.
>>> +
>>> +This package provides modules for managing remote connections to a remote host (node).
>>> +
>>> +The non-interactive sessions send commands and return their output and exit code.
>>> +
>>> +The interactive sessions open an interactive shell which is continuously open,
>>> +allowing it to send and receive data within that particular shell.
>>>    """
>>>
>>>    # pylama:ignore=W0611
>>> @@ -26,10 +28,35 @@
>>>    def create_remote_session(
>>>        node_config: NodeConfiguration, name: str, logger: DTSLOG
>>>    ) -> RemoteSession:
>>> +    """Factory for non-interactive remote sessions.
>>> +
>>> +    The function returns an SSH session, but will be extended if support
>>> +    for other protocols is added.
>>> +
>>> +    Args:
>>> +        node_config: The test run configuration of the node to connect to.
>>> +        name: The name of the session.
>>> +        logger: The logger instance this session will use.
>>> +
>>> +    Returns:
>>> +        The SSH remote session.
>>> +    """
>>>        return SSHSession(node_config, name, logger)
>>>
>>>
>>>    def create_interactive_session(
>>>        node_config: NodeConfiguration, logger: DTSLOG
>>>    ) -> InteractiveRemoteSession:
>>> +    """Factory for interactive remote sessions.
>>> +
>>> +    The function returns an interactive SSH session, but will be extended if support
>>> +    for other protocols is added.
>>> +
>>> +    Args:
>>> +        node_config: The test run configuration of the node to connect to.
>>> +        logger: The logger instance this session will use.
>>> +
>>> +    Returns:
>>> +        The interactive SSH remote session.
>>> +    """
>>>        return InteractiveRemoteSession(node_config, logger)
>>> diff --git a/dts/framework/remote_session/remote_session.py b/dts/framework/remote_session/remote_session.py
>>> index 0647d93de4..629c2d7b9c 100644
>>> --- a/dts/framework/remote_session/remote_session.py
>>> +++ b/dts/framework/remote_session/remote_session.py
>>> @@ -3,6 +3,13 @@
>>>    # Copyright(c) 2022-2023 PANTHEON.tech s.r.o.
>>>    # Copyright(c) 2022-2023 University of New Hampshire
>>>
>>> +"""Base remote session.
>>> +
>>> +This module contains the abstract base class for remote sessions and defines
>>> +the structure of the result of a command execution.
>>> +"""
>>> +
>>> +
>>>    import dataclasses
>>>    from abc import ABC, abstractmethod
>>>    from pathlib import PurePath
>>> @@ -15,8 +22,14 @@
>>>
>>>    @dataclasses.dataclass(slots=True, frozen=True)
>>>    class CommandResult:
>>> -    """
>>> -    The result of remote execution of a command.
>>> +    """The result of remote execution of a command.
>>> +
>>> +    Attributes:
>>> +        name: The name of the session that executed the command.
>>> +        command: The executed command.
>>> +        stdout: The standard output the command produced.
>>> +        stderr: The standard error output the command produced.
>>> +        return_code: The return code the command exited with.
>>>        """
>>>
>>>        name: str
>>> @@ -26,6 +39,7 @@ class CommandResult:
>>>        return_code: int
>>>
>>>        def __str__(self) -> str:
>>> +        """Format the command outputs."""
>>>            return (
>>>                f"stdout: '{self.stdout}'\n"
>>>                f"stderr: '{self.stderr}'\n"
>>> @@ -34,13 +48,24 @@ def __str__(self) -> str:
>>>
>>>
>>>    class RemoteSession(ABC):
>>> -    """
>>> -    The base class for defining which methods must be implemented in order to connect
>>> -    to a remote host (node) and maintain a remote session. The derived classes are
>>> -    supposed to implement/use some underlying transport protocol (e.g. SSH) to
>>> -    implement the methods. On top of that, it provides some basic services common to
>>> -    all derived classes, such as keeping history and logging what's being executed
>>> -    on the remote node.
>>> +    """Non-interactive remote session.
>>> +
>>> +    The abstract methods must be implemented in order to connect to a remote host (node)
>>> +    and maintain a remote session.
>>> +    The subclasses must use (or implement) some underlying transport protocol (e.g. SSH)
>>> +    to implement the methods. On top of that, it provides some basic services common to all
>>> +    subclasses, such as keeping history and logging what's being executed on the remote node.
>>> +
>>> +    Attributes:
>>> +        name: The name of the session.
>>> +        hostname: The node's hostname. Could be an IP (possibly with port, separated by a colon)
>>> +            or a domain name.
>>> +        ip: The IP address of the node or a domain name, whichever was used in `hostname`.
>>> +        port: The port of the node, if given in `hostname`.
>>> +        username: The username used in the connection.
>>> +        password: The password used in the connection. Most frequently empty,
>>> +            as the use of passwords is discouraged.
>>> +        history: The executed commands during this session.
>>>        """
>>>
>>>        name: str
>>> @@ -59,6 +84,16 @@ def __init__(
>>>            session_name: str,
>>>            logger: DTSLOG,
>>>        ):
>>> +        """Connect to the node during initialization.
>>> +
>>> +        Args:
>>> +            node_config: The test run configuration of the node to connect to.
>>> +            session_name: The name of the session.
>>> +            logger: The logger instance this session will use.
>>> +
>>> +        Raises:
>>> +            SSHConnectionError: If the connection to the node was not successful.
>>> +        """
>>>            self._node_config = node_config
>>>
>>>            self.name = session_name
>>> @@ -79,8 +114,13 @@ def __init__(
>>>
>>>        @abstractmethod
>>>        def _connect(self) -> None:
>>> -        """
>>> -        Create connection to assigned node.
>>> +        """Create a connection to the node.
>>> +
>>> +        The implementation must assign the established session to self.session.
>>> +
>>> +        The implementation must except all exceptions and convert them to an SSHConnectionError.
>>> +
>>> +        The implementation may optionally implement retry attempts.
>>>            """
>>>
>>>        def send_command(
>>> @@ -90,11 +130,24 @@ def send_command(
>>>            verify: bool = False,
>>>            env: dict | None = None,
>>>        ) -> CommandResult:
>>> -        """
>>> -        Send a command to the connected node using optional env vars
>>> -        and return CommandResult.
>>> -        If verify is True, check the return code of the executed command
>>> -        and raise a RemoteCommandExecutionError if the command failed.
>>> +        """Send `command` to the connected node.
>>> +
>>> +        The :option:`--timeout` command line argument and the :envvar:`DTS_TIMEOUT`
>>> +        environment variable configure the timeout of command execution.
>>> +
>>> +        Args:
>>> +            command: The command to execute.
>>> +            timeout: Wait at most this long in seconds to execute `command`.
>>> +            verify: If :data:`True`, will check the exit code of `command`.
>>> +            env: A dictionary with environment variables to be used with `command` execution.
>>> +
>>> +        Raises:
>>> +            SSHSessionDeadError: If the session isn't alive when sending `command`.
>>> +            SSHTimeoutError: If `command` execution timed out.
>>> +            RemoteCommandExecutionError: If verify is :data:`True` and `command` execution failed.
>>> +
>>> +        Returns:
>>> +            The output of the command along with the return code.
>>>            """
>>>            self._logger.info(
>>>                f"Sending: '{command}'" + (f" with env vars: '{env}'" if env else "")
>>> @@ -115,29 +168,36 @@ def send_command(
>>>        def _send_command(
>>>            self, command: str, timeout: float, env: dict | None
>>>        ) -> CommandResult:
>>> -        """
>>> -        Use the underlying protocol to execute the command using optional env vars
>>> -        and return CommandResult.
>>> +        """Send a command to the connected node.
>>> +
>>> +        The implementation must execute the command remotely with `env` environment variables
>>> +        and return the result.
>>> +
>>> +        The implementation must except all exceptions and raise an SSHSessionDeadError if
>>> +        the session is not alive and an SSHTimeoutError if the command execution times out.
>>
>> 3 way "and". Needs comas or splitting the sentence.
>>
> 
> What about this?
> 
> The implementation must except all exceptions and raise:
> 
>      * SSHSessionDeadError if the session is not alive,
>      * SSHTimeoutError if the command execution times out.
> 

Sounds good.

> 
>>>            """
>>>
>>>        def close(self, force: bool = False) -> None:
>>> -        """
>>> -        Close the remote session and free all used resources.
>>> +        """Close the remote session and free all used resources.
>>> +
>>> +        Args:
>>> +            force: Force the closure of the connection. This may not clean up all resources.
>>>            """
>>>            self._logger.logger_exit()
>>>            self._close(force)
>>>
>>>        @abstractmethod
>>>        def _close(self, force: bool = False) -> None:
>>> -        """
>>> -        Execute protocol specific steps needed to close the session properly.
>>> +        """Protocol specific steps needed to close the session properly.
>>> +
>>> +        Args:
>>> +            force: Force the closure of the connection. This may not clean up all resources.
>>> +                This doesn't have to be implemented in the overloaded method.
>>>            """
>>>
>>>        @abstractmethod
>>>        def is_alive(self) -> bool:
>>> -        """
>>> -        Check whether the remote session is still responding.
>>> -        """
>>> +        """Check whether the remote session is still responding."""
>>>
>>>        @abstractmethod
>>>        def copy_from(
>>> @@ -147,12 +207,12 @@ def copy_from(
>>>        ) -> None:
>>>            """Copy a file from the remote Node to the local filesystem.
>>>
>>> -        Copy source_file from the remote Node associated with this remote
>>> -        session to destination_file on the local filesystem.
>>> +        Copy `source_file` from the remote Node associated with this remote session
>>> +        to `destination_file` on the local filesystem.
>>>
>>>            Args:
>>> -            source_file: the file on the remote Node.
>>> -            destination_file: a file or directory path on the local filesystem.
>>> +            source_file: The file on the remote Node.
>>> +            destination_file: A file or directory path on the local filesystem.
>>>            """
>>>
>>>        @abstractmethod
>>> @@ -163,10 +223,10 @@ def copy_to(
>>>        ) -> None:
>>>            """Copy a file from local filesystem to the remote Node.
>>>
>>> -        Copy source_file from local filesystem to destination_file
>>> -        on the remote Node associated with this remote session.
>>> +        Copy `source_file` from local filesystem to `destination_file` on the remote Node
>>> +        associated with this remote session.
>>>
>>>            Args:
>>> -            source_file: the file on the local filesystem.
>>> -            destination_file: a file or directory path on the remote Node.
>>> +            source_file: The file on the local filesystem.
>>> +            destination_file: A file or directory path on the remote Node.
>>>            """
>>> diff --git a/dts/framework/remote_session/ssh_session.py b/dts/framework/remote_session/ssh_session.py
>>> index cee11d14d6..7186490a9a 100644
>>> --- a/dts/framework/remote_session/ssh_session.py
>>> +++ b/dts/framework/remote_session/ssh_session.py
>>> @@ -1,6 +1,8 @@
>>>    # SPDX-License-Identifier: BSD-3-Clause
>>>    # Copyright(c) 2023 PANTHEON.tech s.r.o.
>>>
>>> +"""SSH session remote session."""
>>
>> Is the double "session" intended?
>>
> 
> Not really, I'll remove the first occurence.
> 
>>> +
>>>    import socket
>>>    import traceback
>>>    from pathlib import PurePath
>>> @@ -26,13 +28,8 @@
>>>    class SSHSession(RemoteSession):
>>>        """A persistent SSH connection to a remote Node.
>>>
>>> -    The connection is implemented with the Fabric Python library.
>>> -
>>> -    Args:
>>> -        node_config: The configuration of the Node to connect to.
>>> -        session_name: The name of the session.
>>> -        logger: The logger used for logging.
>>> -            This should be passed from the parent OSSession.
>>> +    The connection is implemented with
>>> +    `the Fabric Python library <https://docs.fabfile.org/en/latest/>`_.
>>>
>>>        Attributes:
>>>            session: The underlying Fabric SSH connection.
>>> @@ -80,6 +77,7 @@ def _connect(self) -> None:
>>>                raise SSHConnectionError(self.hostname, errors)
>>>
>>>        def is_alive(self) -> bool:
>>> +        """Overrides :meth:`~.remote_session.RemoteSession.is_alive`."""
>>>            return self.session.is_connected
>>>
>>>        def _send_command(
>>> @@ -89,7 +87,7 @@ def _send_command(
>>>
>>>            Args:
>>>                command: The command to execute.
>>> -            timeout: Wait at most this many seconds for the execution to complete.
>>> +            timeout: Wait at most this long in seconds to execute the command.
>>
>> Is the timeout actually to start running the command and not to wait for
>> it to be completed?
>>
> 
> It is to wait for it to be completed. The wording is a bit confusing,
> what about:
> 
> Wait at most this long in seconds for the command execution to complete.
> 
> I'll change this in all places where timeout is documented.

Sounds good. I think I saw this confusing wording 3 times so far, but I 
can't quite remember which files it was.

> 
>>>                env: Extra environment variables that will be used in command execution.
>>>
>>>            Raises:
>>> @@ -118,6 +116,7 @@ def copy_from(
>>>            source_file: str | PurePath,
>>>            destination_file: str | PurePath,
>>>        ) -> None:
>>> +        """Overrides :meth:`~.remote_session.RemoteSession.copy_from`."""
>>>            self.session.get(str(destination_file), str(source_file))
>>>
>>>        def copy_to(
>>> @@ -125,6 +124,7 @@ def copy_to(
>>>            source_file: str | PurePath,
>>>            destination_file: str | PurePath,
>>>        ) -> None:
>>> +        """Overrides :meth:`~.remote_session.RemoteSession.copy_to`."""
>>>            self.session.put(str(source_file), str(destination_file))
>>>
>>>        def _close(self, force: bool = False) -> None:
>>