From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 97E6542EA6; Tue, 18 Jul 2023 11:56:02 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D9C0E410D3; Tue, 18 Jul 2023 11:56:01 +0200 (CEST) Received: from mail-ed1-f51.google.com (mail-ed1-f51.google.com [209.85.208.51]) by mails.dpdk.org (Postfix) with ESMTP id 1FBF740A84 for ; Tue, 18 Jul 2023 11:56:01 +0200 (CEST) Received: by mail-ed1-f51.google.com with SMTP id 4fb4d7f45d1cf-51e28cac164so13853473a12.1 for ; Tue, 18 Jul 2023 02:56:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1689674160; x=1690278960; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=NetDHrOBvFjGHKsOq9EMjxEZuDBhOycwa+cuQm+I3KY=; b=vBlV6Ppb00a6SXiojZSTEQkvqSSYCjssz4EbpBWg+YFyjbFOYu5Jn9eknlcKb+WPs3 4767Yj7euFRKfMWEZlA1wio0cvLaEoSF6ldNn78iB2c5OiC3WtGBei3kP928ZgE9fNww Fp/BRxSeh7vXxOEceTFwmz2eo7h/qCvwPzp6Qjz9hDqTuF23PlAo7H0D5mwF5pDkje22 N2hdstqBDU2GYghA8vCfWT8+p/k37f2hOGHDZGb4LPRbYCqyUSWMe8Csu6KvYpGCz/I0 gqM0E48QWeoZ3iSROBDRegAgB39fvI31a+lgERVbZZO1Md33wiDXQ0ISzjP010s2yMed j+dg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689674160; x=1690278960; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=NetDHrOBvFjGHKsOq9EMjxEZuDBhOycwa+cuQm+I3KY=; b=RVUbSluBqC8sXE6jjSbc/oM64N4qpB4hR/WYXdNIDLV3riwo/zrCoEInTKHhOVhHaS an01mInzN4AgNF7VPjHRLskL2YqYz0oGCWiKooDQnuWBK8UMKK9d1IwTGd1ZUdvImZR0 XNduXzfR5Dl3lRK977C2rcvp3wewdkqaNFa31mjRLkyxGk3u7uQ6Yr1CBrujdxFyrv+P MnaaULxZgF4KSTgO2/3sBOvPpOi8R3Cgo4XoRTV3sNSbjwCGqDavzC8MEDt5tSW3oiZ4 y1ZjK6j/FFOpcZfPB1ynn3WQYJRZFvkU9mHyS1/LellDDpTQjt4SHGCZunJbym8CzHTq qvqw== X-Gm-Message-State: ABy/qLZNVRtatQ0dJpMY4pDIPipvFqvggx+2NPfK+Sf7VKCOmrT8BeCs /gj50P/vDg/jUKGxQ3hIbCRrp+3VJDk+wzJ4ND3dZg== X-Google-Smtp-Source: APBJJlGo/9RriAstShT72OronIEBqNyMW0XJ3CAx/I1cUC6CAGYjwc4aXiMxTtN6u7WP6Up+yX5wwIQLlQflQSk4HlI= X-Received: by 2002:aa7:d683:0:b0:521:29a:8ee3 with SMTP id d3-20020aa7d683000000b00521029a8ee3mr14044425edr.5.1689674160428; Tue, 18 Jul 2023 02:56:00 -0700 (PDT) MIME-Version: 1.0 References: <20230713165347.21997-2-jspewock@iol.unh.edu> <20230713165347.21997-3-jspewock@iol.unh.edu> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Tue, 18 Jul 2023 11:55:49 +0200 Message-ID: Subject: Re: [PATCH v7 1/1] dts: add smoke tests To: Jeremy Spewock Cc: Honnappa.Nagarahalli@arm.com, thomas@monjalon.net, lijuan.tu@intel.com, wathsala.vithanage@arm.com, probb@iol.unh.edu, dev@dpdk.org Content-Type: multipart/mixed; boundary="0000000000001e82650600bfec3b" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --0000000000001e82650600bfec3b Content-Type: multipart/alternative; boundary="0000000000001e82640600bfec38" --0000000000001e82640600bfec38 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Jul 17, 2023 at 9:31=E2=80=AFPM Jeremy Spewock wrote: > > > On Mon, Jul 17, 2023 at 10:50=E2=80=AFAM Juraj Linke=C5=A1 > wrote: > >> I found additional things while working with the interactive shell code. >> >> On Thu, Jul 13, 2023 at 6:54=E2=80=AFPM wrote: >> > >> > From: Jeremy Spewock >> > >> > 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 >> > --- >> >> >> >> > diff --git a/dts/framework/config/conf_yaml_schema.json >> b/dts/framework/config/conf_yaml_schema.json >> > index ca2d4a1ef2..61f52b4365 100644 >> > --- a/dts/framework/config/conf_yaml_schema.json >> > +++ b/dts/framework/config/conf_yaml_schema.json >> >> > @@ -211,8 +332,27 @@ >> > ] >> > } >> > }, >> > + "skip_smoke_tests": { >> > + "description": "Optional field that allows you to skip >> smoke testing", >> > + "type": "boolean" >> > + }, >> > "system_under_test": { >> > - "$ref": "#/definitions/node_name" >> > + "type":"object", >> > + "properties": { >> > + "node_name": { >> > + "$ref": "#/definitions/node_name" >> > + }, >> > + "vdevs": { >> > + "description": "Opentional list of names of vdevs to >> be used in execution", >> >> Typo in Opentional >> >> > + "type": "array", >> > + "items": { >> > + "type": "string" >> > + } >> > + } >> > + }, >> > + "required": [ >> > + "node_name" >> > + ] >> > } >> > }, >> > "additionalProperties": false, >> > diff --git a/dts/framework/dts.py b/dts/framework/dts.py >> > index 0502284580..7b09d8fba8 100644 >> > --- a/dts/framework/dts.py >> > +++ b/dts/framework/dts.py >> >> > @@ -118,14 +124,15 @@ def _run_build_target( >> > >> > try: >> > sut_node.set_up_build_target(build_target) >> > - result.dpdk_version =3D sut_node.dpdk_version >> > + # result.dpdk_version =3D sut_node.dpdk_version >> > + >> build_target_result.add_build_target_versions(sut_node.get_build_target_= info()) >> >> The additions to execution_result and build_target_result are >> inconsistent - one modifies __init__, the other doesn't. The time at >> which we have the info available is different, which is the reason for >> the inconsistency, but I'd rather make all results classes as >> consistent as possible - create from config, add other info after >> that. >> >> > build_target_result.update_setup(Result.PASS) >> > except Exception as e: >> > dts_logger.exception("Build target setup failed.") >> > build_target_result.update_setup(Result.FAIL, e) >> > >> > else: >> > - _run_suites(sut_node, execution, build_target_result) >> > + _run_all_suites(sut_node, execution, build_target_result) >> > >> > finally: >> > try: >> >> >> >> > diff --git a/dts/framework/remote_session/remote/__init__.py >> b/dts/framework/remote_session/remote/__init__.py >> > index 8a1512210a..03fd309f2b 100644 >> > --- a/dts/framework/remote_session/remote/__init__.py >> > +++ b/dts/framework/remote_session/remote/__init__.py >> > @@ -1,16 +1,26 @@ >> > # SPDX-License-Identifier: BSD-3-Clause >> > # Copyright(c) 2023 PANTHEON.tech s.r.o. >> > +# Copyright(c) 2023 University of New Hampshire >> > >> > # pylama:ignore=3DW0611 >> > >> > from framework.config import NodeConfiguration >> > from framework.logger import DTSLOG >> > >> > +from .interactive_remote_session import InteractiveRemoteSession >> > +from .interactive_shell import InteractiveShell >> > from .remote_session import CommandResult, RemoteSession >> > from .ssh_session import SSHSession >> > +from .testpmd_shell import TestPmdDevice, TestPmdShell >> > >> > >> > def create_remote_session( >> > node_config: NodeConfiguration, name: str, logger: DTSLOG >> > ) -> RemoteSession: >> > return SSHSession(node_config, name, logger) >> > + >> > + >> > +def create_interactive_session( >> > + node_config: NodeConfiguration, name: str, logger: DTSLOG >> > +) -> InteractiveRemoteSession: >> >> The name parameter is not used, remove it please. >> >> > + return InteractiveRemoteSession(node_config, logger) >> >> >> >> > diff --git a/dts/framework/remote_session/remote/testpmd_shell.py >> b/dts/framework/remote_session/remote/testpmd_shell.py >> > new file mode 100644 >> > index 0000000000..c0261c00f6 >> > --- /dev/null >> > +++ b/dts/framework/remote_session/remote/testpmd_shell.py >> > @@ -0,0 +1,74 @@ >> > +# SPDX-License-Identifier: BSD-3-Clause >> > +# Copyright(c) 2023 University of New Hampshire >> > + >> > +from pathlib import PurePath >> > + >> > +from paramiko import SSHClient # type: ignore >> > + >> > +from framework.logger import DTSLOG >> > +from framework.settings import SETTINGS >> > + >> > +from .interactive_shell import InteractiveShell >> > + >> > + >> > +class TestPmdDevice(object): >> > + pci_address: str >> > + >> > + def __init__(self, pci_address: str): >> > + self.pci_address =3D pci_address >> > + >> > + def __str__(self) -> str: >> > + return self.pci_address >> > + >> > + >> > +class TestPmdShell(InteractiveShell): >> > + expected_prompt: str =3D "testpmd>" >> > + _eal_flags: str >> > + >> > + def __init__( >> > + self, >> > + interactive_session: SSHClient, >> > + logger: DTSLOG, >> > + path_to_testpmd: PurePath, >> > + eal_flags: str, >> > + timeout: float =3D SETTINGS.timeout, >> > + ) -> None: >> > + """Initializes an interactive testpmd session using specified >> parameters.""" >> > + self._eal_flags =3D eal_flags >> > + >> > + super(TestPmdShell, self).__init__( >> > + interactive_session, >> > + logger=3Dlogger, >> > + path_to_app=3Dpath_to_testpmd, >> > + timeout=3Dtimeout, >> > + ) >> > + >> > + def _start_application(self) -> None: >> > + """Starts a new interactive testpmd shell using >> _path_to_app.""" >> > + self.send_command( >> > + f"{self._path_to_app} {self._eal_flags} -- -i", >> > + ) >> > + >> > + def send_command(self, command: str, prompt: str =3D >> expected_prompt) -> str: >> > + """Specific way of handling the command for testpmd >> > + >> > + An extra newline character is consumed in order to force the >> current line into >> > + the stdout buffer. >> > + """ >> > + return self.send_command_get_output(f"{command}\n", prompt) >> > + >> >> We don't really need two methods with different names which basically >> do the same - both send a command and get output. >> We could use super() to modify the method in subclasses. Or we could >> introduce a new class attribute storing the command suffix, similarly >> to expected_prompt. >> >> A note on the class attributes: we should properly document them in >> InteractiveShell so that it's clear what should be considered to be >> overridden in derived classes. >> >> > + def get_devices(self) -> list[TestPmdDevice]: >> > + """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. >> > + >> > + Returns: >> > + A list of strings representing device names (e.g. >> 0000:14:00.1) >> > + """ >> > + dev_info: str =3D self.send_command("show device info all") >> > + dev_list: list[TestPmdDevice] =3D [] >> > + for line in dev_info.split("\n"): >> > + if "device name:" in line.lower(): >> > + dev_list.append(TestPmdDevice(line.strip().split(": >> ")[1].strip())) >> >> We should pass the full line to TestPmdDevice and process the line in >> the class. This splits the logic properly - the full line is needed to >> get other information about the device. >> >> > + return dev_list >> > diff --git a/dts/framework/test_result.py b/dts/framework/test_result.= py >> > index 743919820c..fe1994673e 100644 >> > --- a/dts/framework/test_result.py >> > +++ b/dts/framework/test_result.py >> >> > @@ -267,14 +288,17 @@ class DTSResult(BaseResult): >> > def __init__(self, logger: DTSLOG): >> > super(DTSResult, self).__init__() >> > self.dpdk_version =3D None >> > + self.output =3D None >> > self._logger =3D logger >> > self._errors =3D [] >> > self._return_code =3D ErrorSeverity.NO_ERR >> > self._stats_result =3D None >> > self._stats_filename =3D os.path.join(SETTINGS.output_dir, >> "statistics.txt") >> > >> > - def add_execution(self, sut_node: NodeConfiguration) -> >> ExecutionResult: >> > - execution_result =3D ExecutionResult(sut_node) >> > + def add_execution( >> > + self, sut_node: NodeConfiguration, sut_version_info: NodeInfo >> > + ) -> ExecutionResult: >> > + execution_result =3D ExecutionResult(sut_node, sut_version_in= fo) >> > self._inner_results.append(execution_result) >> > return execution_result >> > >> > @@ -296,7 +320,8 @@ def process(self) -> None: >> > for error in self._errors: >> > self._logger.debug(repr(error)) >> > >> > - self._stats_result =3D Statistics(self.dpdk_version) >> > + self._stats_result =3D Statistics(self.output) >> >> By changing this (and the semi-related line 121 in dts.py) we've >> changed the behavior a bit. We've talked about removing the part which >> modifies self.output (or something similar) and these (and other >> related parts of the code) look like the remnants of that change. >> Let's remove these as well. >> >> > + # add information gathered from the smoke tests to the >> statistics >> > self.add_stats(self._stats_result) >> > with open(self._stats_filename, "w+") as stats_file: >> > stats_file.write(str(self._stats_result)) >> >> >> >> > diff --git a/dts/tests/TestSuite_smoke_tests.py >> b/dts/tests/TestSuite_smoke_tests.py >> > new file mode 100644 >> > index 0000000000..9cf547205f >> > --- /dev/null >> > +++ b/dts/tests/TestSuite_smoke_tests.py >> > @@ -0,0 +1,113 @@ >> > +# SPDX-License-Identifier: BSD-3-Clause >> > +# Copyright(c) 2023 University of New Hampshire >> > + >> > +import re >> > + >> > +from framework.config import InteractiveApp, PortConfig >> > +from framework.remote_session import TestPmdDevice, 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 =3D True >> > + # dicts in this list are expected to have two keys: >> > + # "pci_address" and "current_driver" >> > + nics_in_node: list[PortConfig] =3D [] >> > + >> > + 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 =3D self.sut_node.remote_dpdk_build_= dir >> > + self.nics_in_node =3D 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", >> > + 300, >> > + verify=3DTrue, >> > + ) >> >> Just a note: we can just add privileged=3DTrue to these calls to make >> then run as root. >> >> > + >> > + def test_driver_tests(self) -> None: >> > + """ >> > + Test: >> > + Run the driver-test unit-test suite through meson. >> > + """ >> > + list_of_vdevs =3D "" >> >> This looks more like a string of vdev args :-). I'd rename it to >> vdev_args. >> >> > + for dev in self.sut_node._execution_config.vdevs: >> >> The test suite should be working with sut_node objects, not >> configuration. We should create the VirtualDevice objects from >> _execution_config.vdevs in _set_up_execution and then only work with >> those. And maybe reset the list in _tear_down_execution. >> >> > + list_of_vdevs +=3D f"--vdev {dev} " >> > + list_of_vdevs =3D list_of_vdevs[:-1] >> > + if list_of_vdevs: >> > + self._logger.info( >> > + "Running driver tests with the following virtual " >> > + f"devices: {list_of_vdevs}" >> > + ) >> > + self.sut_node.main_session.send_command( >> > + f"meson test -C {self.dpdk_build_dir_path} --suite >> driver-tests " >> > + f'--test-args "{list_of_vdevs}"', >> > + 300, >> > + verify=3DTrue, >> > + ) >> > + else: >> > + self.sut_node.main_session.send_command( >> > + f"meson test -C {self.dpdk_build_dir_path} --suite >> driver-tests", >> > + 300, >> > + verify=3DTrue, >> > + ) >> > + >> > + def test_devices_listed_in_testpmd(self) -> None: >> > + """ >> > + Test: >> > + Uses testpmd driver to verify that devices have been foun= d >> by testpmd. >> > + """ >> > + testpmd_driver =3D >> self.sut_node.create_interactive_shell(InteractiveApp.testpmd) >> > + # We know it should always be a TestPmdShell but mypy doesn't >> > + assert isinstance(testpmd_driver, TestPmdShell) >> > + dev_list: list[TestPmdDevice] =3D testpmd_driver.get_devices(= ) >> > + for nic in self.nics_in_node: >> > + self.verify( >> > + nic.pci in map(str, dev_list), >> >> map(str, dev_list) gets called for every nic. I'm more inclined toward >> composing the list right when assigning to dev_list. Or you could look >> into returning (from get_devices()) a derived List overloading the >> __contains__ method :-) >> >> > + 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 =3D self.sut_node.main_session.join_remote_pa= th( >> > + self.sut_node._remote_dpdk_dir, "usertools", >> "dpdk-devbind.py" >> > + ) >> > + >> > + all_nics_in_dpdk_devbind =3D >> 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 =3D re.search( >> > + f"{nic.pci}[^\\n]*drv=3D([\\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) =3D=3D 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 >> > >> > > Thanks for the comments, I went through and addressed them. As we talked > about today, I did also add parts from your patch in regards to the > interactive shells and I added the ability for them to use sudo. We had > talked about using a static method for this, but this would not work > because what type of session you are using is an important distinction. I > could not pass the entire OSSession class into the interactive shell, so = I > instead added just a reference to the method for accessing elevated > privileges. I had to add a few # type: ignore lines for this to work due = to > the issue marked here: https://github.com/python/mypy/issues/2427 . > Please see my new version. > I have some more comments on the new version, the main being that we can actually pass a static method (from the proper object thus from the proper class). I think that makes more sense since the method should've been static from the get go. I've explained a bit in the comments and I'm attaching a diff here. --0000000000001e82640600bfec38 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Mon, Jul 17, 2023 at 9:31=E2=80=AF= PM Jeremy Spewock <jspewock@iol.= unh.edu> wrote:


On Mon, Jul 17, 2023 at 10:50=E2=80=AFAM Juraj Linke=C5= =A1 <juraj.linkes@pantheon.tech> wrote:
I found additional things while working with = the interactive shell code.

On Thu, Jul 13, 2023 at 6:54=E2=80=AFPM <jspewock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@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<= br> > fail, the DTS execution terminates as part of a "fail-fast" = model.
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---

<snip>

> diff --git a/dts/framework/config/conf_yaml_schema.json b/dts/framewor= k/config/conf_yaml_schema.json
> index ca2d4a1ef2..61f52b4365 100644
> --- a/dts/framework/config/conf_yaml_schema.json
> +++ b/dts/framework/config/conf_yaml_schema.json
<snip>
> @@ -211,8 +332,27 @@
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ]
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 },
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "skip_smoke_tests": { > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "description": &q= uot;Optional field that allows you to skip smoke testing",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "type": "boo= lean"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 },
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "system_under_test"= : {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "$ref": "#/d= efinitions/node_name"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "type":"obje= ct",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "properties": { > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "node_name"= ;: {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "$ref&qu= ot;: "#/definitions/node_name"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 },
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "vdevs": {=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "descrip= tion": "Opentional list of names of vdevs to be used in execution= ",

Typo in Opentional

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "type&qu= ot;: "array",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "items&q= uot;: {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "= type": "string"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 },
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "required": [
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "node_name"= ;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ]
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 },
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "additionalProperties": fa= lse,
> diff --git a/dts/framework/dts.py b/dts/framework/dts.py
> index 0502284580..7b09d8fba8 100644
> --- a/dts/framework/dts.py
> +++ b/dts/framework/dts.py
<snip>
> @@ -118,14 +124,15 @@ def _run_build_target(
>
>=C2=A0 =C2=A0 =C2=A0 try:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sut_node.set_up_build_target(build_t= arget)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 result.dpdk_version =3D sut_node.dpdk_ver= sion
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # result.dpdk_version =3D sut_node.dpdk_v= ersion
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 build_target_result.add_build_target_vers= ions(sut_node.get_build_target_info())

The additions to execution_result and build_target_result are
inconsistent - one modifies __init__, the other doesn't. The time at which we have the info available is different, which is the reason for
the inconsistency, but I'd rather make all results classes as
consistent as possible - create from config, add other info after
that.

>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 build_target_result.update_setup(Res= ult.PASS)
>=C2=A0 =C2=A0 =C2=A0 except Exception as e:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dts_logger.exception("Build tar= get setup failed.")
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 build_target_result.update_setup(Res= ult.FAIL, e)
>
>=C2=A0 =C2=A0 =C2=A0 else:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 _run_suites(sut_node, execution, build_ta= rget_result)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 _run_all_suites(sut_node, execution, buil= d_target_result)
>
>=C2=A0 =C2=A0 =C2=A0 finally:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 try:

<snip>

> diff --git a/dts/framework/remote_session/remote/__init__.py b/dts/fra= mework/remote_session/remote/__init__.py
> index 8a1512210a..03fd309f2b 100644
> --- a/dts/framework/remote_session/remote/__init__.py
> +++ b/dts/framework/remote_session/remote/__init__.py
> @@ -1,16 +1,26 @@
>=C2=A0 # SPDX-License-Identifier: BSD-3-Clause
>=C2=A0 # Copyright(c) 2023 PANTHEON.tech s.r.o.
> +# Copyright(c) 2023 University of New Hampshire
>
>=C2=A0 # pylama:ignore=3DW0611
>
>=C2=A0 from framework.config import NodeConfiguration
>=C2=A0 from framework.logger import DTSLOG
>
> +from .interactive_remote_session import InteractiveRemoteSession
> +from .interactive_shell import InteractiveShell
>=C2=A0 from .remote_session import CommandResult, RemoteSession
>=C2=A0 from .ssh_session import SSHSession
> +from .testpmd_shell import TestPmdDevice, TestPmdShell
>
>
>=C2=A0 def create_remote_session(
>=C2=A0 =C2=A0 =C2=A0 node_config: NodeConfiguration, name: str, logger:= DTSLOG
>=C2=A0 ) -> RemoteSession:
>=C2=A0 =C2=A0 =C2=A0 return SSHSession(node_config, name, logger)
> +
> +
> +def create_interactive_session(
> +=C2=A0 =C2=A0 node_config: NodeConfiguration, name: str, logger: DTSL= OG
> +) -> InteractiveRemoteSession:

The name parameter is not used, remove it please.

> +=C2=A0 =C2=A0 return InteractiveRemoteSession(node_config, logger)
<snip>

> diff --git a/dts/framework/remote_session/remote/testpmd_shell.py b/dt= s/framework/remote_session/remote/testpmd_shell.py
> new file mode 100644
> index 0000000000..c0261c00f6
> --- /dev/null
> +++ b/dts/framework/remote_session/remote/testpmd_shell.py
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 University of New Hampshire
> +
> +from pathlib import PurePath
> +
> +from paramiko import SSHClient=C2=A0 # type: ignore
> +
> +from framework.logger import DTSLOG
> +from framework.settings import SETTINGS
> +
> +from .interactive_shell import InteractiveShell
> +
> +
> +class TestPmdDevice(object):
> +=C2=A0 =C2=A0 pci_address: str
> +
> +=C2=A0 =C2=A0 def __init__(self, pci_address: str):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.pci_address =3D pci_address
> +
> +=C2=A0 =C2=A0 def __str__(self) -> str:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return self.pci_address
> +
> +
> +class TestPmdShell(InteractiveShell):
> +=C2=A0 =C2=A0 expected_prompt: str =3D "testpmd>"
> +=C2=A0 =C2=A0 _eal_flags: str
> +
> +=C2=A0 =C2=A0 def __init__(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 interactive_session: SSHClient,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 logger: DTSLOG,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 path_to_testpmd: PurePath,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 eal_flags: str,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 timeout: float =3D SETTINGS.timeout,
> +=C2=A0 =C2=A0 ) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Initializes an interact= ive testpmd session using specified parameters."""
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._eal_flags =3D eal_flags
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 super(TestPmdShell, self).__init__(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 interactive_session,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 logger=3Dlogger,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 path_to_app=3Dpath_to_testp= md,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timeout=3Dtimeout,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +
> +=C2=A0 =C2=A0 def _start_application(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Starts a new interactiv= e testpmd shell using _path_to_app."""
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.send_command(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"{self._path_to_app} = {self._eal_flags} -- -i",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +
> +=C2=A0 =C2=A0 def send_command(self, command: str, prompt: str =3D ex= pected_prompt) -> str:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Specific way of handlin= g the command for testpmd
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 An extra newline character is consumed in= order to force the current line into
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 the stdout buffer.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return self.send_command_get_output(f&quo= t;{command}\n", prompt)
> +

We don't really need two methods with different names which basically do the same - both send a command and get output.
We could use super() to modify the method in subclasses. Or we could
introduce a new class attribute storing the command suffix, similarly
to expected_prompt.

A note on the class attributes: we should properly document them in
InteractiveShell so that it's clear what should be considered to be
overridden in derived classes.

> +=C2=A0 =C2=A0 def get_devices(self) -> list[TestPmdDevice]:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Get a list of device na= mes that are known to testpmd
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Uses the device info listed in testpmd an= d then parses the output to
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return only the names of the devices.
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Returns:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 A list of strings represent= ing device names (e.g. 0000:14:00.1)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_info: str =3D self.send_command("= ;show device info all")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_list: list[TestPmdDevice] =3D []
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for line in dev_info.split("\n"= ):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if "device name:"= in line.lower():
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_list.appe= nd(TestPmdDevice(line.strip().split(": ")[1].strip()))

We should pass the full line to TestPmdDevice and process the line in
the class. This splits the logic properly - the full line is needed to
get other information about the device.

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return dev_list
> diff --git a/dts/framework/test_result.py b/dts/framework/test_result.= py
> index 743919820c..fe1994673e 100644
> --- a/dts/framework/test_result.py
> +++ b/dts/framework/test_result.py
<snip>
> @@ -267,14 +288,17 @@ class DTSResult(BaseResult):
>=C2=A0 =C2=A0 =C2=A0 def __init__(self, logger: DTSLOG):
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super(DTSResult, self).__init__() >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.dpdk_version =3D None
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.output =3D None
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._logger =3D logger
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._errors =3D []
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._return_code =3D ErrorSeverity.= NO_ERR
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stats_result =3D None
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stats_filename =3D os.path.joi= n(SETTINGS.output_dir, "statistics.txt")
>
> -=C2=A0 =C2=A0 def add_execution(self, sut_node: NodeConfiguration) -&= gt; ExecutionResult:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 execution_result =3D ExecutionResult(sut_= node)
> +=C2=A0 =C2=A0 def add_execution(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self, sut_node: NodeConfiguration, sut_ve= rsion_info: NodeInfo
> +=C2=A0 =C2=A0 ) -> ExecutionResult:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 execution_result =3D ExecutionResult(sut_= node, sut_version_info)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._inner_results.append(execution= _result)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return execution_result
>
> @@ -296,7 +320,8 @@ def process(self) -> None:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for error in self._err= ors:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._lo= gger.debug(repr(error))
>
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stats_result =3D Statistics(self.dp= dk_version)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stats_result =3D Statistics(self.ou= tput)

By changing this (and the semi-related line 121 in dts.py) we've
changed the behavior a bit. We've talked about removing the part which<= br> modifies self.output (or something similar) and these (and other
related parts of the code) look like the remnants of that change.
Let's remove these as well.

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # add information gathered from the smoke= tests to the statistics
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.add_stats(self._stats_result) >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 with open(self._stats_filename, &quo= t;w+") as stats_file:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats_file.write(str(s= elf._stats_result))

<snip>

> diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_= smoke_tests.py
> new file mode 100644
> index 0000000000..9cf547205f
> --- /dev/null
> +++ b/dts/tests/TestSuite_smoke_tests.py
> @@ -0,0 +1,113 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 University of New Hampshire
> +
> +import re
> +
> +from framework.config import InteractiveApp, PortConfig
> +from framework.remote_session import TestPmdDevice, TestPmdShell
> +from framework.settings import SETTINGS
> +from framework.test_suite import TestSuite
> +from framework.utils import REGEX_FOR_PCI_ADDRESS
> +
> +
> +class SmokeTests(TestSuite):
> +=C2=A0 =C2=A0 is_blocking =3D True
> +=C2=A0 =C2=A0 # dicts in this list are expected to have two keys:
> +=C2=A0 =C2=A0 # "pci_address" and "current_driver"= ;
> +=C2=A0 =C2=A0 nics_in_node: list[PortConfig] =3D []
> +
> +=C2=A0 =C2=A0 def set_up_suite(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Setup:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Set the build directory pat= h and generate a list of NICs in the SUT node.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.dpdk_build_dir_path =3D self.sut_nod= e.remote_dpdk_build_dir
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.nics_in_node =3D self.sut_node.confi= g.ports
> +
> +=C2=A0 =C2=A0 def test_unit_tests(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Test:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Run the fast-test unit-test= suite through meson.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sut_node.main_session.send_command(<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"meson test -C {self.= dpdk_build_dir_path} --suite fast-tests",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 300,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 verify=3DTrue,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 )

Just a note: we can just add privileged=3DTrue to these calls to make
then run as root.

> +
> +=C2=A0 =C2=A0 def test_driver_tests(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Test:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Run the driver-test unit-te= st suite through meson.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 list_of_vdevs =3D ""

This looks more like a string of vdev args :-). I'd rename it to vdev_a= rgs.

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for dev in self.sut_node._execution_confi= g.vdevs:

The test suite should be working with sut_node objects, not
configuration. We should create the VirtualDevice objects from
_execution_config.vdevs in _set_up_execution and then only work with
those. And maybe reset the list in _tear_down_execution.

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 list_of_vdevs +=3D f"-= -vdev {dev} "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 list_of_vdevs =3D list_of_vdevs[:-1]
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if list_of_vdevs:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._logger.info(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "Running= driver tests with the following virtual "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"device= s: {list_of_vdevs}"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sut_node.main_session.= send_command(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"meson = test -C {self.dpdk_build_dir_path} --suite driver-tests "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f'--test-= args "{list_of_vdevs}"',
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 300,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 verify=3DTrue= ,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 else:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sut_node.main_session.= send_command(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"meson = test -C {self.dpdk_build_dir_path} --suite driver-tests",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 300,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 verify=3DTrue= ,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +
> +=C2=A0 =C2=A0 def test_devices_listed_in_testpmd(self) -> None: > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Test:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Uses testpmd driver to veri= fy that devices have been found by testpmd.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 testpmd_driver =3D self.sut_node.create_i= nteractive_shell(InteractiveApp.testpmd)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # We know it should always be a TestPmdSh= ell but mypy doesn't
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 assert isinstance(testpmd_driver, TestPmd= Shell)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_list: list[TestPmdDevice] =3D testpmd= _driver.get_devices()
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for nic in self.nics_in_node:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.verify(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 nic.pci in ma= p(str, dev_list),

map(str, dev_list) gets called for every nic. I'm more inclined toward<= br> composing the list right when assigning to dev_list. Or you could look
into returning (from get_devices()) a derived List overloading the
__contains__ method :-)

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"Device= {nic.pci} was not listed in testpmd's available devices, "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "please = check your configuration",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +
> +=C2=A0 =C2=A0 def test_device_bound_to_driver(self) -> None:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Test:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Ensure that all drivers lis= ted in the config are bound to the correct driver.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 path_to_devbind =3D self.sut_node.main_se= ssion.join_remote_path(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sut_node._remote_dpdk_= dir, "usertools", "dpdk-devbind.py"
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 all_nics_in_dpdk_devbind =3D self.sut_nod= e.main_session.send_command(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"{path_to_devbind} --= status | awk '{REGEX_FOR_PCI_ADDRESS}'",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 SETTINGS.timeout,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 ).stdout
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 for nic in self.nics_in_node:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # This regular expression f= inds the line in the above string that starts
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # with the address for the = nic we are on in the loop and then captures the
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # name of the driver in a g= roup
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 devbind_info_for_nic =3D re= .search(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"{nic.p= ci}[^\\n]*drv=3D([\\d\\w]*) [^\\n]*",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 all_nics_in_d= pdk_devbind,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.verify(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 devbind_info_= for_nic is not None,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"Failed= to find configured device ({nic.pci}) using dpdk-devbind.py",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # We know this isn't No= ne, but mypy doesn't
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 assert devbind_info_for_nic= is not None
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.verify(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 devbind_info_= for_nic.group(1) =3D=3D nic.os_driver,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"Driver= for device {nic.pci} does not match driver listed in "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"config= uration (bound to {devbind_info_for_nic.group(1)})",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> --
> 2.41.0
>

Thanks for the comments, I went through and addressed them. As we talk= ed about today, I did also add parts from your patch in regards to the inte= ractive shells and I added the ability for them to use sudo. We had talked = about using a static method for this, but this would not work because what = type of session you are using is an important distinction. I could not pass= the entire OSSession class into the interactive shell, so I instead added = just a reference to the method for accessing elevated privileges. I had to = add a few # type: ignore lines for this to work due to the issue marked her= e: https://github.com/python/mypy/issues/2427 . Please see my new version= .

I have some more co= mments on the new version, the main being that we can actually pass a stati= c method (from the proper object thus from the proper class). I think that = makes more sense since the method should've been static from the get go= . I've explained a bit in the comments and I'm attaching a diff her= e.
=C2=A0
--0000000000001e82640600bfec38-- --0000000000001e82650600bfec3b Content-Type: text/x-patch; charset="US-ASCII"; name="privileged_staticmethod.patch" Content-Disposition: attachment; filename="privileged_staticmethod.patch" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_lk84c1io0 ZGlmZiAtLWdpdCBhL2R0cy9mcmFtZXdvcmsvcmVtb3RlX3Nlc3Npb24vbGludXhfc2Vzc2lvbi5w eSBiL2R0cy9mcmFtZXdvcmsvcmVtb3RlX3Nlc3Npb24vbGludXhfc2Vzc2lvbi5weQppbmRleCBm MTNmMzk5MTIxLi5mNjRhYThlZmIwIDEwMDY0NAotLS0gYS9kdHMvZnJhbWV3b3JrL3JlbW90ZV9z ZXNzaW9uL2xpbnV4X3Nlc3Npb24ucHkKKysrIGIvZHRzL2ZyYW1ld29yay9yZW1vdGVfc2Vzc2lv bi9saW51eF9zZXNzaW9uLnB5CkBAIC0xNCw3ICsxNCw4IEBAIGNsYXNzIExpbnV4U2Vzc2lvbihQ b3NpeFNlc3Npb24pOgogICAgIFRoZSBpbXBsZW1lbnRhdGlvbiBvZiBub24tUG9zaXggY29tcGxp YW50IHBhcnRzIG9mIExpbnV4IHJlbW90ZSBzZXNzaW9ucy4KICAgICAiIiIKIAotICAgIGRlZiBf Z2V0X3ByaXZpbGVnZWRfY29tbWFuZChzZWxmLCBjb21tYW5kOiBzdHIpIC0+IHN0cjoKKyAgICBA c3RhdGljbWV0aG9kCisgICAgZGVmIF9nZXRfcHJpdmlsZWdlZF9jb21tYW5kKGNvbW1hbmQ6IHN0 cikgLT4gc3RyOgogICAgICAgICByZXR1cm4gZiJzdWRvIC0tIHNoIC1jICd7Y29tbWFuZH0nIgog CiAgICAgZGVmIGdldF9yZW1vdGVfY3B1cyhzZWxmLCB1c2VfZmlyc3RfY29yZTogYm9vbCkgLT4g bGlzdFtMb2dpY2FsQ29yZV06CmRpZmYgLS1naXQgYS9kdHMvZnJhbWV3b3JrL3JlbW90ZV9zZXNz aW9uL29zX3Nlc3Npb24ucHkgYi9kdHMvZnJhbWV3b3JrL3JlbW90ZV9zZXNzaW9uL29zX3Nlc3Np b24ucHkKaW5kZXggZjAzMjc1YjIxZC4uZGFkOGQ0ZGExYiAxMDA2NDQKLS0tIGEvZHRzL2ZyYW1l d29yay9yZW1vdGVfc2Vzc2lvbi9vc19zZXNzaW9uLnB5CisrKyBiL2R0cy9mcmFtZXdvcmsvcmVt b3RlX3Nlc3Npb24vb3Nfc2Vzc2lvbi5weQpAQCAtMTAwLDggKzEwMCw5IEBAIGRlZiBjcmVhdGVf aW50ZXJhY3RpdmVfc2hlbGwoCiAgICAgICAgICAgICB0aW1lb3V0LAogICAgICAgICApCiAKKyAg ICBAc3RhdGljbWV0aG9kCiAgICAgQGFic3RyYWN0bWV0aG9kCi0gICAgZGVmIF9nZXRfcHJpdmls ZWdlZF9jb21tYW5kKHNlbGYsIGNvbW1hbmQ6IHN0cikgLT4gc3RyOgorICAgIGRlZiBfZ2V0X3By aXZpbGVnZWRfY29tbWFuZChjb21tYW5kOiBzdHIpIC0+IHN0cjoKICAgICAgICAgIiIiTW9kaWZ5 IHRoZSBjb21tYW5kIHNvIHRoYXQgaXQgZXhlY3V0ZXMgd2l0aCBhZG1pbmlzdHJhdGl2ZSBwcml2 aWxlZ2VzLgogCiAgICAgICAgIEFyZ3M6CmRpZmYgLS1naXQgYS9kdHMvZnJhbWV3b3JrL3JlbW90 ZV9zZXNzaW9uL3JlbW90ZS9pbnRlcmFjdGl2ZV9zaGVsbC5weSBiL2R0cy9mcmFtZXdvcmsvcmVt b3RlX3Nlc3Npb24vcmVtb3RlL2ludGVyYWN0aXZlX3NoZWxsLnB5CmluZGV4IDRkOWM3NjM4YTUu LjM1MTJmMWY4YTkgMTAwNjQ0Ci0tLSBhL2R0cy9mcmFtZXdvcmsvcmVtb3RlX3Nlc3Npb24vcmVt b3RlL2ludGVyYWN0aXZlX3NoZWxsLnB5CisrKyBiL2R0cy9mcmFtZXdvcmsvcmVtb3RlX3Nlc3Np b24vcmVtb3RlL2ludGVyYWN0aXZlX3NoZWxsLnB5CkBAIC0yMiw3ICsyMiw2IEBAIGNsYXNzIElu dGVyYWN0aXZlU2hlbGw6CiAgICAgX2FwcF9hcmdzOiBzdHIKICAgICBfZGVmYXVsdF9wcm9tcHQ6 IHN0ciA9ICIiCiAgICAgX3ByaXZpbGVnZWQ6IGJvb2wKLSAgICBfZ2V0X3ByaXZpbGVnZWRfY29t bWFuZDogQ2FsbGFibGVbW3N0cl0sIHN0cl0KICAgICAjIEFsbG93cyBmb3IgYXBwIHNwZWNpZmlj IGV4dHJhIGNoYXJhY3RlcnMgdG8gYmUgYXBwZW5kZWQgdG8gY29tbWFuZHMKICAgICBfY29tbWFu ZF9leHRyYV9jaGFyczogc3RyID0gIiIKICAgICBwYXRoOiBQdXJlUGF0aApAQCAtMzQsNyArMzMs NyBAQCBkZWYgX19pbml0X18oCiAgICAgICAgIGxvZ2dlcjogRFRTTE9HLAogICAgICAgICBzdGFy dHVwX2NvbW1hbmQ6IHN0ciwKICAgICAgICAgcHJpdmlsZWdlZDogYm9vbCwKLSAgICAgICAgX2dl dF9wcml2aWxlZ2VkX2NvbW1hbmQ6IENhbGxhYmxlW1tzdHJdLCBzdHJdLAorICAgICAgICBnZXRf cHJpdmlsZWdlZF9jb21tYW5kOiBDYWxsYWJsZVtbc3RyXSwgc3RyXSwKICAgICAgICAgYXBwX2Fy Z3M6IHN0ciA9ICIiLAogICAgICAgICB0aW1lb3V0OiBmbG9hdCA9IFNFVFRJTkdTLnRpbWVvdXQs CiAgICAgKSAtPiBOb25lOgpAQCAtNDgsMTEgKzQ3LDEwIEBAIGRlZiBfX2luaXRfXygKICAgICAg ICAgc2VsZi5fdGltZW91dCA9IHRpbWVvdXQKICAgICAgICAgc2VsZi5fc3RhcnR1cF9jb21tYW5k ID0gc3RhcnR1cF9jb21tYW5kCiAgICAgICAgIHNlbGYuX2FwcF9hcmdzID0gYXBwX2FyZ3MKLSAg ICAgICAgc2VsZi5fZ2V0X3ByaXZpbGVnZWRfY29tbWFuZCA9IF9nZXRfcHJpdmlsZWdlZF9jb21t YW5kICAjIHR5cGU6IGlnbm9yZQogICAgICAgICBzZWxmLl9wcml2aWxlZ2VkID0gcHJpdmlsZWdl ZAotICAgICAgICBzZWxmLl9zdGFydF9hcHBsaWNhdGlvbigpCisgICAgICAgIHNlbGYuX3N0YXJ0 X2FwcGxpY2F0aW9uKGdldF9wcml2aWxlZ2VkX2NvbW1hbmQpCiAKLSAgICBkZWYgX3N0YXJ0X2Fw cGxpY2F0aW9uKHNlbGYpIC0+IE5vbmU6CisgICAgZGVmIF9zdGFydF9hcHBsaWNhdGlvbihzZWxm LCBnZXRfcHJpdmlsZWdlZF9jb21tYW5kOiBDYWxsYWJsZVtbc3RyXSwgc3RyXSkgLT4gTm9uZToK ICAgICAgICAgIiIiU3RhcnRzIGEgbmV3IGludGVyYWN0aXZlIGFwcGxpY2F0aW9uIGJhc2VkIG9u IF9zdGFydHVwX2NvbW1hbmQuCiAKICAgICAgICAgVGhpcyBtZXRob2QgaXMgb2Z0ZW4gb3ZlcnJp ZGRlbiBieSBzdWJjbGFzc2VzIGFzIHRoZWlyIHByb2Nlc3MgZm9yCkBAIC02MCw3ICs1OCw3IEBA IGRlZiBfc3RhcnRfYXBwbGljYXRpb24oc2VsZikgLT4gTm9uZToKICAgICAgICAgIiIiCiAgICAg ICAgIHN0YXJ0X2NvbW1hbmQgPSBmIntzZWxmLl9zdGFydHVwX2NvbW1hbmR9IHtzZWxmLl9hcHBf YXJnc30iCiAgICAgICAgIGlmIHNlbGYuX3ByaXZpbGVnZWQ6Ci0gICAgICAgICAgICBzdGFydF9j b21tYW5kID0gc2VsZi5fZ2V0X3ByaXZpbGVnZWRfY29tbWFuZChzdGFydF9jb21tYW5kKSAgIyB0 eXBlOiBpZ25vcmUKKyAgICAgICAgICAgIHN0YXJ0X2NvbW1hbmQgPSBnZXRfcHJpdmlsZWdlZF9j b21tYW5kKHN0YXJ0X2NvbW1hbmQpCiAgICAgICAgIHNlbGYuc2VuZF9jb21tYW5kKHN0YXJ0X2Nv bW1hbmQpCiAKICAgICBkZWYgc2VuZF9jb21tYW5kKHNlbGYsIGNvbW1hbmQ6IHN0ciwgcHJvbXB0 OiBzdHIgfCBOb25lID0gTm9uZSkgLT4gc3RyOgpkaWZmIC0tZ2l0IGEvZHRzL2ZyYW1ld29yay9y ZW1vdGVfc2Vzc2lvbi9yZW1vdGUvdGVzdHBtZF9zaGVsbC5weSBiL2R0cy9mcmFtZXdvcmsvcmVt b3RlX3Nlc3Npb24vcmVtb3RlL3Rlc3RwbWRfc2hlbGwucHkKaW5kZXggNGFiZTcwZDQyMS4uYWY4 YWMyYTA1NyAxMDA2NDQKLS0tIGEvZHRzL2ZyYW1ld29yay9yZW1vdGVfc2Vzc2lvbi9yZW1vdGUv dGVzdHBtZF9zaGVsbC5weQorKysgYi9kdHMvZnJhbWV3b3JrL3JlbW90ZV9zZXNzaW9uL3JlbW90 ZS90ZXN0cG1kX3NoZWxsLnB5CkBAIC0yLDYgKzIsNyBAQAogIyBDb3B5cmlnaHQoYykgMjAyMyBV bml2ZXJzaXR5IG9mIE5ldyBIYW1wc2hpcmUKIAogZnJvbSBwYXRobGliIGltcG9ydCBQdXJlUGF0 aAorZnJvbSB0eXBpbmcgaW1wb3J0IENhbGxhYmxlCiAKIGZyb20gLmludGVyYWN0aXZlX3NoZWxs IGltcG9ydCBJbnRlcmFjdGl2ZVNoZWxsCiAKQEAgLTI0LDEwICsyNSwxMCBAQCBjbGFzcyBUZXN0 UG1kU2hlbGwoSW50ZXJhY3RpdmVTaGVsbCk6CiAgICAgICAgICJcbiIgICMgV2Ugd2FudCB0byBh cHBlbmQgYW4gZXh0cmEgbmV3bGluZSB0byBldmVyeSBjb21tYW5kCiAgICAgKQogCi0gICAgZGVm IF9zdGFydF9hcHBsaWNhdGlvbihzZWxmKSAtPiBOb25lOgorICAgIGRlZiBfc3RhcnRfYXBwbGlj YXRpb24oc2VsZiwgZ2V0X3ByaXZpbGVnZWRfY29tbWFuZDogQ2FsbGFibGVbW3N0cl0sIHN0cl0p IC0+IE5vbmU6CiAgICAgICAgICIiIlNlZSAiX3N0YXJ0X2FwcGxpY2F0aW9uIiBpbiBJbnRlcmFj dGl2ZVNoZWxsLiIiIgogICAgICAgICBzZWxmLl9hcHBfYXJncyArPSAiIC0tIC1pIgotICAgICAg ICBzdXBlcigpLl9zdGFydF9hcHBsaWNhdGlvbigpCisgICAgICAgIHN1cGVyKCkuX3N0YXJ0X2Fw cGxpY2F0aW9uKGdldF9wcml2aWxlZ2VkX2NvbW1hbmQpCiAKICAgICBkZWYgZ2V0X2RldmljZXMo c2VsZikgLT4gbGlzdFtUZXN0UG1kRGV2aWNlXToKICAgICAgICAgIiIiR2V0IGEgbGlzdCBvZiBk ZXZpY2UgbmFtZXMgdGhhdCBhcmUga25vd24gdG8gdGVzdHBtZAo= --0000000000001e82650600bfec3b--