Re: [PATCH v2 3/3] dts: mac filter test suite refactored for new dts

[Jeremy Spewock <jspewock@iol.unh.edu>]

Code all looked good to me, just a couple of documentation comments.

On Tue, Jul 2, 2024 at 3:25 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
><snip>
> +class TestMacFilter(TestSuite):
> +    """Mac address allowlist filtering test suite.
> +
> +    Configure mac address filtering on a given port, and test the port's filtering behavior
> +    using both a given port's hardware address as well as dummy addresses. If a port accepts
> +    a packet that is not contained within its mac address allowlist, then a given test case
> +    fails. Alternatively, if a port drops a packet that is designated within its mac address
> +    allowlist, a given test case will fail.
> +
> +    Moreover, a given port should demonstrate proper behavior when bound to the Poll Mode
> +    Driver. A port should not have an mac address allowlist that exceeds its designated size.

I think this should just be " a mac address allowlist" rather than "an".

> +    A port's default hardware address should not be removed from its address pool, and invalid
> +    addresses should not be included in the allowlist. If a port abides by the above rules, the
> +    test case passes.
> +    """
> +
> +    def send_packet_and_verify(
> +        self,
> +        mac_address: str,
> +        add_vlan: bool = False,
> +        should_receive: bool = True,
> +    ) -> None:
> +        """Generate, send, and verify a packet based on specified parameters.
> +
> +        Test cases within this suite utilize this method to create, send, and verify
> +        packets based on criteria relating to the packet's destination mac address,
> +        vlan tag, and whether or not the packet should be received or not. Packets
> +        are verified using an inserted payload. If the list of received packets
> +        contains this payload within any of its packets, the test case passes. Each

It might be worth noting here that it passes assuming `should_recieve` is true.

> +        call with this method sends exactly one packet.
> +
> +        Args:
> +            mac_address: The destination mac address of the packet being sent.
> +            add_vlan: Add a vlan tag to the packet being sent. :data:'2' if the packet

I think if this started with "Whether to add..."  it would be more
clear that it is a boolean.

> +                should be received, :data:'1' if the packet should not be received but
> +                requires a vlan tag, and None for any other condition.

This tripped me up at first because I thought you were saying that
add_vlan could be 2, 1, or None. It might be worth just adding to the
start of that second sentence that "The tag will be...". Also, it
might be more clear if you explain that the tag will be omitted in
other cases rather than it being None.

> +            should_receive: If :data:'True', assert whether or not the sent packet
> +                has been received. If :data:'False', assert that the send packet was not
> +                received. :data:'True' by default
> +        """
> +        if add_vlan:
> +            packet = Ether() / Dot1Q(vlan=2 if should_receive else 1) / IP() / Raw(load="X" * 22)
> +        else:
> +            packet = Ether() / IP() / Raw(load="X" * 22)
> +        packet.dst = mac_address
> +        received_packets = [
> +            packets
> +            for packets in self.send_packet_and_capture(packet, adjust_addresses=False)
> +            if hasattr(packets, "load") and "X" * 22 in str(packets.load)
> +        ]
> +        if should_receive:
> +            self.verify(len(received_packets) == 1, "Expected packet not received")
> +        else:
> +            self.verify(len(received_packets) == 0, "Expected packet received")
> +
> +    def test_add_remove_mac_addresses(self) -> None:
> +        """Assess basic mac addressing filtering functionalities.
> +
> +        This test cases validates for proper behavior of mac address filtering with both

Small typo, but I think this is meant to be "this test case validates...".

> +        a port's default, burned-in mac address, as well as additional mac addresses
> +        added to the PMD. Packets should either be received or not received depending on
> +        the properties applied to the PMD at any given time.
> +
> +        Test:
> +            Start TestPMD with promiscuous mode.
> +            Send a packet with the port's default mac address. (Should receive)
> +            Send a packet with fake mac address. (Should not receive)
> +            Add fake mac address to the PMD's address pool.
> +            Send a packet with the fake mac address to the PMD. (Should receive)
> +            Remove the fake mac address from the PMD's address pool.
> +            Sent a packet with the fake mac address to the PMD. (Should not receive)
> +        """
> +        testpmd = TestPmdShell(self.sut_node)
> +        testpmd.set_promisc(0, on=False)
> +        testpmd.start()
> +        mac_address = self._sut_port_ingress.mac_address
> +
> +        # Send a packet with NIC default mac address
> +        self.send_packet_and_verify(mac_address=mac_address, should_receive=True)
> +        # Send a packet with different mac address
> +        fake_address = "00:00:00:00:00:01"
> +        self.send_packet_and_verify(mac_address=fake_address, should_receive=False)
> +
> +        # Add mac address to pool and rerun tests
> +        testpmd.set_mac_addr(0, mac_address=fake_address, add=True)
> +        self.send_packet_and_verify(mac_address=fake_address, should_receive=True)
> +        testpmd.set_mac_addr(0, mac_address=fake_address, add=False)
> +        self.send_packet_and_verify(mac_address=fake_address, should_receive=False)
> +        testpmd.close()
> +        sleep(6)
> <snip>

> --
> 2.44.0
>