Skip to content

tests: parity mac() helper double-encodes pypcapfile addresses that are already ASCII #749

Closed
Closed
tests: parity mac() helper double-encodes pypcapfile addresses that are already ASCII#749
Labels
bugIssues reporting a defect (set by the bug report template; a default, not an assessment)testPull requests that add or correct tests (test: subject prefix)
Milestone
PyPCAPKit v1.5

Description

@JarryShaw
JarryShaw
opened on Sep 24, 2026

tests/foundation/engines/test_new_engine_parity_runtime.py:82's mac() helper assumes pypcapfile's Ethernet.dst/.src are raw 6-byte values needing hex-formatting. They are already colon-separated ASCII.

Confirmed at source — pcapfile/protocols/linklayer/ethernet.py, Ethernet.__init__:

(dst, src, self.type) = struct.unpack('!6s6sH', packet[:14])
dst = bytearray(dst); src = bytearray(src)
self.dst = b':'.join([('%02x' % o).encode('ascii') for o in dst])
self.src = b':'.join([('%02x' % o).encode('ascii') for o in src])

So .dst is b'40:33:1a:d1:85:1c', and mac() hex-encodes that a second time — producing '33:34:3a:...' against an expected '40:33:1a:d1:85:1c'. This is unconditional in pypcapfile and independent of layers=.

Why this matters for the other two

test_pypcapfile_agrees_with_the_default_engine is one of three HAS_PYPCAPFILE-gated parity methods, and each of the three needs a different fix:

test needs
test_pypcapfile_agrees_with_the_default_engine this issue
ipv4_reassembly parity #746 (engine hexlify) + #743 (toolkit addresses)
traces_only parity #746 + #743

Measured on 3.10.21: with #746's fix alone the Ethernet fields decode correctly but the other two hit #743's AddressValueError; overlaying #747's toolkit fix takes both fully green. This one stays red regardless, because neither the engine nor the toolkit can reach it.

So "the three parity tests go green" was the wrong acceptance criterion for #746 — I set it, and it was only ever true for two of them, and only in combination with #747. Correcting that here rather than leaving it implied.

Scope note

The fix is almost certainly deleting the re-encoding in mac() rather than changing library code — the library is right and the test is wrong. Worth confirming whether mac() is also applied to the default engine's output in the same comparison, because if so it may be correct for one side and wrong for the other, which would make a bare deletion break the other half.

Found while fixing #746 (PR #748), which correctly did not edit this file.

Activity

  1. JarryShaw
    added
    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)
    testPull requests that add or correct tests (test: subject prefix)
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Sep 24, 2026
  2. JarryShaw
    closed this as completedin 9813aa3on Sep 24, 2026
  3. JarryShaw
    removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Sep 24, 2026

  4. JarryShaw commented on Sep 24, 2026

    @JarryShaw
    JarryShaw
    on Sep 24, 2026
    OwnerAuthor

    Correcting my own dependency table. I classified test_pypcapfile_agrees_with_the_default_engine as needing only this issue. It needs #749 + #746, and possibly a third thing.

    The fix landed as 9813aa377 and is right, but the test stays red on main today — for all 4 captures, not a subset. Measured on 3.10.21:

    stage ethernet dst
    before #749 '33:34:3a:33:30:3a:…' — double-hex-encoded
    after #749 '30:30:30:63:32:39' — decodes to '000c29'
    expected '00:0c:29:7d:1d:b4'

    That second value is what Ethernet() produces when it unpacks its 14-byte header from hex-ASCII characters rather than raw bytes — i.e. #746, still unfixed (foundation/engines/pypcapfile.py:362 calls self._declf(packet.packet, …) with no binascii.unhexlify()). Proven two ways without touching that file: constructing Ethernet(binascii.unhexlify(raw_hex), layers=1) directly gives b'00:0c:29:7d:1d:b4'; and simulating #746's fix via mock.patch.object(Ethernet, '__init__', …) plus this fix takes the test fully green, 0 failures, all 4 captures.

    And #747's reviewer found a further blocker for the same test, independently: pcapkit/toolkit/pypcapfile.py:262-263's _ethernet2dict returns bytes(link.dst), which for pypcapfile is already b'40:33:1a:d1:85:1c' — verified. So this test may need that fixed too, and #746 alone will not deliver it.

    Whoever lands #748 should add this test to its "makes green" list — right now nothing tracks that dependency.

    The judgement call, resolved

    mac() has exactly two call sites and is correct at the other one, so it must not change:

    • test_pypcap_agrees_with_the_default_engine:171,173 — mac(packet[0:6]) on pypcap's frame tuple, genuinely raw 6-byte slices. Correct, untouched.
    • test_pypcapfile_agrees_with_the_default_engine:204,206 — the defect.

    Both tests compare against str(info.src)/str(info.dst) from ethernet_of(), never through mac(). So the fix was to the pypcapfile call site only: ethernet.dst.decode('ascii'). Format confirmed compatible — the default engine's Ethernet._read_mac_addr (pcapkit/protocols/link/ethernet.py:218) uses addr.hex(':'), lowercase colon-separated, identical to pypcapfile's own encoding.

  5. JarryShaw
    added this to the 1.5 milestone on Oct 6, 2026
  6. JarryShaw
    added this to PyPCAPKiton Oct 6, 2026
  7. JarryShaw
    moved this to Done in PyPCAPKiton Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)testPull requests that add or correct tests (test: subject prefix)

    Projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions