Skip to content

fix(protocols): _make_data returns keys make() does not accept (Frame ts_src, L2TPv2 prio) - #1115

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1097-make-data-keys
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1097-make-data-keys

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort): ran those tools with the Makefile flags on the two source files. mypy is clean, isort is clean, and pylint reports only pre-existing line-too-long/redefined-builtin, none on the changed lines
  • make test passes, and a test case covers the change: ran the 7 modules listed below plus tests/project (not the full suite)
  • Added a changelog entry — N/A, added centrally after the wave

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1097. Renames Frame._make_data ts_src → ts_sec and L2TPv2._make_data prio → priority, so the keys match make().

Probe. Before: L2TPv2 810200010002 → from_data → 800200010002, warning discarded: 'prio' (did you mean 'priority'?). After: 810200010002, no warning. Frame: 'ts_src' (did you mean 'ts_sec'?) before, and ts_sec preserved after.

Tests. The new tests/protocols/test_make_data_keys_unit.py checks a round-trip per protocol. It fails without the fix (2 failed) and passes with it (2 passed). These tests pinned the old keys and are updated: test_link_unit.py:373, test_header_frame_unit.py:186, and test_construction_keyword_check_unit.py. In the last one, the warns case now uses a contrived subclass, and the recorded table drops Frame and L2TPv2. Per module: make_data_keys 2 passed, header_frame 6, construction_keyword_check 22, link 24, l2tp_version 7, frame_runtime 4, option_roundtrip 6 (none of its entries were mine). tests/project: 379 passed, 1 skipped.

Sweep (AST of 29 _make_data vs make() and declared keywords). Still mismatched, out of scope: Header magic_number, and HTTPv2 length (not in make(), but accepted by __init__).

… ts_src, L2TPv2 prio)

Frame._make_data returned ts_src and L2TPv2._make_data returned prio,
where make() takes ts_sec and priority, so from_data discarded the
field with an UnknownFieldWarning. Rename both keys.

Add tests/protocols/test_make_data_keys_unit.py with a from_data
round-trip per protocol. Update the tests that pinned the old keys,
and rebase the from_data-warns case on a contrived subclass.

Closes #1097
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 6d1275919: GOOD TO GO (ran on Sonnet; author Opus)

  • Renames: both now match make(): ts_sec for Frame and priority for L2TPv2. L2TPv2 from_data is byte-identical with and without priority, ns/nr and offset, and emits no warning.
  • Edited tests: the contrived Mismatched(L2TPv2) subclass still exercises the warning path. With the source reverted, that module still passes 22, which shows it no longer depends on the real bug. Of the three keys the table recorded, Header: magic_number is the one left.
  • Key sweep: the reviewer re-ran it independently over 30 _make_data implementations. Only Header.magic_number remains, plus HTTPv2.length, which is accepted via read/unpack. Nothing was missed.
  • Frame test: the _make_data patch only adds num/header. The real ts_sec key still flows through the real from_data.

Follow-ups, filed separately:

  • Frame.from_data cannot be called publicly, because it has no way to take num/header.
  • The comment at protocol.py:250 still names ts_src/prio as examples.

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.74% (unit tier, Python 3.14, 6d1275919, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1035 2342 846 90.23%
pcapkit/corekit 1874 91 578 22 94.33%
pcapkit/dumpkit 136 0 40 0 100.00%
pcapkit/foundation 2422 143 842 34 92.62%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15653 187 3942 162 98.19%
pcapkit/toolkit 487 71 144 3 84.15%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 47e1e36 into main Oct 6, 2026
41 checks passed
@JarryShaw
JarryShaw deleted the fix/1097-make-data-keys branch October 6, 2026 20:36
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(protocols): _make_data returns keys make() does not accept (Frame ts_src, L2TPv2 prio)

1 participant