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

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On Thu, Nov 30, 2023 at 10:50 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
>
>
> 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?
>

I'll add it, as it sounds fine to me (it is a real contribution), but
I actually don't know.

>>
>> +"""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
>>