Hey Dean, thanks for the update! I just went through and added my comments to the patches, but most of my feedback was about documentation and a few places where I think the classes could be improved by refactoring a little bit and breaking some things out into their own methods.
A few comments on the overall series however: I think this patch should come after the patch that adds the testpmd functions. Just because the order they appear in the series is the order they get applied, so this patch without the patch that adds the testpmd methods would throw a lot of errors because the methods don't exist yet. Also, could you run the formatting script (devtools/dts-check-format.sh in the DPDK directory) on this series when sending out the next version? I noticed it was throwing some warnings/errors. On Tue, Jun 25, 2024 at 11:34 AM Dean Marx <dm...@iol.unh.edu> wrote: > > Test suite for verifying VLAN filtering, stripping, and insertion > functionality on Poll Mode Driver. > > Signed-off-by: Dean Marx <dm...@iol.unh.edu> > --- > + def send_vlan_packet_and_verify( > + self, should_receive: bool, strip: bool, vlan_id: int > + ) -> None: > + """Generate a vlan packet, send and verify a packet with > + the same payload is received on the dut. > + > + Args: > + should_receive: Indicate whether the packet should be > successfully received. > + vlan_id: Expected vlan ID. > + strip: Indicates whether stripping is on or off, and when the > vlan tag is > + checked for a match. We probably should have these args listed in the order that they appear in the function (should_receive, strip, vlan_id). > + """ <snip> > + > + def send_packet_and_verify_insertion(self, expected_id: int) -> None: > + """Generate a packet with no vlan tag, send and verify on the dut. > + > + Args: > + expected_id: The vlan id that is being inserted through > tx_offload configuration. > + should_receive: Indicate whether the packet should be > successfully received. The should_receive parameter seems like it was removed, we should probably remove this part of the doc-string as well. > + """ > + packet = Ether() / Raw(load='xxxxx') > + received_packets = self.send_packet_and_capture(packet) > + test_packet = None > + for packet in received_packets: > + if b'xxxxx' in packet.load: > + test_packet = packet > + break > + self.verify( > + test_packet is not None, "Packet was dropped when it should have > been received" > + ) > + self.verify(Dot1Q in test_packet, "The received packet did not have > a vlan tag") > + self.verify(test_packet.vlan == expected_id, "The received tag did > not match the expected tag") > + > + def test_vlan_receipt_no_stripping(self) -> None: > + """Ensure vlan packet is dropped when receipts are enabled and > header stripping is disabled. > + > + Test: > + Create an interactive testpmd shell and verify a vlan packet. > + """ > + testpmd = TestPmdShell(node=self.sut_node) > + testpmd.set_forward_mode(SimpleForwardingModes.mac) > + testpmd.set_verbose(1) > + testpmd.set_promisc(0, False) > + testpmd.vlan_filter_set_on(0) > + filtered_vlan = 1 > + testpmd.rx_vlan_add(filtered_vlan, 0) > + testpmd.start() All of these test cases have the exact same start sequences (other than one additional command for the tests that involve stripping, or the 3 extra in the insertion case), we should find a way to pull this out into another function that they can all call that does the testpmd setup. Or, alternatively, you could make a decorator for these test methods that handles all of the testpmd commands. That way, these test_ methods only need to concern themselves with how they call the method for sending and verifying packets and don't need to run any testpmd commands. I slightly prefer the decorator approach since I think that would be cleaner than if you were to make a testpmd shell, pass it into a setup function, and then close it after running testing, but it might be less clear what's actually going on. Regardless, the main point of it is having less code duplication between functions. > + > + self.send_vlan_packet_and_verify(True, strip=False, > vlan_id=filtered_vlan) > + testpmd.close() > + def test_vlan_receipt_stripping(self) -> None: > + """Ensure vlan packet received with no tag when receipts and header > stripping are enabled. > + > + Test: > + Create an interactive testpmd shell and verify a vlan packet. > + """ > + testpmd = TestPmdShell(node=self.sut_node) > + testpmd.set_forward_mode(SimpleForwardingModes.mac) > + testpmd.set_verbose(1) > + testpmd.set_promisc(0, False) > + testpmd.vlan_filter_set_on(0) > + testpmd.rx_vlan_add(1, 0) This VLAN tag should probably also be pulled out into a variable like it is in other test methods, but we really should just do it once in a separate setup method instead of changing this to match up. > + testpmd.vlan_strip_set_on(0) > + testpmd.start() > + > + self.send_vlan_packet_and_verify(should_receive=True, strip=True, > vlan_id=1) > + testpmd.close() > + def test_vlan_no_receipt(self) -> None: > + """Ensure vlan packet dropped when filter is on and sent tag not in > the filter list. > + > + Test: > + Create an interactive testpmd shell and verify a vlan packet. > + """ > + testpmd = TestPmdShell(node=self.sut_node) > + testpmd.set_forward_mode(SimpleForwardingModes.mac) > + testpmd.set_verbose(1) > + testpmd.set_promisc(0, False) > + testpmd.vlan_filter_set_on(0) > + filtered_vlan = 1 > + testpmd.rx_vlan_add(filtered_vlan, 0) > + testpmd.start() > + > + self.send_vlan_packet_and_verify(should_receive=False, strip=False, > vlan_id=filtered_vlan + 1) > + testpmd.close() > + > + def test_vlan_header_insertion(self) -> None: > + """Ensure that vlan packet is received with the correct inserted > vlan tag. > + > + Test: > + Create an interactive testpmd shell and verify a non-vlan packet. > + """ > + testpmd = TestPmdShell(node=self.sut_node) > + testpmd.set_forward_mode(SimpleForwardingModes.mac) > + testpmd.set_verbose(1) > + testpmd.set_promisc(0, False) > + testpmd.port_stop_all() > + testpmd.tx_vlan_set(1, 51) > + testpmd.port_start_all() > + testpmd.start() > + > + self.send_packet_and_verify_insertion(expected_id=51) > + testpmd.close() > -- > 2.44.0 >
