Skip to content

tests: PcapngUnescapedKeyTests' skip reason cites line 1376 for a failure at 1517, and #784 falsifies its plist half #785

Closed

Description

@JarryShaw

Found while reviewing #784, and confirmed twice over — the claim was already wrong before that PR, and #784 makes half of it wronger.

tests/integration/test_pcapng_end_to_end.py:185, in PcapngUnescapedKeyTests's skip reason, reads:

…makes the plist report invalid XML at line 1376, where its & is unescaped.

Two problems, both measured:

  1. The line number is wrong, independently of fix(dumpkit): escape mapping keys for the plist writer (#772) #784. The malformed token is at line 1517 of a 1958-line report, not 1376 — ET.parse on 0419c1c97 fails with not well-formed (invalid token): line 1517, column 19. So the citation was already stale before anyone touched the escaping.
  2. The plist half stops being true when fix(dumpkit): escape mapping keys for the plist writer (#772) #784 lands. fix(dumpkit): escape mapping keys for the plist writer (#772) #784 (9bec70e22) routes both mapping branches through an escape_key helper, so the plist and xml reports of that capture then parse. The json half still holds — json.load still rejects the report, for the separate raw-bytes-key quoting defect upstream in dictdumper.

So after #784 the skip reason will assert something demonstrably false about plist while remaining correct about json. A reader chasing "invalid XML at line 1376" finds neither the line nor the failure.

What wants doing: reword the skip reason and the class docstring to name only the defect that survives — the json one — with the right mechanism, and drop or correct the line number. If a line number is kept it should be justified by a measurement at the commit that records it, since this one has already drifted once.

Left untouched deliberately: that file was outside #784's owned set, and a prose fix riding along in an unrelated PR is how stale claims get written in the first place.

Blocked on #784 merging, since the plist half is only false once it lands. Checkable blocker: #784 merged.

Activity

  1. added
    docsPull requests that change documentation only (docs: subject prefix)
    testPull requests that add or correct tests (test: subject prefix)
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    on Sep 25, 2026
  2. JarryShaw commented on Sep 25, 2026

    @JarryShaw
    OwnerAuthor

    Unblocked — #784 merged at 14:23:35Z as c6eede34b, so the blocker this issue named ("Checkable blocker: #784 merged") has cleared. Confirmed the fix is on merged main rather than assuming it from the merge:

    $ git show origin/main:pcapkit/dumpkit/common.py | grep -n "escape_key\|saxutils"
    18:import xml.sax.saxutils
    250:    def escape_key(key: 'Any') -> 'Any':
    289:        return xml.sax.saxutils.escape(format(key, ''))
    332:                    temp[escape_key(key)].append(val)
    339:                    result = {escape_key(key): val for key, val in o.items()}
    

    Both mapping branches route through escape_key, which is what makes the plist half of the skip reason false. The stale text is still there verbatim on main at tests/integration/test_pcapng_end_to_end.py:

    Measured on this fixture: json.load fails with Expecting ':' delimiter: line 915 column 12. The same key makes the plist report invalid XML at line 1376, where its & is unescaped.

    Note there are now two line numbers in that paragraph to re-measure, not one — the json half cites line 915 column 12 as well, and this issue only ever challenged the 1376. Both get measured at the commit that records them, per this issue's own condition that a retained line number be justified by a measurement at that commit.

    Dispatching a worker to measure all three facts on ef859f776 — whether the plist report now parses, what the json failure's real line and column are, and whether 1376 versus the 1517 this issue measured was itself pre-#784 drift — then reword to name only the defect that survives. blocked off, wip on.

  3. added
    wipWork in flight - a covering PR is open or an agent is actively on it
    and removed
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    on Sep 25, 2026
  4. removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Sep 25, 2026
  5. added this to the 1.5 milestone on 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

    docsPull requests that change documentation only (docs: subject prefix)testPull requests that add or correct tests (test: subject prefix)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions