Re: [PATCH] dts: add l2fwd test suite

[Luca Vizzarro <Luca.Vizzarro@arm.com>]

On 09/08/2024 16:27, Jeremy Spewock wrote:
>> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
>> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
>> ---
>> Depends-on: series-32714 ("dts: add pktgen and testpmd changes")
> 
> Out of my own curiosity, are depends on supposed to be outside of the
> commit body? I don't think it really matters for automation or
> anything regardless, but I just didn't know if there was a rule about
> it.

Depends-on tags are metadata for Patchwork, they don't belong in the 
repository. So there shouldn't be any depends-on committed as it 
wouldn't make any sense in Git. My reply aside, the contributing 
guidelines also specify how to do it[1].

>> new file mode 100644
>> index 0000000000..46f07b78eb
>> --- /dev/null
>> +++ b/dts/tests/TestSuite_l2fwd.py
>> @@ -0,0 +1,58 @@
> 
> This file looks like it is missing the copyright information at the top.

Argh! Good catch, thank you!

>> +"""Basic L2 forwarding test suite.
>> +
>> +This testing suites runs basic L2 forwarding on testpmd with different queue sizes per port.
> 
> The phrasing of "different queue sizes per port" makes me initially
> think that like, port 0 will have 2 queues and port 1 will have 4.
> Maybe something like "This testing suites runs basic L2 forwarding on
> testpmd across multiple different queue sizes" would make this more
> clear.
> 

Ack.

>> +    def set_up_suite(self) -> None:
>> +        """Set up the test suite.
>> +
>> +        Setup:
>> +            Verify that we have at least 2 ports in the current test. Generate the random packets
>> +            that will be sent and spawn a reusable testpmd shell.
> 
> Seems like this method is no longer spawning a testpmd shell, so this
> part of the doc-string is no longer relevant.

Ack. Remnants of an earlier version... :')

>> +    def test_l2fwd_integrity(self) -> None:
>> +        """Test the L2 forwarding integrity.
>> +
>> +        Test:
>> +            Configure a testpmd shell with a different numbers of queues per run. Start up L2
> 
> It might make sense to name the numbers of queues in the doc-string
> just so that the rst for the suite is more clear.

Ack.

>> +                expected_packets = [self.get_expected_packet(packet) for packet in self.packets]
> 
> Ahh, the get_expected_packet method also sheds some light on how the
> match_all_packets could be useful.
> 

:)

[1] https://doc.dpdk.org/guides/contributing/patches.html#patch-dependencies