Re: [PATCH v5 1/3] dts: updated testpmd shell class

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

Just documentation comments, the structure of the code and the
functionality look good to me.

Maybe a different subject for the commit would be more descriptive.
Something like "add methods required for VLAN test suite to
TestpmdShell". Then you can explain what methods are added in the
description.

On Tue, Jun 18, 2024 at 12:30 PM Dean Marx <dmarx@iol.unh.edu> wrote:
>
> Ported over the promisc and verbose mode functions
> from v2 of the queue start/stop suite to use for the
> VLAN suite. Tweaked some of the verification methods to be
> more concise, changed some docstrings to be more specific.

The descriptions here should be the commit message explaining
everything that is being added, not just the diff from the previous
version. This is generally because this will end up being the
description of the commit once the final version gets merged, so the
people looking at the git history will only see the description of the
final version and have no context for what was added in the previous
versions. The same for the subject line of the commit.

>
> Signed-off-by: Dean Marx <dmarx@iol.unh.edu>
> ---

If you want to have a change log associated with the commit however,
you can put comments under these 3 hyphens that won't get included in
the final commit message.

>  dts/framework/remote_session/testpmd_shell.py | 260 ++++++++++++++++++
>  1 file changed, 260 insertions(+)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index cb2ab6bd00..aad3a3a448 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -225,6 +225,266 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
>                  f"Test pmd failed to set fwd mode to {mode.value}"
>              )
>
> +    def vlan_filter_set_on(self, port: int, verify: bool = True):
> +        """Set vlan filter on.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.

A comment on this doc-string and the one in the other vlan filter method:
This might be a little more descriptive if you explained how the port
number is being used. Maybe saying something like "number of the port
to add the filter to". Or, alternatively, you could change the first
line of this comment to say "Enable VLAN filtering for port with id
`port`". If you see the code it's clear how this port is being used,
but if you were only looking at the documentation there isn't much
information about how this port number comes into play.

> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan filtering was enabled successfully. If not, it is
> +                considered an error.

This looks like a case where an additional command is sent rather than
the original output being scanned after the update.

> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
> +        filter_cmd_output = self.send_command(f"vlan set filter on {port}")
> +        if verify:
> +            if "Invalid port" in filter_cmd_output or "filter: on" not in self.send_command(f"show port info {port}"):
> +                self._logger.debug(f"Failed to enable vlan filter on port {port}: \n{filter_cmd_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to enable vlan filter on port {port}.")
> +
> +    def vlan_filter_set_off(self, port: int, verify: bool = True):
> +        """Set vlan filter off.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan filtering was disabled successfully. If not, it is
> +                considered an error.

Here it seems like it's not the output of the command being sent, but
another command being sent to verify.

> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the filter
> +                fails to update.
> +        """
<snip>
> +    def vlan_strip_set_on(self, port: int, verify: bool = True):
> +        """Enable vlan stripping on the specified port.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32.
> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan stripping was enabled on the specified port. If not, it is
> +                considered an error.

Same thing here.

> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and stripping
> +                fails to update.
> +        """
> +        vlan_strip_output = self.send_command(f"vlan set strip on {port}")
> +        if verify:
> +            if "strip on" not in self.send_command(f"show port info {port}"):
> +                self._logger.debug(f"Failed to set vlan filter on for port {port}: \n{vlan_strip_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to set vlan filter on for port {port}.")
> +
> +    def vlan_strip_set_off(self, port: int, verify: bool = True):
> +        """Disable vlan stripping on the specified port.
> +
> +        Args:
> +            port: The port number to use, should be within 0-32

This arg is missing a period at the end of the line.

> +            verify: If :data:`True`, the output of the command is scanned to verify that
> +                vlan stripping was disabled on the specified port. If not, it is
> +                considered an error.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and stripping
> +                fails to update.
> +        """
> +        vlan_strip_output = self.send_command(f"vlan set strip off {port}")
> +        if verify:
> +            if "strip off" not in self.send_command(f"show port info {port}"):
> +                self._logger.debug(f"Failed to disable vlan stripping on port {port}: \n{vlan_strip_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to disable vlan stripping on port {port}.")
> +
> +    def port_stop_all(self, verify: bool = True):
> +        """Stop all ports.
> +

This doc-string is missing the Args section for the verify parameter.

> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and all ports
> +                fail to stop."""
> +        port_output = self.send_command("port stop all")
> +        if verify:
> +            if "Done" not in port_output:
> +                self._logger.debug(f"Failed to stop all ports: \n{port_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to stop all ports.")
> +
> +    def port_stop(self, port: int, verify: bool = True):
> +        """Stop all ports.

This seems like a copy paste error from the previous method.

> +
> +        Args:
> +            port: specifies the port number to use, must be between 0-32

verify is missing from this Args section. There is also a missing
period at the end of the line.

> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port
> +                is not stopped."""
> +        port_output = self.send_command(f"port stop {port}")
> +        if verify:
> +            if "Done" not in port_output:
> +                self._logger.debug(f"Failed to stop port {port}: \n{port_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to stop port {port}.")
> +
> +    def port_start_all(self, verify: bool = True):
> +        """Start all ports.
> +

This method is also missing an Args section.

> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and all ports
> +            fail to start."""
> +        port_output = self.send_command("port start all")
> +        if verify:
> +            if "Done" not in port_output:
> +                self._logger.debug(f"Failed to start all ports: \n{port_output}")
> +                raise InteractiveCommandExecutionError("Testpmd failed to start all ports.")
> +
> +    def port_start(self, port: int, verify: bool = True):
> +        """Stop all ports.

Seems like a copy-paste error here.

> +
> +        Args:
> +            port: specifies the port number to use, must be between 0-32

Missing period at the end of the line here. The args section also
seems to be missing the verify parameter.

> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the port
> +                is not started."""
<snip>
> +    def set_promisc(self, port: int, on: bool, verify: bool = True):
> +        """Turns promiscuous mode on/off for the specified port

Missing period at the end of the line here.

> +
> +        Args:
> +            port: port number to use, should be within 0-32.
> +            on: if :data:`True`, turn promisc mode on, otherwise turn off.
> +            verify: if :data:`True` an additional command will be sent to verify that promisc mode
> +                is properly set. Defaults to :data:`True`.
> +
> +        Raises:
<snip>
> +
> +    def set_verbose(self, level: int, verify: bool = True):
> +        """Set debug verbosity level.
> +
> +        Args:
> +            level: 0 - silent except for error
> +            1 - fully verbose except for Tx packets
> +            2 - fully verbose except for Rx packets
> +            >2 - fully verbose

These make sense, but it might help to indent the last 3 just so it's
clear what they apply to.

> +            verify: if :data:`True` an additional command will be sent to verify that verbose level
> +                is properly set. Defaults to :data:`True`.

This method doesn't seem to send another command, it just verifies
from the output of the first.

> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `verify` is :data:`True` and verbose level
> +            is not correctly set.
> +        """
> +        verbose_output = self.send_command(f"set verbose {level}")
> +        if verify:
> +            if "Change verbose level" not in verbose_output:
> +                self._logger.debug(f"Failed to set verbose level to {level}: \n{verbose_output}")
> +                raise InteractiveCommandExecutionError(f"Testpmd failed to set verbose level to {level}.")
> +
>      def close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.send_command("quit", "")
> --
> 2.44.0
>