From 454eb3f973d0c756dd02b51ce51d64d87381ca0f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 28 Aug 2026 14:42:42 -0400 Subject: [PATCH] fix(tests): artifact identity tests talk to artifacts.sh, not to a sibling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 4746 failed lint and pytest on the same cause: `from test_artifact_paths import ROOT, declared_paths`. No other test module in this repo imports another, so that was a new convention introduced for no gain — and the wrong one, since `tests/` is a package and the bare name does not resolve. Everything now goes through `artifacts.sh`, which is the interface build.yml actually calls. The tests exercise the contract rather than a Python re-implementation of it, and the duplicate `declared_paths` helper is gone rather than copied. Two real defects found while fixing it: The newest commit is now computed by committer TIME, matching what artifacts.sh means. It was `git log -1`, whose default order is reverse-chronological only within topological constraints — so on a merged history it can name a different commit than the newest timestamp does. Both agree on this repo today (verified across all four artifacts), which is exactly what makes it a flake waiting for the branch shape that separates them. The third test asserted the same invariant as the first in different words. Removed rather than left as apparent coverage. --- tests/test_artifact_identity.py | 89 ++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 41 deletions(-) diff --git a/tests/test_artifact_identity.py b/tests/test_artifact_identity.py index 04b3a6a..e234ab2 100644 --- a/tests/test_artifact_identity.py +++ b/tests/test_artifact_identity.py @@ -29,10 +29,11 @@ from __future__ import annotations import re import subprocess +from pathlib import Path import pytest -from test_artifact_paths import ROOT, declared_paths +ROOT = Path(__file__).resolve().parent.parent ARTIFACTS = ("web", "ml", "agent", "extension") @@ -40,35 +41,61 @@ ARTIFACTS = ("web", "ml", "agent", "extension") _REVISION = re.compile(r"^[0-9a-f]{12}$") -def revision(artifact: str) -> str: +# Everything here goes through artifacts.sh rather than importing a sibling +# test module. That is the interface build.yml actually calls, so the tests +# exercise the contract instead of a Python re-implementation of it — and no +# other test module in this repo imports another, so a cross-test import would +# be a new convention introduced for no gain. +def artifacts(*args: str) -> str: return subprocess.run( - ["sh", str(ROOT / "scripts" / "artifacts.sh"), "revision", artifact], + ["sh", str(ROOT / "scripts" / "artifacts.sh"), *args], capture_output=True, text=True, check=True, cwd=ROOT, - ).stdout.strip() + ).stdout + + +def revision(artifact: str) -> str: + return artifacts("revision", artifact).strip() + + +def newest_by_commit_time(artifact: str) -> str: + """The full SHA of the newest commit touching this artifact's shipped set. + + Ordered by committer TIME, matching what artifacts.sh means. Deliberately + not `git log -1`: git's default order is reverse-chronological only within + topological constraints, so on a merged history it can name a different + commit than the newest timestamp does. They agree on this repo today, and + a test that silently depends on them continuing to agree would be a flake + waiting for the branch shape that separates them. + """ + paths = artifacts("paths", artifact).split() + log = subprocess.run( + ["git", "log", "--format=%ct %H", "HEAD", "--", *paths], + capture_output=True, text=True, check=True, cwd=ROOT, + ).stdout.split("\n") + commits = [line.split(" ", 1) for line in log if line.strip()] + assert commits, ( + f"no commit in this history touches the {artifact} path set — the " + f"derivation has nothing to stand on" + ) + return max(commits, key=lambda c: int(c[0]))[1] @pytest.mark.parametrize("artifact", ARTIFACTS) def test_revision_is_the_commit_its_own_shipped_files_last_changed_in(artifact): """The claim the whole skip decision rests on. - Asked of git directly rather than of the script, so this fails if the - derivation ever stops meaning what it says — deriving from HEAD, from a - build clock, or from a path set it did not actually use. + Computed from git rather than asked of the script, so it fails if the + derivation ever stops meaning what it says — switching to HEAD, to a build + clock, or to a path set it did not actually use. Each of those still + produces a plausible 12-hex value, which is why this is worth asserting + rather than eyeballing. """ - paths = declared_paths(artifact) - expected = subprocess.run( - ["git", "log", "--format=%H", "-1", "HEAD", "--", *paths], - capture_output=True, text=True, check=True, cwd=ROOT, - ).stdout.strip() - - assert expected, ( - f"no commit in this history touches the {artifact} path set — the " - f"derivation has nothing to stand on" - ) - assert expected.startswith(revision(artifact)), ( - f"{artifact} derives {revision(artifact)!r}, but the newest commit " - f"touching its shipped files is {expected[:12]!r}. The label stamped " - f"into the image would not identify its own content." + expected = newest_by_commit_time(artifact) + got = revision(artifact) + assert expected.startswith(got), ( + f"{artifact} derives {got!r}, but the newest commit touching its " + f"shipped files is {expected[:12]!r}. The label stamped into the image " + f"would not identify its own content." ) @@ -80,23 +107,3 @@ def test_revision_is_a_legal_label_value_and_is_stable(artifact): first = revision(artifact) assert _REVISION.match(first), f"{first!r} is not a 12-char hex revision" assert first == revision(artifact), "revision is not stable across calls" - - -def test_an_artifact_whose_paths_did_not_change_keeps_its_revision(): - """The property that makes skipping possible at all. - - The agent's set is disjoint from web's, so the two must be free to differ. - Asserting they *are* different today would pin an accident of history — - what matters is that the derivation is per-artifact rather than global, so - this asserts each artifact's revision is drawn from its own path set. - """ - seen = {a: revision(a) for a in ARTIFACTS} - for artifact, rev in seen.items(): - touched = subprocess.run( - ["git", "log", "--format=%H", "-1", "HEAD", "--", *declared_paths(artifact)], - capture_output=True, text=True, check=True, cwd=ROOT, - ).stdout.strip() - assert touched.startswith(rev), ( - f"{artifact}'s revision {rev!r} is not the newest commit touching " - f"its own paths — the derivation is not per-artifact" - )