Skip to content

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

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

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

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions