Skip to content

tests: close two IP conflict coverage gaps from #482's review - #486

Merged
JarryShaw merged 1 commit into
mainfrom
test-ip-conflict-coverage-gaps
Sep 18, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
test-ip-conflict-coverage-gaps

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Summary

PR #482 (closing #477) made IP fragment reassembly record overlap conflicts instead of
silently losing data, adding _detect_conflicts and a conflict field on Buffer and
Datagram in pcapkit/foundation/reassembly/ip.py. Its reviewer approved the change but
named two genuine coverage gaps in the new logic -- neither pointing at a suspected bug,
both untested branches of otherwise-verified behaviour. This PR closes both, test-only.

  • Gap 1 -- no test had two BUFIDs genuinely in flight at once. conflict is meant to be
    strictly per-Buffer; the reviewer judged the risk low because Buffer.conflict is
    created fresh per BUFID, but "created fresh" is an argument, not a test. This class of gap
    already burned this programme once: reassembly: resolve conflicting TCP overlaps first-write-wins, per RFC 9293 #478's original six boundary cases all used a single
    ACK bucket, which is exactly why they could not catch a cross-bucket data-loss regression
    that was genuinely present in the code. test_conflict_stays_scoped_to_its_own_bufid
    interleaves two datagrams' fragments -- neither completes before the other's next fragment
    arrives -- conflicts one, and asserts the other's conflict stays empty.

  • Gap 2 -- no test exercised a conflict sandwiched between two matching regions inside one
    write.
    The inner loop of _detect_conflicts breaks a conflict run on a match and resumes
    detection afterwards, but nothing drove a single fragment's comparison pass through two
    separate, non-adjacent conflict runs.
    test_single_write_produces_two_conflict_runs_separated_by_a_match constructs one fragment
    write that disagrees with the buffer at octets 8-15 and 24-31 but agrees at 16-23, and
    asserts both runs appear with the correct absolute inclusive bounds.

Both tests go through the public reassembly entry point and Datagram, not the private
_detect_conflicts routine directly.

No defect was found in pcapkit/ while writing these -- both gaps were genuinely untested
branches of correct logic, not bugs. No source under pcapkit/ is touched by this PR.

Regression proof

Each test was verified by temporarily breaking the exact behaviour it targets, confirming
the test failed, then restoring the source (git checkout -- pcapkit/foundation/reassembly/ip.py):

  • Gap 1: gave every Buffer the same shared conflict list instead of a fresh one per
    BUFID -> the clean datagram's conflict picked up the conflicting datagram's (0, 7)
    entry, failing the assertion that it stays ().
  • Gap 2: broke out of _detect_conflicts' outer loop after the first conflict run instead
    of resuming -> only (8, 15) was returned, missing (24, 31), failing the two-run
    assertion.

Test plan

  • pytest tests/foundation/reassembly/test_ip.py -- 16 passed
  • pytest tests/foundation/reassembly/ -- 65 passed
  • pytest tests/foundation/ -- 202 passed, 11 skipped
  • Both new tests independently confirmed to fail against a targeted regression, then
    pass again against the restored source

Refs: #477, #482

- #482's reviewer named two untested branches of the new overlap-conflict
  logic, neither a suspected bug: no test had two BUFIDs genuinely in
  flight at once, and no test drove one fragment's comparison pass through
  two non-adjacent conflict runs.
- Add test_conflict_stays_scoped_to_its_own_bufid: interleaves two
  datagrams' fragments, conflicts one, and asserts the other's `conflict`
  stays empty. This is the higher-value gap -- #478's original six
  boundary cases all shared one ACK bucket, which is exactly the shape
  that let a cross-bucket regression through undetected.
- Add test_single_write_produces_two_conflict_runs_separated_by_a_match:
  one fragment write disagrees at octets 8-15 and 24-31 but agrees at
  16-23, exercising _detect_conflicts' break-on-match-and-resume path.
- Verified each test by temporarily breaking the behaviour it targets
  (shared conflict list across BUFIDs; break after the first conflict
  run) and confirming the test fails, then restored the source.

Build/test: tests/foundation/reassembly/test_ip.py 16 passed;
tests/foundation/reassembly/ 65 passed; tests/foundation/ 202 passed,
11 skipped.
@JarryShaw
JarryShaw merged commit 8e6ffc8 into main Sep 18, 2026
24 checks passed
@JarryShaw

Copy link
Copy Markdown
Owner Author

Landed directly on main as part of 8e6ffc806, per the repo owner's standing allowance that test-only changes may be pushed straight to main rather than going through a PR.

Closing by hand because GitHub does not mark a PR merged when its commits arrive via a push rather than the merge button, even though they are genuinely on main — verified with git merge-base --is-ancestor <branch> origin/main, which passes for this branch.

Before pushing I confirmed the change really is test-only and that it passes, rather than taking the branch's word for it:

git diff --name-only fcd853032..HEAD | grep -v '^tests/' | wc -l   ->  0
tests/foundation/reassembly/test_ip.py + tests/protocols/internet/test_ipv6_extension_unit.py
    66 passed, 80 subtests passed

tests/foundation/reassembly/ + tests/protocols/internet/
    248 passed, 645 subtests passed   (exit 0)

main went fcd853032 → 8e6ffc806, carrying both test-only branches in one push: the ipv6-route tuple/list packing test and the two IP conflict coverage gaps from #482's review.

@JarryShaw
JarryShaw deleted the test-ip-conflict-coverage-gaps branch September 18, 2026 21:00
@JarryShaw JarryShaw added the test Pull requests that add or correct tests (test: subject prefix) label Sep 22, 2026
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Pull requests that add or correct tests (test: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant