From 16b81ff26557dc0ee5576abf63be133d19290192 Mon Sep 17 00:00:00 2001 From: Paul Molodowitch Date: Tue, 28 Apr 2026 18:10:14 -0700 Subject: [PATCH 1/4] add a basic diff feature test-suite --- pyproject.toml | 9 +- tests/_ast_diff_helpers.py | 55 ++++++++++ tests/feature/README.md | 24 +++++ tests/feature/__init__.py | 0 tests/feature/test_ast_diff.py | 182 +++++++++++++++++++++++++++++++++ 5 files changed, 266 insertions(+), 4 deletions(-) create mode 100644 tests/_ast_diff_helpers.py create mode 100644 tests/feature/README.md create mode 100644 tests/feature/__init__.py create mode 100644 tests/feature/test_ast_diff.py diff --git a/pyproject.toml b/pyproject.toml index d3738ed..5e94158 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,13 +21,14 @@ doc_build = { path = ".", editable = true } [tool.pixi.tasks] build = "python tests/build_scripts/build_docs.py build" build-diff = "python tests/build_scripts/build_diff.py" +feature-test = "python -m unittest discover -s tests/feature -t ." [tool.pixi.tasks.test] - # Convenience command to run both build and build-diff - # ie, `pixi run test` = `pixi run build ; pixi run build-diff` + # Convenience command to run feature-test, build, and build-diff + # ie, `pixi run test` = `pixi run feature-test ; pixi run build ; pixi run build-diff` # Note that `test` / depends-on tasks cannot be passed extra arguments... # (see note below) - depends-on = ["build", "build-diff"] + depends-on = ["feature-test", "build", "build-diff"] # NOTE: this cmd is just to cause an error message to print if you try to pass args # to `pixi run test` @@ -36,7 +37,7 @@ build-diff = "python tests/build_scripts/build_diff.py" # pixi run test --keep-pdf-latexn # ...will seem to work, but actually the flag is just silently discarded. # This command causes the test task to fail in this scenario - cmd = "python -c 'import sys; sys.argv[1:] and sys.exit(\"test does not accept extra args; pass them directly to build or build-diff\")'" + cmd = "python -c 'import sys; sys.argv[1:] and sys.exit(\"test does not accept extra args; pass them directly to build, build-diff, or feature-test\")'" [tool.pixi.workspace] channels = [ diff --git a/tests/_ast_diff_helpers.py b/tests/_ast_diff_helpers.py new file mode 100644 index 0000000..8c70d98 --- /dev/null +++ b/tests/_ast_diff_helpers.py @@ -0,0 +1,55 @@ +"""Shared Pandoc AST helpers for ast_diff tests. + +Factored out for tests/feature/test_ast_diff.py and a soon-to-come +tests/regression/test_diff_ordering_with_duplicate_nodes.py. The functions +construct just enough of the real Pandoc AST shape for the ast_diff module +to walk over without going through pandoc itself. +""" + + +def _str(s): + return {"t": "Str", "c": s} + + +def _space(): + return {"t": "Space"} + + +def _inlines(text): + """Pandoc inline list from a plain string (single-space-separated words).""" + out = [] + for i, word in enumerate(text.split()): + if i: + out.append(_space()) + out.append(_str(word)) + return out + + +def header(level, ident, text): + return {"t": "Header", "c": [level, [ident, [], []], _inlines(text)]} + + +def para(text): + return {"t": "Para", "c": _inlines(text)} + + +def plain(text): + return {"t": "Plain", "c": _inlines(text)} + + +def bullet_list(*item_texts): + return {"t": "BulletList", "c": [[plain(t)] for t in item_texts]} + + +def diff_classes(node): + if node.get("t") == "Div": + return tuple(node["c"][0][1]) + return () + + +def block_kind(node): + """Short identifier for asserting on diff structure.""" + cls = diff_classes(node) + if cls: + return f"Div({'+'.join(cls)})" + return node.get("t", "?") diff --git a/tests/feature/README.md b/tests/feature/README.md new file mode 100644 index 0000000..2e6bef6 --- /dev/null +++ b/tests/feature/README.md @@ -0,0 +1,24 @@ +# Feature tests for `doc_build` + +Stdlib-`unittest` tests that exercise the public behavior of `doc_build` +modules end-to-end (within Python — they do not invoke pandoc). No +third-party test dependency. + +These are *not* strict unit tests — they call high-level entry points +(e.g. `diff_block_lists`) and let the call fan out through helpers like +`find_longest_common_subsequence`, `_pair_adjacent_changes`, and +`diff_list_nodes` rather than mocking them. + +Tests that *specifically* reproduce a previously fixed bug live under +[`tests/regression/`](../regression/) instead — those tests should be +expected to fail against the unfixed code. + +## Running + +From the repo root: + + pixi run python -m unittest discover -s tests/feature -t . + +Or run a single file directly: + + pixi run python tests/feature/test_ast_diff.py diff --git a/tests/feature/__init__.py b/tests/feature/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/feature/test_ast_diff.py b/tests/feature/test_ast_diff.py new file mode 100644 index 0000000..b1e110d --- /dev/null +++ b/tests/feature/test_ast_diff.py @@ -0,0 +1,182 @@ +#!/usr/bin/env python +"""Feature tests for doc_build.ast_diff. + +These tests construct minimal Pandoc-style block AST fragments and assert on +the structural shape of the diff output, without going through pandoc. They +exercise diff_block_lists end-to-end (no mocking of LCS / pairing helpers). +Tests that specifically reproduce a previously fixed bug live under +tests/regression/. +""" + +import sys +import unittest +from pathlib import Path + +# Make `doc_build` importable when run as a script (e.g. `python test_ast_diff.py`) +# as well as via `python -m unittest discover -s tests/feature`. +_REPO_ROOT = Path(__file__).resolve().parents[2] +_TESTS_DIR = _REPO_ROOT / "tests" +for _p in (_REPO_ROOT, _TESTS_DIR): + if str(_p) not in sys.path: + sys.path.insert(0, str(_p)) + +from doc_build.ast_diff import diff_block_lists # noqa: E402 + +from _ast_diff_helpers import ( # noqa: E402 + block_kind, + bullet_list, + diff_classes, + header, + para, +) + + +class TestDiffBlockListsBasics(unittest.TestCase): + """Sanity checks for the simple, no-duplicates path.""" + + def test_no_changes_returns_inputs_unwrapped(self): + before = [header(2, "h", "Title"), para("body text")] + after = [header(2, "h", "Title"), para("body text")] + merged = diff_block_lists(before, after) + self.assertEqual([block_kind(b) for b in merged], ["Header", "Para"]) + + def test_pure_insertion(self): + before = [header(2, "h", "Title")] + after = [header(2, "h", "Title"), para("new body")] + merged = diff_block_lists(before, after) + self.assertEqual( + [block_kind(b) for b in merged], ["Header", "Div(insertion)"] + ) + + def test_pure_deletion(self): + before = [header(2, "h", "Title"), para("old body")] + after = [header(2, "h", "Title")] + merged = diff_block_lists(before, after) + self.assertEqual( + [block_kind(b) for b in merged], ["Header", "Div(deletion)"] + ) + + def test_changed_para_becomes_substitution(self): + before = [header(2, "h", "Title"), para("old body")] + after = [header(2, "h", "Title"), para("new body")] + merged = diff_block_lists(before, after) + self.assertEqual( + [block_kind(b) for b in merged], ["Header", "Div(substitution)"] + ) + + def test_changed_bullet_list_returns_diffed_list(self): + before = [bullet_list("a", "b", "c")] + after = [bullet_list("a", "z", "c")] + merged = diff_block_lists(before, after) + self.assertEqual([block_kind(b) for b in merged], ["BulletList"]) + # The middle item should be a per-item substitution Div. + items = merged[0]["c"] + self.assertEqual(diff_classes(items[1][0]), ("substitution",)) + + +class TestDiffBlockListsStructuralShape(unittest.TestCase): + """Structural checks involving duplicates and drift. + + These cases exercise the same code paths that the LCS-ordering regression + test in tests/regression covers, but they happen to produce the same + output under both the buggy and fixed implementations, so they aren't + sufficient on their own to catch the bug. They serve as documentation + of the expected shape and as guard rails against unrelated regressions. + """ + + def test_duplicated_paragraphs_with_local_change_yields_substitution(self): + # Both 'Deprecated Fields' subsections share the SAME boilerplate + # paragraph ("These fields are reserved..."). The pandoc-style + # auto-numbered ids make the headings distinct. Only the *first* + # subsection's bullet list differs between before and after. + boiler = para("These fields are reserved though their usage is deprecated") + + before = [ + header(4, "deprecated-fields", "Deprecated Fields"), + boiler, + bullet_list("relocates", "permission"), + header(3, "property-spec", "Property Spec"), + header(4, "deprecated-fields-1", "Deprecated Fields"), + boiler, + bullet_list("foo", "bar"), + ] + after = [ + header(4, "deprecated-fields", "Deprecated Fields"), + boiler, + # added a leading item: + bullet_list("displayGroupOrder", "relocates", "permission"), + header(3, "property-spec", "Property Spec"), + header(4, "deprecated-fields-1", "Deprecated Fields"), + boiler, + bullet_list("foo", "bar"), + ] + + merged = diff_block_lists(before, after) + kinds = [block_kind(b) for b in merged] + + # Expected: heading, paragraph, (single diffed BulletList from + # diff_list_nodes), property-spec heading, second heading, paragraph, + # second (unchanged) BulletList. + self.assertEqual( + kinds, + [ + "Header", + "Para", + "BulletList", + "Header", + "Header", + "Para", + "BulletList", + ], + "boiler paragraph must remain between the heading and the diffed " + "bullet list, not be displaced by a separate insertion+deletion", + ) + + # The diffed BulletList should carry one item-level insertion. + diffed_list = merged[2] + items = diffed_list["c"] + item_kinds = [block_kind(item[0]) for item in items] + self.assertEqual( + item_kinds, + ["Div(insertion)", "Plain", "Plain"], + "first item should be an item-level insertion, others preserved", + ) + + def test_transposition_emits_separate_insertion_and_deletion(self): + # Pure transposition: 'X' moves from before slot 1 to after slot 2. + # LCS-based diffs cannot represent moves, so the canonical output is + # a separate deletion (at the old location) and insertion (at the + # new location), rather than a substitution. This is the same shape + # under both the buggy and fixed implementations - it documents that + # the fix did not change handling of transpositions. + a = header(2, "a", "A") + b = header(2, "b", "B") + x = header(2, "x", "X") + y = header(2, "y", "Y") + + before = [a, x, b, y] + after = [a, b, x, y] + + merged = diff_block_lists(before, after) + kinds = [block_kind(blk) for blk in merged] + + # The selected LCS (with the current tie-breaking) is [A, B, Y], so + # the unmatched X appears as a deletion before B and an insertion + # after B. Either is fine as long as one is a deletion, the other + # an insertion, and they straddle a preserved B. + self.assertEqual( + kinds, + [ + "Header", + "Div(deletion)", + "Header", + "Div(insertion)", + "Header", + ], + "transposition should emit a deletion+insertion pair around the " + "preserved element, not a substitution", + ) + + +if __name__ == "__main__": + unittest.main() From e068f83a0f804868021d69cff36bedef83eedaef Mon Sep 17 00:00:00 2001 From: Paul Molodowitch Date: Tue, 28 Apr 2026 06:44:07 -0700 Subject: [PATCH 2/4] Generalize _merge_with_lcs and _pair_adjacent_changes - Extract the LCS-driven walk that was inlined inside diff_block_lists (and duplicated inside diff_list_nodes) into a single generic _merge_with_lcs that returns an ordered (op, element) stream. - Generalize _pair_adjacent_changes to take pair / wrap_deletion / wrap_insertion callbacks so it can fold the same stream for both Pandoc blocks and list items, instead of operating on already-wrapped Divs. - Add _pair_blocks helper carrying the per-pair recursion strategy (lists / BlockQuotes / LineBlocks) used by diff_block_lists. - Reduce diff_list_nodes from a hand-written merge+pair loop to a thin call into the shared helpers. --- doc_build/ast_diff.py | 251 +++++++++++++++++++----------------------- 1 file changed, 116 insertions(+), 135 deletions(-) diff --git a/doc_build/ast_diff.py b/doc_build/ast_diff.py index da4d7b7..dc18c3d 100644 --- a/doc_build/ast_diff.py +++ b/doc_build/ast_diff.py @@ -17,6 +17,7 @@ import json +from collections.abc import Callable from typing import List, Dict, Any, Optional, Tuple from doc_build.filters.shared_filter_utils import HASH_ATTR_KEY @@ -184,63 +185,74 @@ def find_longest_common_subsequence(list_a: NodeList, list_b: NodeList) -> NodeL return dp[m][n] -def _pair_adjacent_changes(blocks: NodeList) -> NodeList: - """Pair adjacent deletion+insertion runs into substitution Divs. - - The LCS-based diff emits all deletions before all insertions within a - changed section (because nodes not in the LCS are drained from 'before' - first). This pass pairs them 1-to-1 as substitution Divs so the render - filter can produce per-word inline diffs. - - Excess deletions or insertions (when counts differ) remain as-is. +def _pair_adjacent_changes( + raw: List[Tuple[str, Any]], + *, + pair: Callable[[Any, Any], List[Any]], + wrap_deletion: Callable[[Any], Any], + wrap_insertion: Callable[[Any], Any], +) -> List[Any]: + """Walk an ``(op, element)`` stream from `_merge_with_lcs` and fold + adjacent deletion+insertion runs into per-pair recursive diffs (or + substitutions). + + ``"equal"`` elements pass through unchanged. Each contiguous deletion run + is gathered together with its immediately following insertion run; pairs + are converted via ``pair(deletion, insertion)``, which returns a (possibly + empty or multi-element) list of result elements to splice in. Excess + unpaired deletions / insertions are emitted via ``wrap_deletion`` / + ``wrap_insertion`` respectively. + + Generic over element type: callers pass blocks (``PandocNode``) or list + items (``List[PandocNode]``) and supply matching callbacks. The LCS-based + diff in `_merge_with_lcs` always emits deletions before insertions within + each gap, which is what this function expects. """ - - def _div_classes(block: PandocNode) -> List[str]: - if block.get("t") == "Div" and block.get("c"): - return block["c"][0][1] - return [] - - result: NodeList = [] + result: List[Any] = [] i = 0 - while i < len(blocks): - if "deletion" not in _div_classes(blocks[i]): - result.append(blocks[i]) + while i < len(raw): + op, element = raw[i] + if op == "equal": + result.append(element) i += 1 continue - - # Collect a consecutive run of deletions. - deletions: NodeList = [] - while i < len(blocks) and "deletion" in _div_classes(blocks[i]): - deletions.append(blocks[i]["c"][1][0]) # unwrap inner block + if op == "insertion": + # Insertion not preceded by a deletion run: emit bare. + result.append(wrap_insertion(element)) i += 1 - - # Collect the immediately following run of insertions. - insertions: NodeList = [] - while i < len(blocks) and "insertion" in _div_classes(blocks[i]): - insertions.append(blocks[i]["c"][1][0]) # unwrap inner block + continue + # op == "deletion". Gather the deletion run, then any immediately + # following insertion run. + deletions: List[Any] = [] + while i < len(raw) and raw[i][0] == "deletion": + deletions.append(raw[i][1]) + i += 1 + insertions: List[Any] = [] + while i < len(raw) and raw[i][0] == "insertion": + insertions.append(raw[i][1]) i += 1 - - # Pair 1-to-1 as substitutions; excess remain as bare deletions/insertions. n_pairs = min(len(deletions), len(insertions)) for j in range(n_pairs): - d, ins = deletions[j], insertions[j] - if _is_list_node(d) and _is_list_node(ins) and d.get("t") == ins.get("t"): - result.append(diff_list_nodes(d, ins)) - elif d.get("t") == "BlockQuote" and ins.get("t") == "BlockQuote": - result.append(diff_block_quote_nodes(d, ins)) - elif d.get("t") == "LineBlock" and ins.get("t") == "LineBlock": - result.extend(diff_line_block_nodes(d, ins)) - else: - extra_kv = _image_substitution_kv(d, ins) - result.append(make_substitution_div(d, ins, extra_kv=extra_kv)) - for node in deletions[n_pairs:]: - result.append(add_diff_meta(node, "deletion")) - for node in insertions[n_pairs:]: - result.append(add_diff_meta(node, "insertion")) - + result.extend(pair(deletions[j], insertions[j])) + for d in deletions[n_pairs:]: + result.append(wrap_deletion(d)) + for ins in insertions[n_pairs:]: + result.append(wrap_insertion(ins)) return result +def _pair_blocks(d: PandocNode, ins: PandocNode) -> NodeList: + """Pair strategy for `diff_block_lists`: recurse for like containers, + otherwise produce a substitution Div.""" + if _is_list_node(d) and _is_list_node(ins) and d.get("t") == ins.get("t"): + return [diff_list_nodes(d, ins)] + if d.get("t") == "BlockQuote" and ins.get("t") == "BlockQuote": + return [diff_block_quote_nodes(d, ins)] + if d.get("t") == "LineBlock" and ins.get("t") == "LineBlock": + return diff_line_block_nodes(d, ins) + return [make_substitution_div(d, ins, extra_kv=_image_substitution_kv(d, ins))] + + LIST_TYPES = frozenset({"BulletList", "OrderedList"}) @@ -287,69 +299,18 @@ def diff_list_nodes(old_node: PandocNode, new_node: PandocNode) -> PandocNode: old_items = _get_list_items(old_node) new_items = _get_list_items(new_node) - # find_longest_common_subsequence serializes each element via json.dumps, so - # it works on list items (List[PandocNode]) just as well as on PandocNode. + # find_longest_common_subsequence and _merge_with_lcs both serialize each + # element via json.dumps, so they work on list items (List[PandocNode]) + # just as well as on PandocNode. lcs = find_longest_common_subsequence(old_items, new_items) # type: ignore - lcs_strs = {json.dumps(item, sort_keys=True) for item in lcs} - - # Walk like diff_block_lists to produce an ordered stream of (op, item) pairs. - raw: List[Tuple[str, List[PandocNode]]] = [] - ptr_a, ptr_b = 0, 0 - while ptr_a < len(old_items) or ptr_b < len(new_items): - a = old_items[ptr_a] if ptr_a < len(old_items) else None - b = new_items[ptr_b] if ptr_b < len(new_items) else None - a_str = json.dumps(a, sort_keys=True) if a is not None else None - b_str = json.dumps(b, sort_keys=True) if b is not None else None - - if a is not None and a_str not in lcs_strs: - raw.append(("deletion", a)) - ptr_a += 1 - elif b is not None and b_str not in lcs_strs: - raw.append(("insertion", b)) - ptr_b += 1 - elif a is not None and b is not None: - raw.append(("equal", a)) - ptr_a += 1 - ptr_b += 1 - elif ptr_a < len(old_items): - raw.append(("deletion", old_items[ptr_a])) - ptr_a += 1 - else: - raw.append(("insertion", new_items[ptr_b])) - ptr_b += 1 - - # Pair consecutive deletion+insertion runs into substitution items. - result_items: List[List[PandocNode]] = [] - i = 0 - while i < len(raw): - op, item = raw[i] - if op == "equal": - result_items.append(item) - i += 1 - continue - if op == "insertion": - result_items.append([add_diff_meta(_item_to_block(item), "insertion")]) - i += 1 - continue - - # Collect a run of deletions then the immediately following insertions. - deletions: List[List[PandocNode]] = [] - while i < len(raw) and raw[i][0] == "deletion": - deletions.append(raw[i][1]) - i += 1 - insertions: List[List[PandocNode]] = [] - while i < len(raw) and raw[i][0] == "insertion": - insertions.append(raw[i][1]) - i += 1 - - n_pairs = min(len(deletions), len(insertions)) - for j in range(n_pairs): - result_items.append(diff_block_lists(deletions[j], insertions[j])) - for del_item in deletions[n_pairs:]: - result_items.append([add_diff_meta(_item_to_block(del_item), "deletion")]) - for ins_item in insertions[n_pairs:]: - result_items.append([add_diff_meta(_item_to_block(ins_item), "insertion")]) - + raw = _merge_with_lcs(old_items, new_items, lcs) # type: ignore + + result_items = _pair_adjacent_changes( + raw, + pair=lambda d, ins: [diff_block_lists(d, ins)], + wrap_deletion=lambda item: [add_diff_meta(_item_to_block(item), "deletion")], + wrap_insertion=lambda item: [add_diff_meta(_item_to_block(item), "insertion")], + ) return _build_list_with_items(old_node, result_items) @@ -397,41 +358,61 @@ def diff_block_lists(before_blocks: NodeList, after_blocks: NodeList) -> NodeLis groups as substitution Divs for per-word inline diffing. """ lcs_nodes = find_longest_common_subsequence(before_blocks, after_blocks) - lcs_set = {json.dumps(node, sort_keys=True) for node in lcs_nodes} - - merged_blocks: NodeList = [] - - ptr_a, ptr_b = 0, 0 - while ptr_a < len(before_blocks) or ptr_b < len(after_blocks): - node_a = before_blocks[ptr_a] if ptr_a < len(before_blocks) else None - node_b = after_blocks[ptr_b] if ptr_b < len(after_blocks) else None + raw = _merge_with_lcs(before_blocks, after_blocks, lcs_nodes) + return _pair_adjacent_changes( + raw, + pair=_pair_blocks, + wrap_deletion=lambda b: add_diff_meta(b, "deletion"), + wrap_insertion=lambda b: add_diff_meta(b, "insertion"), + ) + + +def _merge_with_lcs( + before: List[Any], + after: List[Any], + lcs_elements: List[Any], +) -> List[Tuple[str, Any]]: + """Walk `before` and `after` in lockstep with the LCS, yielding an ordered + stream of ``(op, element)`` tuples in document order. + + `op` is one of ``"equal"``, ``"deletion"`` or ``"insertion"``. Elements + not in the LCS are emitted as deletions (from `before`) or insertions + (from `after`); LCS elements are emitted as ``"equal"``. Within each + changed gap, deletions are emitted before insertions, which is what + `_pair_adjacent_changes` needs to fold them into substitution-style + annotations. + + Elements may be Pandoc blocks (``PandocNode``) or list items + (``List[PandocNode]``); the only requirement is that ``json.dumps(element, + sort_keys=True)`` gives a stable equality key. + """ + lcs_set = {json.dumps(n, sort_keys=True) for n in lcs_elements} - node_a_str = json.dumps(node_a, sort_keys=True) if node_a else None - node_b_str = json.dumps(node_b, sort_keys=True) if node_b else None + raw: List[Tuple[str, Any]] = [] + ptr_a = ptr_b = 0 + while ptr_a < len(before) or ptr_b < len(after): + a = before[ptr_a] if ptr_a < len(before) else None + b = after[ptr_b] if ptr_b < len(after) else None + a_str = json.dumps(a, sort_keys=True) if a is not None else None + b_str = json.dumps(b, sort_keys=True) if b is not None else None - if node_a and node_a_str not in lcs_set: - # This node from 'before' is not in the LCS, so it was removed. - merged_blocks.append(add_diff_meta(node_a, "deletion")) + if a is not None and a_str not in lcs_set: + raw.append(("deletion", a)) ptr_a += 1 - elif node_b and node_b_str not in lcs_set: - # This node from 'after' is not in the LCS, so it was added. - merged_blocks.append(add_diff_meta(node_b, "insertion")) + elif b is not None and b_str not in lcs_set: + raw.append(("insertion", b)) ptr_b += 1 - elif node_a and node_b: - # Both nodes are in the LCS (and therefore identical). - merged_blocks.append(node_a) + elif a is not None and b is not None: + raw.append(("equal", a)) ptr_a += 1 ptr_b += 1 - elif ptr_a < len(before_blocks): - # Exhausted 'after_blocks', remaining 'before' blocks are removals. - merged_blocks.append(add_diff_meta(before_blocks[ptr_a], "deletion")) + elif ptr_a < len(before): + raw.append(("deletion", before[ptr_a])) ptr_a += 1 - elif ptr_b < len(after_blocks): - # Exhausted 'before_blocks', remaining 'after' blocks are additions. - merged_blocks.append(add_diff_meta(after_blocks[ptr_b], "insertion")) + else: + raw.append(("insertion", after[ptr_b])) ptr_b += 1 - - return _pair_adjacent_changes(merged_blocks) + return raw def diff_ast_files(before_path, after_path, output_path): From d09cd73dcccebacf1f2859651d1e59438c520ee6 Mon Sep 17 00:00:00 2001 From: Paul Molodowitch Date: Tue, 28 Apr 2026 05:59:26 -0700 Subject: [PATCH 3/4] diff: add regression test for duplicate nodes causing order problems ie, a before with: - - and after with: - - would sometimes result in: - > - - > ...instead of - - > - > This would make it seem that the new version moved the unchanged node after the altered node, when it didn't. Additionally, it prevented the altered node from being detected as a substitution. --- pyproject.toml | 9 +- tests/_ast_diff_helpers.py | 2 +- tests/regression/README.md | 19 +++++ tests/regression/__init__.py | 0 ...test_diff_ordering_with_duplicate_nodes.py | 82 +++++++++++++++++++ 5 files changed, 107 insertions(+), 5 deletions(-) create mode 100644 tests/regression/README.md create mode 100644 tests/regression/__init__.py create mode 100644 tests/regression/test_diff_ordering_with_duplicate_nodes.py diff --git a/pyproject.toml b/pyproject.toml index 5e94158..81bc1bf 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,13 +22,14 @@ doc_build = { path = ".", editable = true } build = "python tests/build_scripts/build_docs.py build" build-diff = "python tests/build_scripts/build_diff.py" feature-test = "python -m unittest discover -s tests/feature -t ." +regression-test = "python -m unittest discover -s tests/regression -t ." [tool.pixi.tasks.test] - # Convenience command to run feature-test, build, and build-diff - # ie, `pixi run test` = `pixi run feature-test ; pixi run build ; pixi run build-diff` + # Convenience command to run feature-test, regression-test, build, and build-diff + # ie, `pixi run test` = `pixi run feature-test ; pixi run regression-test ; pixi run build ; pixi run build-diff` # Note that `test` / depends-on tasks cannot be passed extra arguments... # (see note below) - depends-on = ["feature-test", "build", "build-diff"] + depends-on = ["feature-test", "regression-test", "build", "build-diff"] # NOTE: this cmd is just to cause an error message to print if you try to pass args # to `pixi run test` @@ -37,7 +38,7 @@ feature-test = "python -m unittest discover -s tests/feature -t ." # pixi run test --keep-pdf-latexn # ...will seem to work, but actually the flag is just silently discarded. # This command causes the test task to fail in this scenario - cmd = "python -c 'import sys; sys.argv[1:] and sys.exit(\"test does not accept extra args; pass them directly to build, build-diff, or feature-test\")'" + cmd = "python -c 'import sys; sys.argv[1:] and sys.exit(\"test does not accept extra args; pass them directly to build, build-diff, feature-test, or regression-test\")'" [tool.pixi.workspace] channels = [ diff --git a/tests/_ast_diff_helpers.py b/tests/_ast_diff_helpers.py index 8c70d98..d42ecd9 100644 --- a/tests/_ast_diff_helpers.py +++ b/tests/_ast_diff_helpers.py @@ -1,6 +1,6 @@ """Shared Pandoc AST helpers for ast_diff tests. -Factored out for tests/feature/test_ast_diff.py and a soon-to-come +Used by tests/feature/test_ast_diff.py and tests/regression/test_diff_ordering_with_duplicate_nodes.py. The functions construct just enough of the real Pandoc AST shape for the ast_diff module to walk over without going through pandoc itself. diff --git a/tests/regression/README.md b/tests/regression/README.md new file mode 100644 index 0000000..7ff60ae --- /dev/null +++ b/tests/regression/README.md @@ -0,0 +1,19 @@ +# Regression tests for `doc_build` + +Stdlib-`unittest` regression tests for the Python modules in `doc_build/`. +No third-party test dependency. + +Each test here corresponds to a specific bug that was fixed; the docstring +should describe the bug and the failure mode it would reproduce against the +unfixed code. Add a new test file (or test case) per fixed bug, rather than +rewriting existing ones. + +## Running + +From the repo root: + + pixi run python -m unittest discover -s tests/regression -t . + +Or run a single file directly: + + pixi run python tests/regression/test_diff_ordering_with_duplicate_nodes.py diff --git a/tests/regression/__init__.py b/tests/regression/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/regression/test_diff_ordering_with_duplicate_nodes.py b/tests/regression/test_diff_ordering_with_duplicate_nodes.py new file mode 100644 index 0000000..0011903 --- /dev/null +++ b/tests/regression/test_diff_ordering_with_duplicate_nodes.py @@ -0,0 +1,82 @@ +#!/usr/bin/env python +"""Regression tests for doc_build.ast_diff. + +Each test here corresponds to a specific bug that was fixed and should be +expected to FAIL when run against the unfixed code. Tests that document +normal behavior or structural shape (and pass against both fixed and unfixed +code) belong under tests/unit/ instead. +""" + +import sys +import unittest +from pathlib import Path + +# Make `doc_build` and the shared test helpers importable when run as a +# script (e.g. `python test_diff_ordering_with_duplicate_nodes.py`) as well +# as via `python -m unittest discover -s tests/regression`. +_REPO_ROOT = Path(__file__).resolve().parents[2] +_TESTS_DIR = _REPO_ROOT / "tests" +for _p in (_REPO_ROOT, _TESTS_DIR): + if str(_p) not in sys.path: + sys.path.insert(0, str(_p)) + +from doc_build.ast_diff import diff_block_lists # noqa: E402 + +from _ast_diff_helpers import block_kind, header # noqa: E402 + + +class TestDiffBlockListsLcsOrderingRegression(unittest.TestCase): + """Regression tests for the bug where lcs_set membership lost positional + info, causing two pointers that were each 'in the LCS' but at different + positions to be treated as a matched pair. + + The most visible symptom in the spec build was a section like: + + #### Deprecated Fields + These fields are reserved... + - + + rendering as: + + #### Deprecated Fields + [INSERTED bullet list] + These fields are reserved... + [REMOVED bullet list] + + instead of the expected substitution between the two lists. + """ + + def test_extra_duplicate_does_not_eat_following_lcs_block(self): + # Minimal direct repro of the lcs_set-vs-lcs_list bug. 'X' appears + # 3 times in 'before' and 2 times in 'after'. The LCS uses two of + # the X instances (the 1st and 3rd in 'before'), leaving the middle + # one as a deletion. But the buggy algorithm checked "X in lcs_set" + # rather than "X is the next LCS element here", so the surplus X + # was treated as preserved, forcing the next LCS element ('C' in + # 'after') to be falsely reported as a deletion. + x = header(2, "x", "X") + a = header(2, "a", "A") + b = header(2, "b", "B") + c = header(2, "c", "C") + + before = [x, a, x, b, x, c] + after = [x, a, x, c] + + merged = diff_block_lists(before, after) + kinds = [block_kind(blk) for blk in merged] + + # Expected: [X, A, X, del(B), del(X), C] -- the surplus X is a + # deletion, B is a deletion, and C is preserved. + # Buggy output was [X, A, X, del(B), X, del(C)] (C falsely deleted). + self.assertEqual( + kinds, + ["Header", "Header", "Header", "Div(deletion)", "Div(deletion)", "Header"], + "the surplus duplicate X should be reported as a deletion, " + "not allowed to consume the following preserved element", + ) + # And specifically, C must remain preserved, not deleted. + self.assertEqual(merged[-1], c) + + +if __name__ == "__main__": + unittest.main() From 7b165d1f069968e57d24da245e8fc07e3de65f66 Mon Sep 17 00:00:00 2001 From: Paul Molodowitch Date: Tue, 28 Apr 2026 06:23:58 -0700 Subject: [PATCH 4/4] diff: walk LCS as ordered list, not set, to fix block ordering diff_block_lists and diff_list_nodes previously checked LCS membership via set lookup, then assumed two pointers each "in the LCS" must be at the same LCS position. With duplicate nodes (or earlier pointer drift) the two pointers could be at different LCS elements; pairing them advanced both ptrs across mismatched positions and produced merged output where deletions and insertions were no longer adjacent for _pair_adjacent_changes to fold into substitutions. Drive the merge from the LCS list in order: at each LCS element drain non-LCS blocks from 'before' as deletions and from 'after' as insertions, then take the LCS block. Same shape applied to diff_list_nodes. Co-Authored-By: Claude Opus 4.7 --- doc_build/ast_diff.py | 46 +++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/doc_build/ast_diff.py b/doc_build/ast_diff.py index dc18c3d..b250d84 100644 --- a/doc_build/ast_diff.py +++ b/doc_build/ast_diff.py @@ -382,36 +382,44 @@ def _merge_with_lcs( `_pair_adjacent_changes` needs to fold them into substitution-style annotations. + The LCS is consumed as an *ordered* sequence: at each LCS element we drain + every preceding non-LCS element from `before` as a deletion, then every + preceding non-LCS element from `after` as an insertion, then take the LCS + element itself. A set-membership check on the LCS would be incorrect: + with duplicate elements (or after earlier drift between the two pointers) + two different elements can each be 'in the LCS' while not being the same + LCS element, and pairing them would advance both pointers across + mismatched positions. + Elements may be Pandoc blocks (``PandocNode``) or list items (``List[PandocNode]``); the only requirement is that ``json.dumps(element, sort_keys=True)`` gives a stable equality key. """ - lcs_set = {json.dumps(n, sort_keys=True) for n in lcs_elements} + before_strs = [json.dumps(n, sort_keys=True) for n in before] + after_strs = [json.dumps(n, sort_keys=True) for n in after] + lcs_strs = [json.dumps(n, sort_keys=True) for n in lcs_elements] raw: List[Tuple[str, Any]] = [] ptr_a = ptr_b = 0 - while ptr_a < len(before) or ptr_b < len(after): - a = before[ptr_a] if ptr_a < len(before) else None - b = after[ptr_b] if ptr_b < len(after) else None - a_str = json.dumps(a, sort_keys=True) if a is not None else None - b_str = json.dumps(b, sort_keys=True) if b is not None else None - - if a is not None and a_str not in lcs_set: - raw.append(("deletion", a)) - ptr_a += 1 - elif b is not None and b_str not in lcs_set: - raw.append(("insertion", b)) - ptr_b += 1 - elif a is not None and b is not None: - raw.append(("equal", a)) - ptr_a += 1 - ptr_b += 1 - elif ptr_a < len(before): + for target in lcs_strs: + while ptr_a < len(before) and before_strs[ptr_a] != target: raw.append(("deletion", before[ptr_a])) ptr_a += 1 - else: + while ptr_b < len(after) and after_strs[ptr_b] != target: raw.append(("insertion", after[ptr_b])) ptr_b += 1 + # Both pointers are now at an instance of the current LCS element. + raw.append(("equal", before[ptr_a])) + ptr_a += 1 + ptr_b += 1 + + while ptr_a < len(before): + raw.append(("deletion", before[ptr_a])) + ptr_a += 1 + while ptr_b < len(after): + raw.append(("insertion", after[ptr_b])) + ptr_b += 1 + return raw