From 53915c9d2664b08b3ea653d9104f698b958e27be Mon Sep 17 00:00:00 2001 From: n1ckyb Date: Mon, 10 Aug 2026 07:59:10 +0100 Subject: [PATCH] test: sync two test files that never reached the publishing repo Both exist in the monorepo and have NEVER run in this repo's CI. ## test_rename_does_not_hide_behaviour.py (3 tests) The acceptance tests for intentumdiff-core#18: a rename that also changed the body was collapsed into a single REFACTORING, discarding the behavioural change and telling the reviewer nothing had changed. That is the worst output this engine can produce, and the guard against it has never executed here. Gated on the engine fix, which is verified present on core's RC (`carries_entity_name` in draft_suppressors.rs) before syncing - landing the test first would have arrived red. ## test_live_server_asset_diff.py (13 tests) Covers the live-server asset_diff op. Verified the op exists on this branch (5 references in live_server.py) before syncing, same reason. ## Held back deliberately test_uast_structural_facts.py is NOT in this commit. Its 4 tests need the early_exit_count / negated_condition_count / has_guard_clause fields on NodeFacts, which are still only on the unmerged fix/uast-facts-cross-boundary branch. Landing them now would put 4 red tests on the release branch. They follow once #26 merges. ## Verification note These cannot be validated from a bare clone: running them here raises PluginNotFoundError('unknown') because parser components are not staged outside CI - the condition tracked as #22. CI is the verification, which is the standing model for this repo (the monorepo has no CI at all). Found by auditing why the CI suite collects 84 fewer tests than the monorepo. Co-Authored-By: Claude Opus 5 --- tests/unit/test_live_server_asset_diff.py | 240 ++++++++++++++++++ .../test_rename_does_not_hide_behaviour.py | 79 ++++++ 2 files changed, 319 insertions(+) create mode 100644 tests/unit/test_live_server_asset_diff.py create mode 100644 tests/unit/test_rename_does_not_hide_behaviour.py diff --git a/tests/unit/test_live_server_asset_diff.py b/tests/unit/test_live_server_asset_diff.py new file mode 100644 index 0000000..d7c6807 --- /dev/null +++ b/tests/unit/test_live_server_asset_diff.py @@ -0,0 +1,240 @@ +"""Acceptance tests for the live-server ``asset_diff`` op (intentumdiff-vscode#25). + +The Rust core owns this behaviour and pins it with its own ``#[cfg(test)]`` tests; these +prove the guarantee survives the binding — that an editor sending a repo-relative path and a +ref really does get the engine's artifact manifest back over the protocol, rather than a +plausible-looking summary assembled on this side of the boundary. +""" +from __future__ import annotations + +import shutil +import struct +import subprocess +import zlib +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +# Reason string matches the skip ratchet's existing "platform" class (skip_reasons_baseline.json) +# rather than minting a new one — a new phrasing for an already-classified condition is exactly +# what that gate exists to stop. +pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="git not installed") + +# Every layer the perceptual viewer can render. The panel reported "perceptual diff pending" +# for as long as nothing asked the engine for these, so the test asserts all of them. +_ARTIFACT_LAYERS = ("before", "after", "diff", "heatmap", "mask", "overlay", "contact_sheet") + + +def _png_bytes(pixels: list[tuple[int, int, int]], width: int, height: int) -> bytes: + """A minimal RGB PNG, so the fixture needs no image library on the Python side.""" + raw = b"".join( + b"\x00" + b"".join(struct.pack("BBB", *pixels[y * width + x]) for x in range(width)) + for y in range(height) + ) + + def chunk(tag: bytes, data: bytes) -> bytes: + crc = zlib.crc32(tag + data) & 0xFFFFFFFF + return struct.pack(">I", len(data)) + tag + data + struct.pack(">I", crc) + + return ( + b"\x89PNG\r\n\x1a\n" + + chunk(b"IHDR", struct.pack(">IIBBBBB", width, height, 8, 2, 0, 0, 0)) + + chunk(b"IDAT", zlib.compress(raw)) + + chunk(b"IEND", b"") + ) + + +def _checkerboard(width: int, height: int) -> list[tuple[int, int, int]]: + return [ + (200, 40, 90) if (x // 4 + y // 4) % 2 else (30, 60, 140) + for y in range(height) + for x in range(width) + ] + + +def _git(repo: Path, *args: str) -> None: + subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + ) + + +@pytest.fixture() +def image_repo(tmp_path: Path) -> Path: + """A git repo whose committed image differs from its working-tree copy.""" + repo = tmp_path / "repo" + (repo / "assets").mkdir(parents=True) + card = repo / "assets" / "card.png" + width, height = 32, 32 + card.write_bytes(_png_bytes(_checkerboard(width, height), width, height)) + _git(repo, "init") + _git(repo, "config", "user.email", "intentumdiff@example.test") + _git(repo, "config", "user.name", "IntentumDiff Test") + _git(repo, "add", ".") + _git(repo, "commit", "-m", "base") + + changed = _checkerboard(width, height) + for y in range(8, 22): + for x in range(6, 20): + changed[y * width + x] = (250, 250, 40) + card.write_bytes(_png_bytes(changed, width, height)) + return repo + + +def _server(repo: Path, ref: str = "HEAD"): + from intentumdiff.live_server import LiveServer + + differ = MagicMock() + differ._config = MagicMock() + differ._registry = MagicMock() + return LiveServer(differ, repo_path=str(repo), ref=ref) + + +def _asset_diff(repo: Path, request: dict, ref: str = "HEAD") -> dict: + sent: list[dict] = [] + _server(repo, ref)._process_request({"op": "asset_diff", "seq": 7, **request}, sent.append) + assert len(sent) == 1, sent + return sent[0] + + +class TestAssetDiffOp: + def test_tracked_image_returns_the_engines_artifact_manifest(self, image_repo: Path) -> None: + response = _asset_diff(image_repo, {"path": "assets/card.png"}) + + assert response["ok"] is True + assert response["op"] == "asset_diff" + assert response["seq"] == 7 + result = response["result"] + assert result["status"] == "compared" + assert result["file_path"] == "assets/card.png" + + for layer in _ARTIFACT_LAYERS: + artifact = result["artifacts"].get(layer) + assert artifact, f"engine did not report the {layer} artifact" + assert Path(artifact).is_file(), f"{layer} announced at {artifact} but not written" + + # The overlay geometry the viewer draws is only meaningful with these three together. + assert result["comparison_dimensions"] == {"width": 32, "height": 32} + assert result["hotspots"], "a 14x14 block change should surface at least one hotspot" + assert result["histograms"]["red_delta"] + assert result["changed_pixel_percentage"] > 0 + + def test_artifacts_land_in_a_cache_that_ignores_itself(self, image_repo: Path) -> None: + response = _asset_diff(image_repo, {"path": "assets/card.png"}) + + cache = image_repo / ".intentumdiff-cache" + artifact = Path(response["result"]["artifacts"]["heatmap"]).resolve() + assert cache.resolve() in artifact.parents, "artifacts must not land in the work tree" + assert "*" in (cache / ".gitignore").read_text(encoding="utf-8") + + def test_added_image_is_reported_as_skipped_with_a_reason(self, image_repo: Path) -> None: + (image_repo / "assets" / "new.png").write_bytes(_png_bytes(_checkerboard(8, 8), 8, 8)) + + result = _asset_diff(image_repo, {"path": "assets/new.png"})["result"] + + # An added image has nothing to compare against. Saying so is the point: the state this + # replaced claimed a comparison was merely "pending" and never produced one. + assert result["status"] == "skipped" + assert result["change_type"] == "A" + assert "no before" in result["reason"] + assert not result.get("artifacts") + + def test_deleted_image_is_reported_as_skipped(self, image_repo: Path) -> None: + (image_repo / "assets" / "card.png").unlink() + + result = _asset_diff(image_repo, {"path": "assets/card.png"})["result"] + + assert result["status"] == "skipped" + assert result["change_type"] == "D" + + def test_unresolvable_ref_is_an_error_not_an_added_image(self, image_repo: Path) -> None: + response = _asset_diff(image_repo, {"path": "assets/card.png", "ref": "no-such-ref"}) + + assert response["ok"] is False + assert "git rev not found" in response["error"]["message"] + + @pytest.mark.parametrize( + "path", + ["../outside.png", "..\\outside.png", "assets/../../outside.png"], + ) + def test_path_escaping_the_served_repo_is_refused(self, image_repo: Path, path: str) -> None: + response = _asset_diff(image_repo, {"path": path}) + + assert response["ok"] is False + assert response["error"]["code"] == "invalid_request" + + def test_request_without_a_path_or_pair_is_refused(self, image_repo: Path) -> None: + response = _asset_diff(image_repo, {}) + + assert response["ok"] is False + assert response["error"]["code"] == "invalid_request" + + def test_explicit_before_after_paths_still_work(self, image_repo: Path) -> None: + before = image_repo / "assets" / "before.png" + after = image_repo / "assets" / "after.png" + pixels = _checkerboard(16, 16) + before.write_bytes(_png_bytes(pixels, 16, 16)) + changed = list(pixels) + for index in range(20, 60): + changed[index] = (255, 255, 255) + after.write_bytes(_png_bytes(changed, 16, 16)) + + result = _asset_diff( + image_repo, + {"before_path": "assets/before.png", "after_path": "assets/after.png"}, + )["result"] + + assert result["status"] == "compared" + assert result["artifacts"]["overlay"] + + def test_serving_a_subdirectory_still_resolves_the_base_blob(self, image_repo: Path) -> None: + # A monorepo subfolder opened as a workspace addresses files relative to itself; git + # addresses blobs from the repository root. + result = _asset_diff(image_repo / "assets", {"path": "card.png"})["result"] + + assert result["status"] == "compared" + assert result["file_path"] == "card.png" + + def test_a_users_own_cache_gitignore_is_left_alone(self, image_repo: Path) -> None: + _asset_diff(image_repo, {"path": "assets/card.png"}) + marker = image_repo / ".intentumdiff-cache" / ".gitignore" + marker.write_text("mine\n", encoding="utf-8") + + _asset_diff(image_repo, {"path": "assets/card.png"}) + + assert marker.read_text(encoding="utf-8") == "mine\n" + + def test_a_non_image_path_is_an_error_not_a_comparison(self, image_repo: Path) -> None: + (image_repo / "notes.txt").write_text("not an image", encoding="utf-8") + + response = _asset_diff(image_repo, {"path": "notes.txt"}) + + assert response["ok"] is False + assert "asset diff failed" in response["error"]["message"] + + +class TestAssetDiffContract: + """The op is served by BOTH live-servers from one core implementation. + + The Python server and the native binary (``crates/live-server``) call the same + ``live_handle_asset_diff``, so what they advertise and what they answer cannot drift apart — + which matters because the extension prefers the native binary whenever one is bundled, and + for a while that binary had no arm for this op at all. + """ + + def test_capabilities_advertise_the_asset_diff_op(self, image_repo: Path) -> None: + capabilities = _server(image_repo)._capabilities() + + assert "asset_diff" in capabilities["operations"] + + def test_the_response_comes_from_the_core_not_this_layer(self, image_repo: Path) -> None: + from intentumdiff import rust_core + + request = {"op": "asset_diff", "seq": 7, "path": "assets/card.png"} + expected = rust_core.live_handle_asset_diff(str(image_repo), "HEAD", request, 7) + served = _asset_diff(image_repo, {"path": "assets/card.png"}) + + # Same request, same engine, same content-addressed output -> identical answer. + assert served == expected diff --git a/tests/unit/test_rename_does_not_hide_behaviour.py b/tests/unit/test_rename_does_not_hide_behaviour.py new file mode 100644 index 0000000..075852b --- /dev/null +++ b/tests/unit/test_rename_does_not_hide_behaviour.py @@ -0,0 +1,79 @@ +"""A rename must never hide a behavioural change in the same function. + +# Why this exists + +Renaming a function while editing its body is one of the commonest shapes in real review — +extract, rename, adjust. The engine collapsed that into a single `REFACTORING` change and +discarded the body edits with the DELETION/ADDITION pair it replaced. + +`REFACTORING` means "structure changed, behaviour did not". So the tool was not merely missing +a change; it was *asserting the change was safe* in its own vocabulary, and a reviewer acting +on that label would skim past it. + +The guard that allowed it asked "does the function start on the same line?" and, if so, treated +the rename as compatible without ever comparing the bodies. Same start line is evidence of +nothing. + +See intentumdiff-core#18. The Rust half lives in `draft_suppressors.rs`; this is the +acceptance half proving it survives the binding. +""" + +from __future__ import annotations + +from intentumdiff import SemanticDiffer + +BEFORE = "def total(cart):\n return sum(i.price for i in cart)\n" + +RENAMED_ONLY = "def calculate_total(cart):\n return sum(i.price for i in cart)\n" + +# Totals over 100 now get 10% off. A reviewer MUST see this. +BODY_CHANGED = ( + "def total(cart):\n" + " subtotal = sum(i.price for i in cart)\n" + " if subtotal > 100:\n" + " return subtotal * 0.9\n" + " return subtotal\n" +) + +RENAMED_AND_BODY_CHANGED = ( + "def calculate_total(cart):\n" + " subtotal = sum(i.price for i in cart)\n" + " if subtotal > 100:\n" + " return subtotal * 0.9\n" + " return subtotal\n" +) + + +def _kinds(old: str, new: str) -> list[str]: + diff = SemanticDiffer().diff_strings(old, new, "billing.py") + return [str(c.change_type).rsplit(".", 1)[-1] for c in diff.changes] + + +def test_a_rename_with_a_changed_body_is_not_reported_as_refactoring() -> None: + """THE regression. + + Not "reports more changes" — the load-bearing assertion is that it does NOT claim + REFACTORING, because that label tells a reviewer the behaviour is unchanged. + """ + kinds = _kinds(BEFORE, RENAMED_AND_BODY_CHANGED) + assert "REFACTORING" not in kinds, ( + "A rename that also changed the body was labelled REFACTORING — which asserts " + f"behaviour did not change. The discount logic would be skimmed. Got: {kinds}" + ) + assert kinds, "a rename plus a behavioural change must report something" + + +def test_a_pure_rename_is_still_one_refactoring() -> None: + """The fix must not be 'disable rename detection'. + + A rename with an untouched body is exactly what REFACTORING is for, and collapsing it to + one change is the value the feature adds. + """ + assert _kinds(BEFORE, RENAMED_ONLY) == ["REFACTORING"] + + +def test_a_body_change_without_a_rename_is_unaffected() -> None: + """Control: the path that always worked must keep working.""" + kinds = _kinds(BEFORE, BODY_CHANGED) + assert "REFACTORING" not in kinds + assert len(kinds) >= 3, f"expected the inserted statements to surface, got {kinds}"