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 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()