You forgot to add me to the patch :-) A few comments below.
On Mon, Jul 17, 2023 at 9:37 PM <jspew...@iol.unh.edu> wrote: > > From: Jeremy Spewock <jspew...@iol.unh.edu> > > Adds a new test suite for running smoke tests that verify general > configuration aspects of the system under test. If any of these tests > fail, the DTS execution terminates as part of a "fail-fast" model. > > Signed-off-by: Jeremy Spewock <jspew...@iol.unh.edu> > --- <snip> > diff --git a/dts/framework/remote_session/remote/interactive_shell.py > b/dts/framework/remote_session/remote/interactive_shell.py > new file mode 100644 > index 0000000000..4d9c7638a5 > --- /dev/null > +++ b/dts/framework/remote_session/remote/interactive_shell.py > @@ -0,0 +1,98 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2023 University of New Hampshire > + > +from pathlib import PurePath > +from typing import Callable > + > +from paramiko import Channel, SSHClient, channel # type: ignore > + > +from framework.logger import DTSLOG > +from framework.settings import SETTINGS > + > + > +class InteractiveShell: > + > + _interactive_session: SSHClient > + _stdin: channel.ChannelStdinFile > + _stdout: channel.ChannelFile > + _ssh_channel: Channel > + _logger: DTSLOG > + _timeout: float > + _startup_command: str > + _app_args: str > + _default_prompt: str = "" > + _privileged: bool > + _get_privileged_command: Callable[[str], str] > + # Allows for app specific extra characters to be appended to commands > + _command_extra_chars: str = "" > + path: PurePath > + dpdk_app: bool = False > + We should document this class - let's use the google format: https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings You can use this as an example: http://patches.dpdk.org/project/dpdk/patch/20230511091408.236638-5-juraj.lin...@pantheon.tech/ A note on class attributes: we should document the public ones as well as those which should be considered to be overridden in derived classes. > + def __init__( > + self, > + interactive_session: SSHClient, > + logger: DTSLOG, > + startup_command: str, > + privileged: bool, > + _get_privileged_command: Callable[[str], str], > + app_args: str = "", > + timeout: float = SETTINGS.timeout, > + ) -> None: We can simplify the constructor signature a bit. We can combine privileged and _get_privileged_command (which shouldn't start with the underscore - only private class attributes should have that) in that get_privileged_command could be None (which would be the same as privileged=False and not None the same as privileged=True). We don't need startup_command as we're just passing self.path to it, which we already have access to. There is an alternative approach to the path manipulation logic - we don't modify shell_cls.path at all and pass the resulting path (the result of all the operations we do in Node objects) to the constructor (in this case we'd retain the startup_command parameter, although it should be then renamed to path_app or something similar). This doesn't move the logic entirely out of InteractiveShell though, as we still need to access the class attribute. For that reason, I don't see much of a point in pursuing this alternative - we'd basically be just passing one extra unneeded path argument through all the calls. > + self._interactive_session = interactive_session > + self._ssh_channel = self._interactive_session.invoke_shell() > + self._stdin = self._ssh_channel.makefile_stdin("w") > + self._stdout = self._ssh_channel.makefile("r") > + self._ssh_channel.settimeout(timeout) > + self._ssh_channel.set_combine_stderr(True) # combines stdout and > stderr streams > + self._logger = logger > + self._timeout = timeout > + self._startup_command = startup_command > + self._app_args = app_args > + self._get_privileged_command = _get_privileged_command # type: > ignore We can actually make this work with a static method: 1. Don't store the method in this object. We'd need to pass it to _start_application, but this should be ok as we're only using the method when starting the application - once it's running, we'll never need it again. 2. When we stored the method, we were passing the self parameter when calling self._get_privileged_command, which is why it couldn't be static. When calling without self, static works. Note: the staticmethod decorator must be above the abstractmethod decorator, otherwise the interpreter throws an error. 3. The type can be either Callable[[str], str] or MethodType (from the types module). I'd go with callable here as it's more precise. > + self._privileged = privileged > + self._start_application() > + > + def _start_application(self) -> None: > + """Starts a new interactive application based on _startup_command. > + > + This method is often overridden by subclasses as their process for > + starting may look different. > + """ > + start_command = f"{self._startup_command} {self._app_args}" > + if self._privileged: > + start_command = self._get_privileged_command(start_command) # > type: ignore > + 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. > + > + Lines that expect input are not included in the stdout buffer so > they cannot be My IDE spell checker says there should be a comma before so. > + 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 work around for this could be > consuming an Workaround without the space. > + extra newline character to force the current prompt into the stdout > buffer. > + > + Returns: > + All output in the buffer before expected string > + """ > + self._logger.info(f"Sending command {command.strip()}...") > + if prompt is None: > + prompt = self._default_prompt > + self._stdin.write(f"{command}{self._command_extra_chars}\n") > + self._stdin.flush() > + out: str = "" > + for line in self._stdout: > + out += line > + if prompt in line and not line.rstrip().endswith( > + command.rstrip() > + ): # ignore line that sent command > + break > + self._logger.debug(f"Got output: {out}") > + return out > + > + def close(self) -> None: > + self._stdin.close() > + self._ssh_channel.close() > + > + def __del__(self) -> None: > + self.close() <snip> > diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py > index 743919820c..724e7328eb 100644 > --- a/dts/framework/test_result.py > +++ b/dts/framework/test_result.py <snip> > @@ -213,6 +218,12 @@ def __init__(self, build_target: > BuildTargetConfiguration): > self.os = build_target.os > self.cpu = build_target.cpu > self.compiler = build_target.compiler > + self.compiler_version = None > + self.dpdk_version = None > + > + def add_build_target_versions(self, versions: BuildTargetInfo) -> None: Just noticed the name - rename to add_build_target_info to align with everything else. > + self.compiler_version = versions.compiler_version > + self.dpdk_version = versions.dpdk_version > > def add_test_suite(self, test_suite_name: str) -> TestSuiteResult: > test_suite_result = TestSuiteResult(test_suite_name) > diff --git a/dts/framework/testbed_model/node.py > b/dts/framework/testbed_model/node.py > index d48fafe65d..39237a95d6 100644 > --- a/dts/framework/testbed_model/node.py > +++ b/dts/framework/testbed_model/node.py > @@ -7,7 +7,7 @@ > A node is a generic host that DTS connects to and manages. > """ > > -from typing import Any, Callable > +from typing import Any, Callable, Type > > from framework.config import ( > BuildTargetConfiguration, > @@ -15,7 +15,7 @@ > NodeConfiguration, > ) > from framework.logger import DTSLOG, getLogger > -from framework.remote_session import OSSession, create_session > +from framework.remote_session import InteractiveShellType, OSSession, > create_session > from framework.settings import SETTINGS > > from .hw import ( > @@ -23,6 +23,7 @@ > LogicalCoreCount, > LogicalCoreList, > LogicalCoreListFilter, > + VirtualDevice, > lcore_filter, > ) > > @@ -40,6 +41,8 @@ class Node(object): > lcores: list[LogicalCore] > _logger: DTSLOG > _other_sessions: list[OSSession] > + _execution_config: ExecutionConfiguration > + _virtual_devices: list[VirtualDevice] > > def __init__(self, node_config: NodeConfiguration): > self.config = node_config > @@ -54,6 +57,7 @@ def __init__(self, node_config: NodeConfiguration): > ).filter() > > self._other_sessions = [] > + self._virtual_devices = [] This is not a private attribute, so let's drop the starting underscore. > > self._logger.info(f"Created node: {self.name}") > > @@ -64,6 +68,9 @@ def set_up_execution(self, execution_config: > ExecutionConfiguration) -> None: > """ > self._setup_hugepages() > self._set_up_execution(execution_config) > + self._execution_config = execution_config > + for vdev in execution_config.vdevs: > + self._virtual_devices.append(VirtualDevice(vdev)) > > def _set_up_execution(self, execution_config: ExecutionConfiguration) -> > None: > """ > @@ -77,6 +84,7 @@ def tear_down_execution(self) -> None: > this node is part of concludes. > """ > self._tear_down_execution() > + self._virtual_devices = [] I'd prefer this to be above self._tear_down_execution(). If there's a failure in self._tear_down_execution(), the devices won't be removed so removing them before the call is safer. > > def _tear_down_execution(self) -> None: > """ <snip> > diff --git a/dts/tests/TestSuite_smoke_tests.py > b/dts/tests/TestSuite_smoke_tests.py > new file mode 100644 > index 0000000000..83712e75c6 > --- /dev/null > +++ b/dts/tests/TestSuite_smoke_tests.py > @@ -0,0 +1,114 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2023 University of New Hampshire > + > +import re > + > +from framework.config import PortConfig > +from framework.remote_session import TestPmdShell > +from framework.settings import SETTINGS > +from framework.test_suite import TestSuite > +from framework.utils import REGEX_FOR_PCI_ADDRESS > + > + > +class SmokeTests(TestSuite): > + is_blocking = True > + # dicts in this list are expected to have two keys: > + # "pci_address" and "current_driver" > + nics_in_node: list[PortConfig] = [] > + > + def set_up_suite(self) -> None: > + """ > + Setup: > + Set the build directory path and generate a list of NICs in the > SUT node. > + """ > + self.dpdk_build_dir_path = self.sut_node.remote_dpdk_build_dir > + self.nics_in_node = self.sut_node.config.ports > + > + def test_unit_tests(self) -> None: > + """ > + Test: > + Run the fast-test unit-test suite through meson. > + """ > + self.sut_node.main_session.send_command( > + f"meson test -C {self.dpdk_build_dir_path} --suite fast-tests -t > 60", > + 480, > + verify=True, > + privileged=True, > + ) > + > + def test_driver_tests(self) -> None: > + """ > + Test: > + Run the driver-test unit-test suite through meson. > + """ > + vdev_args = "" > + for dev in self.sut_node._virtual_devices: > + vdev_args += f"--vdev {dev} " > + vdev_args = vdev_args[:-1] > + driver_tests_command = ( > + f"meson test -C {self.dpdk_build_dir_path} --suite driver-tests" > + ) > + if vdev_args: > + self._logger.info( > + "Running driver tests with the following virtual " > + f"devices: {vdev_args}" > + ) > + driver_tests_command += f' --test-args "{vdev_args}"' > + > + self.sut_node.main_session.send_command( > + driver_tests_command, > + 300, > + verify=True, > + privileged=True, > + ) > + > + def test_devices_listed_in_testpmd(self) -> None: > + """ > + Test: > + Uses testpmd driver to verify that devices have been found by > testpmd. > + """ > + testpmd_driver = self.sut_node.create_interactive_shell( > + TestPmdShell, privileged=True > + ) > + dev_list: list[str] = [str(x) for x in testpmd_driver.get_devices()] The type hint is not necessary, at least according to my mypy. We can keep it, but we don't do this anywhere else, so I'd go for consistency. > + for nic in self.nics_in_node: > + self.verify( > + nic.pci in dev_list, > + f"Device {nic.pci} was not listed in testpmd's available > devices, " > + "please check your configuration", > + ) > + > + def test_device_bound_to_driver(self) -> None: > + """ > + Test: > + Ensure that all drivers listed in the config are bound to the > correct > + driver. > + """ > + path_to_devbind = self.sut_node.main_session.join_remote_path( > + self.sut_node._remote_dpdk_dir, "usertools", "dpdk-devbind.py" > + ) > + > + all_nics_in_dpdk_devbind = self.sut_node.main_session.send_command( > + f"{path_to_devbind} --status | awk '{REGEX_FOR_PCI_ADDRESS}'", > + SETTINGS.timeout, > + ).stdout > + > + for nic in self.nics_in_node: > + # This regular expression finds the line in the above string > that starts > + # with the address for the nic we are on in the loop and then > captures the > + # name of the driver in a group > + devbind_info_for_nic = re.search( > + f"{nic.pci}[^\\n]*drv=([\\d\\w]*) [^\\n]*", > + all_nics_in_dpdk_devbind, > + ) > + self.verify( > + devbind_info_for_nic is not None, > + f"Failed to find configured device ({nic.pci}) using > dpdk-devbind.py", > + ) > + # We know this isn't None, but mypy doesn't > + assert devbind_info_for_nic is not None > + self.verify( > + devbind_info_for_nic.group(1) == nic.os_driver, > + f"Driver for device {nic.pci} does not match driver listed > in " > + f"configuration (bound to {devbind_info_for_nic.group(1)})", > + ) > -- > 2.41.0 >
