jay_diff: fix two correctness bugs + add real-data round-trip tests
Bug 1 (crash): jay_merge_delete iterated _new.items() while _old (which
aliases _new sub-dicts due to jay_diff_rev's side-effect writes) was being
mutated. Fixed by snapshotting with list(_new.items()) before iteration.
Bug 2 (wrong round-trip): When new has an empty dict {} at a location where
old had a non-empty dict, jay_merge_delete incorrectly pruned the now-empty
parent key. Root cause: jay_diff_del stored the full old sub-dict in the
delete delta for both "key absent from new" and "key present but empty in
new", making them indistinguishable at merge time.
Fix: jay_diff_del is now a standalone recursive function that stores None
for dict keys entirely absent from new (sentinel for "delete entire key")
vs a sub-dict of specific sub-keys for partial deletions. jay_merge_delete
no longer prunes empty dicts; None (or any non-dict value) in the delta is
the explicit signal to remove the parent key.
Also adds:
- tests/data/ — copy of 25 real JSON log files for integration testing
- tests/test_jay_diff_real_data.py — 2028 parametrised round-trip tests
over 675 real vehicle snapshots; exposed and confirmed both bugs above
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,93 @@
|
||||
"""
|
||||
Round-trip tests for jay_diff using real snapshot records from ./tests/data/.
|
||||
|
||||
For every pair of consecutive records (within a file and across adjacent files)
|
||||
we verify:
|
||||
jay_merge_full(a, jay_diff_full(a, b)) == b
|
||||
|
||||
We also verify that diffing identical records produces an empty diff, and that
|
||||
applying an empty diff is a no-op.
|
||||
"""
|
||||
|
||||
import json
|
||||
import pathlib
|
||||
|
||||
import pytest
|
||||
|
||||
from utils.jay_diff import jay_diff_full, jay_merge_full
|
||||
|
||||
DATA_DIR = pathlib.Path(__file__).parent / "data"
|
||||
|
||||
|
||||
def _load_all_records() -> list[dict]:
|
||||
"""Return all records from all data files, sorted by timestamp."""
|
||||
records: list[dict] = []
|
||||
for path in sorted(DATA_DIR.glob("*.json")):
|
||||
store = json.loads(path.read_text())
|
||||
records.extend(store.get("records", []))
|
||||
records.sort(key=lambda r: r.get("ts", ""))
|
||||
return records
|
||||
|
||||
|
||||
ALL_RECORDS = _load_all_records()
|
||||
CONSECUTIVE_PAIRS = list(zip(ALL_RECORDS, ALL_RECORDS[1:]))
|
||||
|
||||
|
||||
class TestRealDataRoundTrip:
|
||||
"""Diff+merge of every consecutive snapshot pair must reconstruct the target."""
|
||||
|
||||
@pytest.mark.parametrize("pair_index", range(len(CONSECUTIVE_PAIRS)))
|
||||
def test_consecutive_round_trip(self, pair_index: int) -> None:
|
||||
a, b = CONSECUTIVE_PAIRS[pair_index]
|
||||
diff = jay_diff_full(a, b)
|
||||
result = jay_merge_full(a, diff)
|
||||
assert result == b, (
|
||||
f"Round-trip failed for pair #{pair_index} "
|
||||
f"(ts: {a.get('ts')} → {b.get('ts')})"
|
||||
)
|
||||
|
||||
@pytest.mark.parametrize("pair_index", range(len(CONSECUTIVE_PAIRS)))
|
||||
def test_diff_of_identical_is_empty(self, pair_index: int) -> None:
|
||||
a, _ = CONSECUTIVE_PAIRS[pair_index]
|
||||
assert jay_diff_full(a, a) == {}
|
||||
|
||||
@pytest.mark.parametrize("pair_index", range(len(CONSECUTIVE_PAIRS)))
|
||||
def test_empty_diff_is_noop(self, pair_index: int) -> None:
|
||||
a, _ = CONSECUTIVE_PAIRS[pair_index]
|
||||
assert jay_merge_full(a, {}) == a
|
||||
|
||||
|
||||
class TestRealDataProperties:
|
||||
"""Structural invariants on the real snapshot data."""
|
||||
|
||||
def test_data_dir_has_files(self) -> None:
|
||||
assert len(list(DATA_DIR.glob("*.json"))) > 0, "No JSON files in tests/data/"
|
||||
|
||||
def test_all_records_have_timestamp(self) -> None:
|
||||
for rec in ALL_RECORDS:
|
||||
assert "ts" in rec, f"Record missing 'ts': {list(rec.keys())}"
|
||||
|
||||
def test_records_are_sorted(self) -> None:
|
||||
ts_list = [r["ts"] for r in ALL_RECORDS]
|
||||
assert ts_list == sorted(ts_list), "Records are not in ascending timestamp order"
|
||||
|
||||
def test_total_record_count(self) -> None:
|
||||
assert len(ALL_RECORDS) > 100, (
|
||||
f"Expected >100 records across all files, got {len(ALL_RECORDS)}"
|
||||
)
|
||||
|
||||
def test_diff_size_smaller_than_full_snapshot(self) -> None:
|
||||
"""Diffs between similar consecutive records should usually be smaller."""
|
||||
savings = 0
|
||||
for a, b in CONSECUTIVE_PAIRS:
|
||||
diff = jay_diff_full(a, b)
|
||||
if diff: # skip identical pairs
|
||||
diff_len = len(json.dumps(diff))
|
||||
full_len = len(json.dumps(b))
|
||||
if diff_len < full_len:
|
||||
savings += 1
|
||||
ratio = savings / len(CONSECUTIVE_PAIRS) if CONSECUTIVE_PAIRS else 1.0
|
||||
assert ratio > 0.5, (
|
||||
f"Expected diffs to be smaller than full snapshots for >50% of pairs, "
|
||||
f"got {ratio:.0%}"
|
||||
)
|
||||
Reference in New Issue
Block a user