From d09cd73dcccebacf1f2859651d1e59438c520ee6 Mon Sep 17 00:00:00 2001 From: Paul Molodowitch Date: Tue, 28 Apr 2026 05:59:26 -0700 Subject: [PATCH 1/2] 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 2/2] 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