[4/6] dts: use testpmd params for scatter test suite

  Update the buffer scatter test suite to use TestPmdParameters
instead of the StrParams implementation.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 dts/tests/TestSuite_pmd_buffer_scatter.py | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)
  

On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Update the buffer scatter test suite to use TestPmdParameters
> instead of the StrParams implementation.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  dts/tests/TestSuite_pmd_buffer_scatter.py | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
> index 4cdbdc4272..c6d313fc64 100644
> --- a/dts/tests/TestSuite_pmd_buffer_scatter.py
> +++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
> @@ -23,7 +23,11 @@
>  from scapy.utils import hexstr  # type: ignore[import]
>
>  from framework.params import StrParams
> -from framework.remote_session.testpmd_shell import TestPmdForwardingModes, TestPmdShell
> +from framework.remote_session.testpmd_shell import (
> +    TestPmdForwardingModes,
> +    TestPmdShell,
> +    TestPmdParameters,
> +)
>  from framework.test_suite import TestSuite
>
>
> @@ -104,16 +108,15 @@ def pmd_scatter(self, mbsize: int) -> None:
>          """
>          testpmd = self.sut_node.create_interactive_shell(
>              TestPmdShell,
> -            app_parameters=StrParams(
> -                "--mbcache=200 "
> -                f"--mbuf-size={mbsize} "
> -                "--max-pkt-len=9000 "
> -                "--port-topology=paired "
> -                "--tx-offloads=0x00008000"
> +            app_parameters=TestPmdParameters(
> +                forward_mode=TestPmdForwardingModes.mac,
> +                mbcache=200,
> +                mbuf_size=[mbsize],
> +                max_pkt_len=9000,
> +                tx_offloads=0x00008000,
>              ),
>              privileged=True,
>          )
> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)

Jeremy, does this change the test? Instead of configuring the fw mode
after starting testpmd, we're starting testpmd with fw mode
configured.

If not, we should remove the testpmd.set_forward_mode method, as it's
not used anymore.

>          testpmd.start()
>
>          for offset in [-1, 0, 1, 4, 5]:
> --
> 2.34.1
>
  

On 09/04/2024 20:12, Juraj Linkeš wrote:
>> @@ -104,16 +108,15 @@ def pmd_scatter(self, mbsize: int) -> None:
>>           """
>>           testpmd = self.sut_node.create_interactive_shell(
>>               TestPmdShell,
>> -            app_parameters=StrParams(
>> -                "--mbcache=200 "
>> -                f"--mbuf-size={mbsize} "
>> -                "--max-pkt-len=9000 "
>> -                "--port-topology=paired "
>> -                "--tx-offloads=0x00008000"
>> +            app_parameters=TestPmdParameters(
>> +                forward_mode=TestPmdForwardingModes.mac,
>> +                mbcache=200,
>> +                mbuf_size=[mbsize],
>> +                max_pkt_len=9000,
>> +                tx_offloads=0x00008000,
>>               ),
>>               privileged=True,
>>           )
>> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> 
> Jeremy, does this change the test? Instead of configuring the fw mode
> after starting testpmd, we're starting testpmd with fw mode
> configured.

I am not Jeremy (please Jeremy still reply), but we discussed this on 
Slack. Reading through the testpmd source code, setting arguments like 
forward-mode in the command line, is the exact equivalent of calling 
`set forward mode` right after start-up. So it is equivalent in theory.

> If not, we should remove the testpmd.set_forward_mode method, as it's
> not used anymore.

Could there be test cases that change the forward mode multiple times in 
the same shell, though? As this could still be needed to cover this.
  

On Wed, Apr 10, 2024 at 12:53 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 09/04/2024 20:12, Juraj Linkeš wrote:
> >> @@ -104,16 +108,15 @@ def pmd_scatter(self, mbsize: int) -> None:
> >>           """
> >>           testpmd = self.sut_node.create_interactive_shell(
> >>               TestPmdShell,
> >> -            app_parameters=StrParams(
> >> -                "--mbcache=200 "
> >> -                f"--mbuf-size={mbsize} "
> >> -                "--max-pkt-len=9000 "
> >> -                "--port-topology=paired "
> >> -                "--tx-offloads=0x00008000"
> >> +            app_parameters=TestPmdParameters(
> >> +                forward_mode=TestPmdForwardingModes.mac,
> >> +                mbcache=200,
> >> +                mbuf_size=[mbsize],
> >> +                max_pkt_len=9000,
> >> +                tx_offloads=0x00008000,
> >>               ),
> >>               privileged=True,
> >>           )
> >> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> >
> > Jeremy, does this change the test? Instead of configuring the fw mode
> > after starting testpmd, we're starting testpmd with fw mode
> > configured.
>
> I am not Jeremy (please Jeremy still reply), but we discussed this on
> Slack. Reading through the testpmd source code, setting arguments like
> forward-mode in the command line, is the exact equivalent of calling
> `set forward mode` right after start-up. So it is equivalent in theory.
>
> > If not, we should remove the testpmd.set_forward_mode method, as it's
> > not used anymore.
>
> Could there be test cases that change the forward mode multiple times in
> the same shell, though? As this could still be needed to cover this.

Yes, but we don't have such a test now. It's good practice to remove
unused code. We can still bring it back anytime, it'll be in git
history.
  

On Wed, Apr 10, 2024 at 9:19 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> On Wed, Apr 10, 2024 at 12:53 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
> >
> > On 09/04/2024 20:12, Juraj Linkeš wrote:
> > >> @@ -104,16 +108,15 @@ def pmd_scatter(self, mbsize: int) -> None:
> > >>           """
> > >>           testpmd = self.sut_node.create_interactive_shell(
> > >>               TestPmdShell,
> > >> -            app_parameters=StrParams(
> > >> -                "--mbcache=200 "
> > >> -                f"--mbuf-size={mbsize} "
> > >> -                "--max-pkt-len=9000 "
> > >> -                "--port-topology=paired "
> > >> -                "--tx-offloads=0x00008000"
> > >> +            app_parameters=TestPmdParameters(
> > >> +                forward_mode=TestPmdForwardingModes.mac,
> > >> +                mbcache=200,
> > >> +                mbuf_size=[mbsize],
> > >> +                max_pkt_len=9000,
> > >> +                tx_offloads=0x00008000,
> > >>               ),
> > >>               privileged=True,
> > >>           )
> > >> -        testpmd.set_forward_mode(TestPmdForwardingModes.mac)
> > >
> > > Jeremy, does this change the test? Instead of configuring the fw mode
> > > after starting testpmd, we're starting testpmd with fw mode
> > > configured.

To my knowledge, as Luca mentions below, this does not functionally
change anything about the test, scatter should just need the MAC
forwarding mode to be set at some point before forwarding starts, it
doesn't technically matter when. One thing to note that this does
change, however, is that we lose the verification step that the method
within testpmd provides. I'm not sure off the top of my head if
testpmd just completely fails to start if the forwarding mode flag is
set and it fails to change modes or if it still starts and then just
goes back to default (io) which would make the test operate in an
invalid state without anyway of knowing.

As another note however, I've never seen a mode change fail and I
don't know what could even make it fail, so this would be a rare thing
anyway, but still just something to consider.


> >
> > I am not Jeremy (please Jeremy still reply), but we discussed this on
> > Slack. Reading through the testpmd source code, setting arguments like
> > forward-mode in the command line, is the exact equivalent of calling
> > `set forward mode` right after start-up. So it is equivalent in theory.
> >
> > > If not, we should remove the testpmd.set_forward_mode method, as it's
> > > not used anymore.
> >
> > Could there be test cases that change the forward mode multiple times in
> > the same shell, though? As this could still be needed to cover this.
>
> Yes, but we don't have such a test now. It's good practice to remove
> unused code. We can still bring it back anytime, it'll be in git
> history.