Re: [PATCH v20 24/25] test: add comprehensive test suite for pcap PMD

Bruce Richardson
2026-03-16T15:31:36Z

On Tue, Mar 10, 2026 at 09:10:02AM -0700, Stephen Hemminger wrote:
> Add unit tests for the pcap PMD covering file and interface modes.
>
> Tests include:
> - basic TX to file and RX from file
> - varied packet sizes and jumbo frames
> - infinite RX mode
> - TX drop mode
> - statistics
> - interface (iface=) pass-through mode
> - link status reporting for file and iface modes
> - link status change (LSC) with interface toggle
> - EOF notification via LSC
> - RX timestamps and timestamp with infinite RX
> - multiple TX/RX queues
> - VLAN strip, insert, and runtime offload configuration
> - snapshot length (snaplen) and truncation
>
> Cross-platform helpers handle temp file creation, interface
> discovery, and VLAN packet generation.
>
> The LSC link toggle test requires a pre-created dummy interface
> (Linux: dummy0, FreeBSD: disc0) and is skipped if unavailable.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> app/test/meson.build | 2 +
> app/test/test_pmd_pcap.c | 3755 ++++++++++++++++++++++++
> doc/guides/rel_notes/release_26_03.rst | 1 +
> 3 files changed, 3758 insertions(+)
> create mode 100644 app/test/test_pmd_pcap.c
>
Ran this patch through AI for a code review. Below is the output. In
general seems a good addition.

/Bruce

Code Review: test: add comprehensive test suite for pcap PMD
The patch adds a 3755-line test file. The breadth of coverage is commendable — it exercises most of the pcap PMD's devargs. However, there are several concrete problems:

Bugs / Incorrect Logic
1. Duplicate values in test_tx_varied_sizes test array (test_pmd_pcap.c:1203-1205)

PKT_SIZE_SMALL (128) == PACKET_BURST_GEN_PKT_LEN_128 (128), so only two distinct sizes are tested. Furthermore, PKT_SIZE_MIN(60) == PACKET_BURST_GEN_PKT_LEN (60) — the whole point of mixing these constants obscures the fact. The array should use distinct, intentional sizes like PKT_SIZE_MIN, PKT_SIZE_MEDIUM, PKT_SIZE_LARGE.

2. Timestamp exact-match check is non-asserting (test_pmd_pcap.c:2322-2325)

A timestamp accuracy mismatch only prints a warning; the test still passes. This should be a TEST_ASSERT_EQUAL(ts, expected, ...). This is the entire point of having controllable timestamps via create_timestamped_pcap().

Test Quality / Weak Assertions
3. test_tx_varied_sizes doesn't verify file content (test_pmd_pcap.c:1240-1244)
It only checks nb_tx > 0 — it never reads the pcap file back to confirm the sizes were written correctly. Compare with test_jumbo_tx which does read back via get_pcap_packet_sizes(). Both tests should do the same.

4. Statistics test doesn't verify ibytes/obytes values (test_pmd_pcap.c:1462-1474)
test_stats validates ipackets/opackets but despite checking ibytes == 0 && obytes == 0 at init, it never asserts the actual byte totals after traffic. The driver tracks bytes; the test should verify ibytes == received * sizeof(test_packet).

5. test_lsc_iface skips with TEST_SUCCESS instead of TEST_SKIPPED (test_pmd_pcap.c:1981-2000)
Three early-exit paths when no interface is available return TEST_SUCCESS:

Compare with test_iface() at test_pmd_pcap.c:1644 which correctly returns TEST_SKIPPED. This makes CI results misleading — a skipped test looks like a passing one.

6. Similarly in test_rx_timestamp (test_pmd_pcap.c:2308)
When the timestamp dynamic field isn't available, return TEST_SUCCESS is used instead of TEST_SKIPPED.

Racy / Fragile Tests
7. LSC callback detection relies entirely on usleep(1500ms) (test_pmd_pcap.c:2050, test_pmd_pcap.c:2074)
The test sleeps 1.5 seconds twice waiting for the pcap LSC polling thread to detect the interface state change. With no retry/poll loop, this is fragile under load and adds 3 seconds to every run. A small polling loop (for (i = 0; i < 20; i++) { if (lsc_callback_count >= 1) break; usleep(100000); }) would be more robust.

8. EOF callback timing (test_pmd_pcap.c:2229)
usleep(100 * 1000) (100 ms) is used to wait for the EOF alarm to fire, then immediately asserts eof_callback_count == 1. The driver uses rte_eal_alarm_set(1, ...) (1 µs delay), but alarm delivery is not instantaneous under load. The same polling loop pattern would be safer.

Missing Coverage
9. No tests for rx_iface=/tx_iface=/rx_iface_in= devargs
The test covers iface= (symmetric single-interface mode) but completely misses rx_iface=, tx_iface=, and rx_iface_in= (asymmetric / separate RX and TX interface modes). These are distinct code paths in the driver including PCAP_D_IN direction filtering for rx_iface_in.

10. No test for per-queue start/stop
The driver implements rx_queue_start, tx_queue_start, rx_queue_stop, tx_queue_stop. None of these are exercised.

11. imissed stat not tested
The driver has queue_missed_stat_update() using pcap_stats() to track kernel-level drops. This code path (iface mode under load, or mocked via the pcap stat interface) is never exercised.

Minor Issues
12. Shared state between tests (test_pmd_pcap.c:60-61)
tx_pcap_path and vlan_rx_pcap_path are global state shared between test pairs (test_tx_to_file→test_rx_from_file, test_vlan_strip_rx→test_vlan_no_strip_rx). Both have fallback access() checks, but this creates implicit ordering dependencies. The comment documents this, but it's an unusual pattern for a test suite that should have independent cases.

13. Missing rte_cycles.h include for NS_PER_S (test_pmd_pcap.c:2319)
NS_PER_S is used in test_rx_timestamp() but <rte_cycles.h> (where it's defined) is not explicitly included. It compiles today because another included header transitively pulls it in, but the dependency is fragile. A direct #include <rte_cycles.h> should be added.