[v2,1/5] dts: add ability to send/receive multiple packets

The framework allows only to send one packet at once via Scapy. This
change adds the ability to send multiple packets, and also introduces a
new fast way to verify if we received several expected packets.

Moreover, it reduces code duplication by keeping a single packet sending
method only at the test suite level.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
Reviewed-by: Alex Chapman <alex.chapman@arm.com>
---
 dts/framework/test_suite.py                   | 68 +++++++++++++++++--
 dts/framework/testbed_model/tg_node.py        | 14 ++--
 .../capturing_traffic_generator.py            | 31 ---------
 3 files changed, 71 insertions(+), 42 deletions(-)
  

Hey Luca,

Thanks for the patch! I think this change makes a lot of sense in
general, and I like the idea of removing the duplication between
capturing traffic generators and the test suite. I just had one
thought that I left below, but otherwise:

Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>

On Tue, Aug 6, 2024 at 8:49 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> The framework allows only to send one packet at once via Scapy. This
> change adds the ability to send multiple packets, and also introduces a
> new fast way to verify if we received several expected packets.
>
> Moreover, it reduces code duplication by keeping a single packet sending
> method only at the test suite level.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Reviewed-by: Alex Chapman <alex.chapman@arm.com>
> ---
<snip>
> +    def match_all_packets(
> +        self, expected_packets: list[Packet], received_packets: list[Packet]
> +    ) -> None:

This is a very interesting approach to comparing what you expect to
what you received. I hadn't seen counters used before but they seem
useful for this purpose. This method reminds me a lot of the filtering
process that was discussed in bugzilla ticket 1438
(https://bugs.dpdk.org/show_bug.cgi?id=1438) where there was some talk
about how to handle the noise vs what you received. This is different
in a few ways though of course in that it allows you to specify
multiple different criteria that are acceptable rather than just
getting the payload and really only concerns itself with matching
lists instead of doing any filtering.

The main reason I mention this is it brought up a question for me: Do
you think, in the future when the payload filtering method is
implemented, these two methods will co-exist or it would make more
sense to just let test suites get their noise-free list of packets and
then check that what they care about is present? I think a method like
this might be useful in some more niche cases where you have multiple
packet structures to look for, so I don't doubt there are some reasons
to have both, I was just wondering if you had thought about it or had
an opinion.

> +        """Matches all the expected packets against the received ones.
> +
> +        Matching is performed by counting down the occurrences in a dictionary which keys are the
> +        raw packet bytes. No deep packet comparison is performed. All the unexpected packets (noise)
> +        are automatically ignored.
> +
> +        Args:
> +            expected_packets: The packets we are expecting to receive.
> +            received_packets: All the packets that were received.
> +
> +        Raises:
> +            TestCaseVerifyError: if and not all the `expected_packets` were found in
> +                `received_packets`.
> +        """
> +        expected_packets_counters = Counter(map(raw, expected_packets))
> +        received_packets_counters = Counter(map(raw, received_packets))
> +        # The number of expected packets is subtracted by the number of received packets, ignoring
> +        # any unexpected packets and capping at zero.
> +        missing_packets_counters = expected_packets_counters - received_packets_counters
> +        missing_packets_count = missing_packets_counters.total()
> +        self._logger.debug(
> +            f"match_all_packets: expected {len(expected_packets)}, "
> +            f"received {len(received_packets)}, missing {missing_packets_count}"
> +        )
> +
> +        if missing_packets_count != 0:
> +            self._fail_test_case_verify(
> +                f"Not all packets were received, expected {len(expected_packets)} "
> +                f"but {missing_packets_count} were missing."
> +            )
> +
<snip>
> --
> 2.34.1
>
  

This is a nice improvement. The comment below won't necessarily results 
in any changes, so:
Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>

> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py

> @@ -303,6 +329,40 @@ def verify_packets(self, expected_packet: Packet, received_packets: list[Packet]
>               )
>               self._fail_test_case_verify("An expected packet not found among received packets.")
>   
> +    def match_all_packets(
> +        self, expected_packets: list[Packet], received_packets: list[Packet]
> +    ) -> None:
> +        """Matches all the expected packets against the received ones.
> +
> +        Matching is performed by counting down the occurrences in a dictionary which keys are the
> +        raw packet bytes. No deep packet comparison is performed. All the unexpected packets (noise)
> +        are automatically ignored.
> +
> +        Args:
> +            expected_packets: The packets we are expecting to receive.
> +            received_packets: All the packets that were received.
> +
> +        Raises:
> +            TestCaseVerifyError: if and not all the `expected_packets` were found in
> +                `received_packets`.
> +        """
> +        expected_packets_counters = Counter(map(raw, expected_packets))
> +        received_packets_counters = Counter(map(raw, received_packets))
> +        # The number of expected packets is subtracted by the number of received packets, ignoring
> +        # any unexpected packets and capping at zero.
> +        missing_packets_counters = expected_packets_counters - received_packets_counters
> +        missing_packets_count = missing_packets_counters.total()
> +        self._logger.debug(
> +            f"match_all_packets: expected {len(expected_packets)}, "
> +            f"received {len(received_packets)}, missing {missing_packets_count}"
> +        )
> +
> +        if missing_packets_count != 0:
> +            self._fail_test_case_verify(
> +                f"Not all packets were received, expected {len(expected_packets)} "
> +                f"but {missing_packets_count} were missing."
> +            )
> +

Is it worthwhile to log the missing packets? It's not necessary, as the 
received packets are logged elsewhere, but it would be convenient. On 
the other hand, it could just unnecessarily bloat logs.
  

On 09/08/2024 16:10, Jeremy Spewock wrote:
> <snip>
>> +    def match_all_packets(
>> +        self, expected_packets: list[Packet], received_packets: list[Packet]
>> +    ) -> None:
> 
> This is a very interesting approach to comparing what you expect to
> what you received. I hadn't seen counters used before but they seem
> useful for this purpose. This method reminds me a lot of the filtering
> process that was discussed in bugzilla ticket 1438
> (https://bugs.dpdk.org/show_bug.cgi?id=1438) where there was some talk
> about how to handle the noise vs what you received. This is different
> in a few ways though of course in that it allows you to specify
> multiple different criteria that are acceptable rather than just
> getting the payload and really only concerns itself with matching
> lists instead of doing any filtering.
> 
> The main reason I mention this is it brought up a question for me: Do
> you think, in the future when the payload filtering method is
> implemented, these two methods will co-exist or it would make more
> sense to just let test suites get their noise-free list of packets and
> then check that what they care about is present? I think a method like
> this might be useful in some more niche cases where you have multiple
> packet structures to look for, so I don't doubt there are some reasons
> to have both, I was just wondering if you had thought about it or had
> an opinion.

I am not actually entirely sure, it sounds as if everything has space 
for their own case. This method is useful to just plainly check that all 
the packets we sent came back, simple and straightforward. Payload 
filtering can be more niche and complex. I think if it can provide the 
same level of functionality as this function, it can be replaced for 
sure. But in that case it sounds like the test developer would be tied 
to a specific payload. In the case of random packet generation I don't 
think it would work, in which case they could both coexist.
  

On 23/08/2024 11:17, Juraj Linkeš wrote:
> Is it worthwhile to log the missing packets? It's not necessary, as the 
> received packets are logged elsewhere, but it would be convenient. On 
> the other hand, it could just unnecessarily bloat logs.

I considered it but as you said I think it may unecessarily bloat the 
logs. The "unfiltered" packets can already be retrieved anyways if needed.
  

On Fri, Sep 6, 2024 at 12:23 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 09/08/2024 16:10, Jeremy Spewock wrote:
> > <snip>
> >> +    def match_all_packets(
> >> +        self, expected_packets: list[Packet], received_packets: list[Packet]
> >> +    ) -> None:
> >
> > This is a very interesting approach to comparing what you expect to
> > what you received. I hadn't seen counters used before but they seem
> > useful for this purpose. This method reminds me a lot of the filtering
> > process that was discussed in bugzilla ticket 1438
> > (https://bugs.dpdk.org/show_bug.cgi?id=1438) where there was some talk
> > about how to handle the noise vs what you received. This is different
> > in a few ways though of course in that it allows you to specify
> > multiple different criteria that are acceptable rather than just
> > getting the payload and really only concerns itself with matching
> > lists instead of doing any filtering.
> >
> > The main reason I mention this is it brought up a question for me: Do
> > you think, in the future when the payload filtering method is
> > implemented, these two methods will co-exist or it would make more
> > sense to just let test suites get their noise-free list of packets and
> > then check that what they care about is present? I think a method like
> > this might be useful in some more niche cases where you have multiple
> > packet structures to look for, so I don't doubt there are some reasons
> > to have both, I was just wondering if you had thought about it or had
> > an opinion.
>
> I am not actually entirely sure, it sounds as if everything has space
> for their own case. This method is useful to just plainly check that all
> the packets we sent came back, simple and straightforward. Payload
> filtering can be more niche and complex. I think if it can provide the
> same level of functionality as this function, it can be replaced for
> sure. But in that case it sounds like the test developer would be tied
> to a specific payload. In the case of random packet generation I don't
> think it would work, in which case they could both coexist.

Good points, this makes sense to me!