Repository navigation
Move APRS tests to JSONL case vectors, compared against FAP and live traffic - #6
Conversation
|
Review of head Recommendation: address the following before merging. 1. P2: Compressed area-like comment text is silently discardedReproducer (Python bytes literal): b'N0CALL>APRS:!\\5c!!<<!!l7@C102/503'The new 2. P2: North wind emits an out-of-spec directionThis now serializes unchanged with 3. P2: W-symbol heuristic reinterprets incomplete weather as vehicle motionThis becomes motion (180, 10), no weather, and comment 4. P2: Replacement runner loses serializer refusal checksRejected rows return before calling Restore refusal checks, distinguishing header-only failures from payload failures, and restrict context exemptions to models that actually need context. 5. P3: Proposal output hides signed-zero changesThe outer comparison uses exact JSON comparison, while the per-field display uses Python equality. A change only in a coordinate's zero sign reports one differing row without printing the old/new decoded values. The same applies to 1 versus 1.0. Use the exact comparator when deciding which fields to display. Validation and scope
The exact snapshots and round trips are useful regression checks, but independently reviewed expectations remain important for protocol correctness. |
|
Thanks for the review. All five points were valid and were introduced by this PR; they are fixed in f54a790. 1. Compressed area-like comment text silently discardedAfter a compressed position no 7-byte data extension is read at all (course/speed, PHG, RNG, DFS, area, DF bearing): APRS101 pp. 27 and 36 place them only after uncompressed positions, and course/speed and range travel in csT. Text shaped like one stays comment and is sent unchanged, and the invalid-extension lookalike check no longer applies there. Your reproducer is now CMP-163: 2. North wind emitted as
|
afourney
left a comment
There was a problem hiding this comment.
Re-review of f54a790: the original reproductions are fixed, but two adjacent P2 gaps remain. I recommend addressing these before merging.
- Compressed W weather still becomes vehicle motion under the new opt-in policy.
With APRS_IS(extensions={"w_symbol_motion"}), parsing b"N0CALL>APRS:!/5c!!<<!!W7@Cr001" produces vehicle motion, weather=None, comment "r001", no diagnostics, and successful unchanged emission. The extension promises motion only when no weather field follows, but compressed detection recognizes only g, t, and c. Rainfall, humidity and pressure fields must also retain weather interpretation and trigger diagnostics/refusal for incomplete reports. Reproduced with both symbol tables.
Code:
hamkit/packages/aprs/src/hamkit/aprs/_extensions.py
Lines 111 to 115 in f54a790
hamkit/packages/aprs/src/hamkit/aprs/_codec.py
Lines 347 to 352 in f54a790
- Rejected Mic-E payloads remain exempt from refusal assertions.
The runner invokes both serializers now, but skips checking the payload result for every Mic-E or symbol-bearing NMEA model. For MIC-424, serialize_payload correctly raises ConformanceError because altitude exceeds its wire range. Replacing it with a function returning b"INVALID" still lets test_case(row) pass. This altitude refusal is payload-local and does not depend on destination context. Restrict exemptions to actual context-dependent outcomes and assert payload-local refusals.
Code:
hamkit/packages/aprs/tests/test_cases.py
Lines 124 to 131 in f54a790
Resolved: compressed area-tail loss (including additional numeric/PHG/DFS/RNG comment checks), north-wind normalization to 360, default W-symbol classification and the original W repro, original status serializer mutations, and signed-zero proposal display.
Validation: full suite passed 7,830 tests plus 793 subtests on Python 3.12.14. Independent re-review had 31 passing checks and 11 failing checks: ten compressed-W variants and one refusal-coverage mutation. Six old W-symbol rejection tests now pass. FAP regeneration and the original full traffic capture remain unverified. Head was reverified at completion.
|
Thanks for the re-review. Both gaps were real; they are fixed in 7a6e1cb. 1. Compressed W weather read as motionConfirmed, and the gap was wider than 2. Refused Mic-E payloads exemptThe runner no longer exempts Mic-E. For a refused Mic-E row it derives the expected The stricter check exposed three library bugs, also fixed:
25 Mic-E rows have updated expectations as a result. Full suite: 7,836 passed plus 793 subtests; ruff, mypy, black and prettier clean. |
Summary
APRS packet behaviour is now tested by one data-driven suite: JSONL case vectors in
packages/aprs/tests/cases, checked by a single runner. The old corpus machinery and most packet-level Python tests are gone. Every packet row also records go-aprs-fap's output, and the differences are reviewed and explained in each row's comment. Running live APRS-IS traffic and FAP through the suite found several decoder and encoder problems, fixed here.Case vectors
id,comment,tnc2,policy,decoded,reencoded,encoding_error,fap(documented intests/cases/README.md). A row starts from received bytes (tnc2) or from a composed model (tnc2: null).traffic_sample.jsonl, a curated sample of a several-hour Pacific Northwest APRS-IS capture: the first two packets of each shape and software family.tests/test_cases.py) checks every row every way it can:-0.0versus0.0), and rebuilds the same value;serialize_packet,validate_packet,serialize_tnc2_line,check_packetandserialize_payloadall agree;tests/case_tool.py:propose [--write]shows rows whose expectations changed;fapfills in FAP snapshots;sweep CAPTURE [--fap BIN] [--add FILE]runs a raw capture through the parser, FAP and the runner, reporting failures, accept/reject differences from FAP, and new packet shapes.tests/fap_snapshot/: a small Go harness pinned to go-aprs-fap commit94c7118(the same commit as the original FAP snapshots, which it reproduces exactly).Differs from FAP: …,Differs from FAP (representation only): …,Decision: …andUnder this row's policy ….Library changes
Weather: only
000/000reads as unknown wind. A000direction with a non-zero speed is now sent as received; it used to be rewritten to360.Lenient decoding of usable but out-of-spec input. These now give a
Packetwith anonconformingdiagnostic instead ofMalformed:z(the timestamp is left out);Objects still require a valid timestamp.
Malformedresults now report a bad timestamp in their diagnostics. - Timestamps: an uppercaseZsuffix on a position or object is read asz, with aCompressed positions: PHG, DF signal, area and DF bearing text after a compressed position is now comment text (APRS101 p. 36: the compressed format "does not support PHG"). This matches what sending already refused.
/Wand\W: treated as weather symbols only when weather fields follow (peraprs.org/aprs11/spec-wx.txt). A bareddd/sssunderWis now course and speed.PARM/UNIT: trailing empty slots are no longer sent, so decoded definitions re-send as.
Removed
tests/vectors/,generate_vectors.py,source.py,roundtrip_exceptions.json,fap_divergences.json,sequence_packet_expectations.json, the corpus runners and contract tests, andtests/fixtures/.tespyandtests/fields/test_codecs.py.Testing
sweepover the full 31,294-packet capture finds no runner failures and no packet shapes missing from the sample.Known differences and open items
000/000as zero instead of unknown;$.payload.device_code;serialize_payloadon a Mic-E packet with anunknownmessage raisesConformanceEuiredError.