Re: [PATCH v3 2/2] dts: add QinQ test suite

[Dean Marx <dmarx@iol.unh.edu>]

On Fri, Nov 7, 2025 at 3:15 PM Patrick Robb <probb@iol.unh.edu> wrote:
<snip>
>
> The testcase does look good except that form what I'm seeing, vlan_filter is not supported for 88a8 + 8100 packets (it won't read the 88a8 tag). I sent Bruce and Morten an email about it. But, I think the most reasonable thing is to remove this testcase for the 25.11 release since the expected behavior is either not well defined, or vlan_filter is not supposed to be able to read 88a8 tags.

Agreed, I'll remove this in the new version

<snip>
>> +
>> +            verify(packet is not None, "Packet was dropped when it should have been received.")
>> +
>> +            if packet is not None:
>> +                verify(bool(packet.haslayer(Dot1AD)), "QinQ layer not found in packet")
>
>
> I guess you can also verify that packet haslayer(Dot1Q)

Great point, I hadn't realized the final if statement could be
silently failing if the Dot1Q layer doesn't exist. I'll update this

<snip>
>> +            received_packets1 = send_packet_and_capture(test_packets[0])
>> +            packet1 = self._get_relevant_packet(received_packets1)
>> +            received_packets2 = send_packet_and_capture(test_packets[1])
>> +            packet2 = self._get_relevant_packet(received_packets2)
>> +            received_packets3 = send_packet_and_capture(test_packets[2])
>> +            packet3 = self._get_relevant_packet(received_packets3)
>> +            received_packets4 = send_packet_and_capture(test_packets[3])
>> +            packet4 = self._get_relevant_packet(received_packets4)
>
>
> rename to make them more descriptive? I.e. vlan_packet, 2_vlan_packet, qinq_packet, qinq_vlan_packet? Or similar.

Got it

>
>>
>> +
>> +            testpmd.stop()
>> +
>> +            tests = [
>> +                ("Single 8100 tag", self._strip_verify(packet1, False, "Single 8100 tag")),
>> +                (
>> +                    "Double 8100 tag",
>> +                    self._strip_verify(packet2, True, "Double 8100 tag"),
>> +                ),
>> +                ("Single 88a8 tag", self._strip_verify(packet3, False, "Single 88a8 tag")),
>> +                (
>> +                    "QinQ (88a8 and 8100 tags)",
>> +                    self._strip_verify(packet4, False, "QinQ (88a8 and 8100 tags)"),
>> +                ),
>> +            ]
>
>
> this is good for testing behavior when we have both vlan_strip and qinq_strip enabled. What about when we have only vlan_strip enabled or only qinq_strip enabled? I.e. when we have only vlan_strip enabled, and we send an 88a8 only packet, nothing should get stripped at all. It probably makes sense to have different testcases for vlan strip, qinq_strip, and vlan_strip + qinq_strip.

My logic here was that vlan strip only is being tested in the vlan
suite already, even if it isn't with this set of QinQ style packets. I
think it would make more sense to add those extra cases in the vlan
suite in the future anyways, rather than in the QinQ suite.

I didn't test only qinq strip either because Stephen Hemminger
mentioned it was undefined in this email thread (second message):

https://mail.google.com/mail/u/0/#search/label%3Adev+qinq/FMfcgzQbgJNPMqLMRLkTsrxMrfRjJLzq

>
>>
>> +
>> +            failed = [ctx for ctx, result in tests if not result]
>> +
>> +            verify(
>> +                not failed,
>> +                f"The following packets were not stripped correctly: {', '.join(failed)}",
>> +            )
>> --
>> 2.51.0
>>
>
> Reviewed-by: Patrick Robb <probb@iol.unh.edu>