Re: [PATCH 2/3] dts: add testpmd methods for test suite

[Nicholas Pratte <npratte@iol.unh.edu>]

On Wed, Jun 26, 2024 at 11:50 AM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> I think the subject line of this commit makes sense in the context of
> this series, but might be less helpful when the commit is applied to
> the git tree. For this reason I might favor changing it to something
> that briefly says what the added testpmd commands actually do.
>
> On Fri, Jun 21, 2024 at 1:23 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
> >
> > Several methods are either imported from currently in-develoment test
> > suites, or they have been created specifically for this test suite.
> >
> > Methods here that can be found in other test suites include vlan
> > filtering, adding and removing vlan tags, and setting promiscuous mode.
> >
> > Methods that are specific to this test suite include adding or removing
> > mac addresses, and adding or removing multicast mac addresses.
>
> You probably could make the subject about the adding/modification of
> mac addresses judging by the body.
>

Good point. I'll make adjustments to the commit subject as well as the
commit messages.

> >
> > Bugzilla ID: 1454
> > Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
> > ---
> >  dts/framework/remote_session/testpmd_shell.py | 209 ++++++++++++++++++
> >  1 file changed, 209 insertions(+)
> >
> > diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> > index ec22f72221..8a7d5905b3 100644
> > --- a/dts/framework/remote_session/testpmd_shell.py
> > +++ b/dts/framework/remote_session/testpmd_shell.py
> > @@ -767,6 +767,215 @@ def show_port_info(self, port_id: int) -> TestPmdPort:
> >
> >          return TestPmdPort.parse(output)
> >
> > +    def set_mac_addr(self, port_id: int, mac_address: str, add: bool = True, verify: bool = True):
>
> I'm not sure it makes sense to default the `add` parameter in this
> method or the following. It seems to me like this method would likely
> be split pretty evenly between adding and removing and I can't think
> of a reason why one would be done in the general case over the other,
> so I think it would be more clear if we didn't default it.
>

Good point. It made sense for me to do it this way in a vacuum (since
I'm mostly adding addresses in each test case), but there's no good
reason it should be this way.

> > +        """Add or remove a mac address on a given port.
> > +
>
> Is it worth mentioning that we are adding the MAC address to the
> allowlist of the port?

Yes, I made a subtle change to the docstring.

>
> > +        Args:
> > +            port_id: The port ID the mac address is set on.
> > +            mac_address: The mac address to be added or removed to the specified port.
> > +            add: If :data:`True`, add the specified mac address. If :data:`False`, remove specified
> > +                mac address.
> > +            verify: If :data:'True', assert that the 'mac_addr' operation was successful. If
> > +                :data:'False', run the command and skip this assertion.
> > +
> > +        Raises:
> > +            InteractiveCommandExecutionError: If either the 'add' or 'remove' operations fail.
>
> This error documentation confuses me a little because it sounds like
> both `add` and `remove` operations are happening in this method when
> really either one happens or the other happens. Maybe changing this to
> just saying the "mac address operation" failed would fix this.
>

Agreed. I made the change.

> > +        """
>
> I think something that could save you a lot of space in this method
> (and the next one) is creating a variable for what command to use if
> `add` is true or not.
> You could do something like this:
>
> mac_cmd = "add" if add else "remove"
>
> > +        if add:
> > +            output = self.send_command(f"mac_addr add {port_id} {mac_address}")
> > +        else:
> > +            output = self.send_command(f"mac_addr remove {port_id} {mac_address}")
>
> Then you don't need this if-statement because this just becomes:
> output = self.send_command(f"mac_addr {mac_cmd} {port_id} {mac_address}")
>

I love this! Good thinking. I'll make the changes.

<snip>
> > +            ):
> > +                self._logger.debug(f"Failed to add {multi_addr} on port {port_id}")
> > +                raise InteractiveCommandExecutionError(
> > +                    f"Failed to add {multi_addr} on port {port_id} \n{output}"
> > +                )
> > +            if (
> > +                not add
> > +                and "Invalid multicast addr"
>
> Is this a typo or does testpmd remove the underscore when you're removing?

Not a typo. Good attention to detail though.

<snip>
> > +
> > +    def rx_vlan_rm(self, vlan: int, port: int, verify: bool = True):
> > +        """Remove specified vlan tag from filter list on a port.
> > +
> > +        Args:
> > +            vlan: The vlan tag to remove, should be within 1-4094.
> > +            port: The port number to remove the tag from, should be within 0-32.
> > +            verify: If :data:`True`, the output of the command is scanned to verify that
> > +                the vlan tag was removed from the filter list on the specified port. If not, it is
> > +                considered an error.
> > +
> > +        Raises:
> > +            InteractiveCommandExecutionError: If `verify` is :data:`True` and the tag
> > +            is not removed.
>
> Other methods have this second line indented. I'm not sure if I
> addressed this on the VLAN suite already or not but this should
> probably do the same.

All of the remaining methods were ripped from the VLAN suite, and I'll
update these as that series progresses. I've been doing it this way to
hopefully make it less confusing come the time these patches series
are ready to be merged.