Re: [PATCH v8 12/21] dts: interactive remote session docstring update

[Jeremy Spewock <jspewock@iol.unh.edu>]

[-- Attachment #1: Type: text/plain, Size: 18142 bytes --]

On Thu, Nov 23, 2023 at 10:14 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:

> Format according to the Google format and PEP257, with slight
> deviations.
>
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
>  .../interactive_remote_session.py             | 36 +++----
>  .../remote_session/interactive_shell.py       | 99 +++++++++++--------
>  dts/framework/remote_session/python_shell.py  | 26 ++++-
>  dts/framework/remote_session/testpmd_shell.py | 58 +++++++++--
>  4 files changed, 149 insertions(+), 70 deletions(-)
>
> diff --git a/dts/framework/remote_session/interactive_remote_session.py
> b/dts/framework/remote_session/interactive_remote_session.py
> index 098ded1bb0..1cc82e3377 100644
> --- a/dts/framework/remote_session/interactive_remote_session.py
> +++ b/dts/framework/remote_session/interactive_remote_session.py
> @@ -22,27 +22,23 @@
>  class InteractiveRemoteSession:
>      """SSH connection dedicated to interactive applications.
>
> -    This connection is created using paramiko and is a persistent
> connection to the
> -    host. This class defines methods for connecting to the node and
> configures this
> -    connection to send "keep alive" packets every 30 seconds. Because
> paramiko attempts
> -    to use SSH keys to establish a connection first, providing a password
> is optional.
> -    This session is utilized by InteractiveShells and cannot be
> interacted with
> -    directly.
> -
> -    Arguments:
> -        node_config: Configuration class for the node you are connecting
> to.
> -        _logger: Desired logger for this session to use.
> +    The connection is created using `paramiko <
> https://docs.paramiko.org/en/latest/>`_
> +    and is a persistent connection to the host. This class defines the
> methods for connecting
> +    to the node and configures the connection to send "keep alive"
> packets every 30 seconds.
> +    Because paramiko attempts to use SSH keys to establish a connection
> first, providing
> +    a password is optional. This session is utilized by InteractiveShells
> +    and cannot be interacted with directly.
>
>      Attributes:
> -        hostname: Hostname that will be used to initialize a connection
> to the node.
> -        ip: A subsection of hostname that removes the port for the
> connection if there
> +        hostname: The hostname that will be used to initialize a
> connection to the node.
> +        ip: A subsection of `hostname` that removes the port for the
> connection if there
>              is one. If there is no port, this will be the same as
> hostname.
> -        port: Port to use for the ssh connection. This will be extracted
> from the
> -            hostname if there is a port included, otherwise it will
> default to 22.
> +        port: Port to use for the ssh connection. This will be extracted
> from `hostname`
> +            if there is a port included, otherwise it will default to
> ``22``.
>          username: User to connect to the node with.
>          password: Password of the user connecting to the host. This will
> default to an
>              empty string if a password is not provided.
> -        session: Underlying paramiko connection.
> +        session: The underlying paramiko connection.
>
>      Raises:
>          SSHConnectionError: There is an error creating the SSH connection.
> @@ -58,9 +54,15 @@ class InteractiveRemoteSession:
>      _node_config: NodeConfiguration
>      _transport: Transport | None
>
> -    def __init__(self, node_config: NodeConfiguration, _logger: DTSLOG)
> -> None:
> +    def __init__(self, node_config: NodeConfiguration, logger: DTSLOG) ->
> None:
> +        """Connect to the node during initialization.
> +
> +        Args:
> +            node_config: The test run configuration of the node to
> connect to.
> +            logger: The logger instance this session will use.
> +        """
>          self._node_config = node_config
> -        self._logger = _logger
> +        self._logger = logger
>          self.hostname = node_config.hostname
>          self.username = node_config.user
>          self.password = node_config.password if node_config.password else
> ""
> diff --git a/dts/framework/remote_session/interactive_shell.py
> b/dts/framework/remote_session/interactive_shell.py
> index 4db19fb9b3..b158f963b6 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -3,18 +3,20 @@
>
>  """Common functionality for interactive shell handling.
>
> -This base class, InteractiveShell, is meant to be extended by other
> classes that
> -contain functionality specific to that shell type. These derived classes
> will often
> -modify things like the prompt to expect or the arguments to pass into the
> application,
> -but still utilize the same method for sending a command and collecting
> output. How
> -this output is handled however is often application specific. If an
> application needs
> -elevated privileges to start it is expected that the method for gaining
> those
> -privileges is provided when initializing the class.
> +The base class, :class:`InteractiveShell`, is meant to be extended by
> subclasses that contain
> +functionality specific to that shell type. These subclasses will often
> modify things like
> +the prompt to expect or the arguments to pass into the application, but
> still utilize
> +the same method for sending a command and collecting output. How this
> output is handled however
> +is often application specific. If an application needs elevated
> privileges to start it is expected
> +that the method for gaining those privileges is provided when
> initializing the class.
> +
> +The :option:`--timeout` command line argument and the
> :envvar:`DTS_TIMEOUT`
> +environment variable configure the timeout of getting the output from
> command execution.
>  """
>
>  from abc import ABC
>  from pathlib import PurePath
> -from typing import Callable
> +from typing import Callable, ClassVar
>
>  from paramiko import Channel, SSHClient, channel  # type: ignore[import]
>
> @@ -30,28 +32,6 @@ class InteractiveShell(ABC):
>      and collecting input until reaching a certain prompt. All interactive
> applications
>      will use the same SSH connection, but each will create their own
> channel on that
>      session.
> -
> -    Arguments:
> -        interactive_session: The SSH session dedicated to interactive
> shells.
> -        logger: Logger used for displaying information in the console.
> -        get_privileged_command: Method for modifying a command to allow
> it to use
> -            elevated privileges. If this is None, the application will
> not be started
> -            with elevated privileges.
> -        app_args: Command line arguments to be passed to the application
> on startup.
> -        timeout: Timeout used for the SSH channel that is dedicated to
> this interactive
> -            shell. This timeout is for collecting output, so if reading
> from the buffer
> -            and no output is gathered within the timeout, an exception is
> thrown.
> -
> -    Attributes
> -        _default_prompt: Prompt to expect at the end of output when
> sending a command.
> -            This is often overridden by derived classes.
> -        _command_extra_chars: Extra characters to add to the end of every
> command
> -            before sending them. This is often overridden by derived
> classes and is
> -            most commonly an additional newline character.
> -        path: Path to the executable to start the interactive application.
> -        dpdk_app: Whether this application is a DPDK app. If it is, the
> build
> -            directory for DPDK on the node will be prepended to the path
> to the
> -            executable.
>      """
>
>      _interactive_session: SSHClient
> @@ -61,10 +41,22 @@ class InteractiveShell(ABC):
>      _logger: DTSLOG
>      _timeout: float
>      _app_args: str
> -    _default_prompt: str = ""
> -    _command_extra_chars: str = ""
> -    path: PurePath
> -    dpdk_app: bool = False
> +
> +    #: Prompt to expect at the end of output when sending a command.
> +    #: This is often overridden by subclasses.
> +    _default_prompt: ClassVar[str] = ""
> +
> +    #: Extra characters to add to the end of every command
> +    #: before sending them. This is often overridden by subclasses and is
> +    #: most commonly an additional newline character.
> +    _command_extra_chars: ClassVar[str] = ""
> +
> +    #: Path to the executable to start the interactive application.
> +    path: ClassVar[PurePath]
> +
> +    #: Whether this application is a DPDK app. If it is, the build
> directory
> +    #: for DPDK on the node will be prepended to the path to the
> executable.
> +    dpdk_app: ClassVar[bool] = False
>
>      def __init__(
>          self,
> @@ -74,6 +66,19 @@ def __init__(
>          app_args: str = "",
>          timeout: float = SETTINGS.timeout,
>      ) -> None:
> +        """Create an SSH channel during initialization.
> +
> +        Args:
> +            interactive_session: The SSH session dedicated to interactive
> shells.
> +            logger: The logger instance this session will use.
> +            get_privileged_command: A method for modifying a command to
> allow it to use
> +                elevated privileges. If :data:`None`, the application
> will not be started
> +                with elevated privileges.
> +            app_args: The command line arguments to be passed to the
> application on startup.
> +            timeout: The timeout used for the SSH channel that is
> dedicated to this interactive
> +                shell. This timeout is for collecting output, so if
> reading from the buffer
> +                and no output is gathered within the timeout, an
> exception is thrown.
> +        """
>          self._interactive_session = interactive_session
>          self._ssh_channel = self._interactive_session.invoke_shell()
>          self._stdin = self._ssh_channel.makefile_stdin("w")
> @@ -90,6 +95,10 @@ def _start_application(self, get_privileged_command:
> Callable[[str], str] | None
>
>          This method is often overridden by subclasses as their process for
>          starting may look different.
> +
> +        Args:
> +            get_privileged_command: A function (but could be any
> callable) that produces
> +                the version of the command with elevated privileges.
>          """
>          start_command = f"{self.path} {self._app_args}"
>          if get_privileged_command is not None:
> @@ -97,16 +106,24 @@ def _start_application(self, get_privileged_command:
> Callable[[str], str] | None
>          self.send_command(start_command)
>
>      def send_command(self, command: str, prompt: str | None = None) ->
> str:
> -        """Send a command and get all output before the expected ending
> string.
> +        """Send `command` and get all output before the expected ending
> string.
>
>          Lines that expect input are not included in the stdout buffer, so
> they cannot
> -        be used for expect. For example, if you were prompted to log into
> something
> -        with a username and password, you cannot expect "username:"
> because it won't
> -        yet be in the stdout buffer. A workaround for this could be
> consuming an
> -        extra newline character to force the current prompt into the
> stdout buffer.
> +        be used for expect.
> +
> +        Example:
> +            If you were prompted to log into something with a username
> and password,
> +            you cannot expect ``username:`` because it won't yet be in
> the stdout buffer.
> +            A workaround for this could be consuming an extra newline
> character to force
> +            the current `prompt` into the stdout buffer.
> +
> +        Args:
> +            command: The command to send.
> +            prompt: After sending the command, `send_command` will be
> expecting this string.
> +                If :data:`None`, will use the class's default prompt.
>
>          Returns:
> -            All output in the buffer before expected string
> +            All output in the buffer before expected string.
>          """
>          self._logger.info(f"Sending: '{command}'")
>          if prompt is None:
> @@ -124,8 +141,10 @@ def send_command(self, command: str, prompt: str |
> None = None) -> str:
>          return out
>
>      def close(self) -> None:
> +        """Properly free all resources."""
>          self._stdin.close()
>          self._ssh_channel.close()
>
>      def __del__(self) -> None:
> +        """Make sure the session is properly closed before deleting the
> object."""
>          self.close()
> diff --git a/dts/framework/remote_session/python_shell.py
> b/dts/framework/remote_session/python_shell.py
> index cc3ad48a68..ccfd3783e8 100644
> --- a/dts/framework/remote_session/python_shell.py
> +++ b/dts/framework/remote_session/python_shell.py
> @@ -1,12 +1,32 @@
>  # SPDX-License-Identifier: BSD-3-Clause
>  # Copyright(c) 2023 PANTHEON.tech s.r.o.
>
> +"""Python interactive shell.
> +
> +Typical usage example in a TestSuite::
> +
> +    from framework.remote_session import PythonShell
> +    python_shell = self.tg_node.create_interactive_shell(
> +        PythonShell, timeout=5, privileged=True
> +    )
> +    python_shell.send_command("print('Hello World')")
> +    python_shell.close()
> +"""
> +
>  from pathlib import PurePath
> +from typing import ClassVar
>
>  from .interactive_shell import InteractiveShell
>
>
>  class PythonShell(InteractiveShell):
> -    _default_prompt: str = ">>>"
> -    _command_extra_chars: str = "\n"
> -    path: PurePath = PurePath("python3")
> +    """Python interactive shell."""
> +
> +    #: Python's prompt.
> +    _default_prompt: ClassVar[str] = ">>>"
> +
> +    #: This forces the prompt to appear after sending a command.
> +    _command_extra_chars: ClassVar[str] = "\n"
> +
> +    #: The Python executable.
> +    path: ClassVar[PurePath] = PurePath("python3")
> diff --git a/dts/framework/remote_session/testpmd_shell.py
> b/dts/framework/remote_session/testpmd_shell.py
> index 08ac311016..79481e845c 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -1,41 +1,79 @@
>  # SPDX-License-Identifier: BSD-3-Clause
>  # Copyright(c) 2023 University of New Hampshire
>
>
Should you add to the copyright here for adding comments?


> +"""Testpmd interactive shell.
> +
> +Typical usage example in a TestSuite::
> +
> +    testpmd_shell = self.sut_node.create_interactive_shell(
> +            TestPmdShell, privileged=True
> +        )
> +    devices = testpmd_shell.get_devices()
> +    for device in devices:
> +        print(device)
> +    testpmd_shell.close()
> +"""
> +
>  from pathlib import PurePath
> -from typing import Callable
> +from typing import Callable, ClassVar
>
>  from .interactive_shell import InteractiveShell
>
>
>  class TestPmdDevice(object):
> +    """The data of a device that testpmd can recognize.
> +
> +    Attributes:
> +        pci_address: The PCI address of the device.
> +    """
> +
>      pci_address: str
>
>      def __init__(self, pci_address_line: str):
> +        """Initialize the device from the testpmd output line string.
> +
> +        Args:
> +            pci_address_line: A line of testpmd output that contains a
> device.
> +        """
>          self.pci_address = pci_address_line.strip().split(": ")[1].strip()
>
>      def __str__(self) -> str:
> +        """The PCI address captures what the device is."""
>          return self.pci_address
>
>
>  class TestPmdShell(InteractiveShell):
> -    path: PurePath = PurePath("app", "dpdk-testpmd")
> -    dpdk_app: bool = True
> -    _default_prompt: str = "testpmd>"
> -    _command_extra_chars: str = "\n"  # We want to append an extra
> newline to every command
> +    """Testpmd interactive shell.
> +
> +    The testpmd shell users should never use
> +    the :meth:`~.interactive_shell.InteractiveShell.send_command` method
> directly, but rather
> +    call specialized methods. If there isn't one that satisfies a need,
> it should be added.
> +    """
> +
> +    #: The path to the testpmd executable.
> +    path: ClassVar[PurePath] = PurePath("app", "dpdk-testpmd")
> +
> +    #: Flag this as a DPDK app so that it's clear this is not a system
> app and
> +    #: needs to be looked in a specific path.
> +    dpdk_app: ClassVar[bool] = True
> +
> +    #: The testpmd's prompt.
> +    _default_prompt: ClassVar[str] = "testpmd>"
> +
> +    #: This forces the prompt to appear after sending a command.
> +    _command_extra_chars: ClassVar[str] = "\n"
>
>      def _start_application(self, get_privileged_command: Callable[[str],
> str] | None) -> None:
> -        """See "_start_application" in InteractiveShell."""
>          self._app_args += " -- -i"
>          super()._start_application(get_privileged_command)
>
>      def get_devices(self) -> list[TestPmdDevice]:
> -        """Get a list of device names that are known to testpmd
> +        """Get a list of device names that are known to testpmd.
>
> -        Uses the device info listed in testpmd and then parses the output
> to
> -        return only the names of the devices.
> +        Uses the device info listed in testpmd and then parses the output.
>
>          Returns:
> -            A list of strings representing device names (e.g.
> 0000:14:00.1)
> +            A list of devices.
>          """
>          dev_info: str = self.send_command("show device info all")
>          dev_list: list[TestPmdDevice] = []
> --
> 2.34.1
>
>

[-- Attachment #2: Type: text/html, Size: 21199 bytes --]