From a7941a8eca6e75eaf702bfb248ffb7b082ec32ef Mon Sep 17 00:00:00 2001 From: rdbtCVS <87152661+rdbtCVS@users.noreply.github.com> Date: Wed, 21 Oct 2026 00:56:37 +0100 Subject: [PATCH] Scope PR validation to affected crates and avoid unrelated example builds --- CONTRIBUTING.md | 5 ++ tests/test_pr_ready.py | 101 ++++++++++++++++++++++++++++++++++++++++- tools/pr_ready.py | 59 ++++++++++++++++++++---- 3 files changed, 154 insertions(+), 11 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 16cb9d86f..af44d89f8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -53,6 +53,11 @@ crates you touched (and the launcher's), generated files, `Cargo.lock`, the Pyth your change can affect. Without the vanilla data on your machine it lists those harnesses instead; CI runs them. `--quick` skips the harnesses. It exits non-zero when something fails. +For the game workspace, Clippy checks all targets of the touched crates and their transitive workspace dependents, +including examples. The dependency scope includes optional, platform, build and development dependencies. +Tests run library, binary, integration and doc tests without building unrelated example executables. Shared Cargo +configuration and toolchain changes select the whole workspace; CI also keeps its workspace-wide checks. + ## Pull requests - **Small.** One change per pull request, under about 500 lines of code where you can; split bigger work into diff --git a/tests/test_pr_ready.py b/tests/test_pr_ready.py index c5b1b4815..c10e7b2ac 100644 --- a/tests/test_pr_ready.py +++ b/tests/test_pr_ready.py @@ -1,4 +1,5 @@ import argparse +import json import unittest from unittest import mock @@ -6,6 +7,8 @@ from tools import pr_ready PACKAGES = {"horizon-core": "crates/horizon-core", "horizon-client": "crates/horizon-client", "horizon-theme": "crates/horizon-theme", "xtask": "xtask"} +LIBRARIES = set(PACKAGES) - {"xtask"} +DEPENDENCIES = {"horizon-client": {"horizon-core"}} def args(host="local", quick=False): @@ -38,6 +41,100 @@ class PrReadyTests(unittest.TestCase): self.assertFalse(pr_ready.launcher_rust("launcher/README.md")) self.assertFalse(pr_ready.launcher_rust("crates/horizon-client/src/game.rs")) + def test_crate_checks_do_not_link_unrelated_examples(self): + with mock.patch.object(pr_ready, "affected_here", return_value=[]): + steps = pr_ready.plan(args(quick=True), "origin/main", {"crates/horizon-client/src/game.rs"}, PACKAGES, LIBRARIES, DEPENDENCIES) + by_name = {step.name: step for step in steps} + self.assertEqual(by_name["clippy"].command, + "cargo clippy -p horizon-client --all-targets --locked -- -D warnings") + self.assertEqual(by_name["tests: horizon-client"].command, + "cargo test --locked -p horizon-client --tests") + self.assertEqual(by_name["doc tests"].command, "cargo test --locked -p horizon-client --doc") + self.assertIsNone(by_name["doc tests"].status) + + def test_shared_rust_configuration_keeps_workspace_coverage(self): + for path in sorted(pr_ready.ROOT_RUST | {".cargo/config.toml", "crates/unknown/src/lib.rs"}): + with self.subTest(path=path), mock.patch.object(pr_ready, "affected_here", return_value=[]): + steps = pr_ready.plan(args(quick=True), "origin/main", {path}, PACKAGES, LIBRARIES, DEPENDENCIES) + by_name = {step.name: step for step in steps} + self.assertEqual(by_name["clippy"].command, + "cargo clippy --workspace --all-targets --locked -- -D warnings") + self.assertEqual(by_name["tests: workspace"].command, + "cargo test --locked --workspace --tests") + self.assertEqual(by_name["doc tests"].command, "cargo test --locked --workspace --doc") + + def test_non_rust_changes_skip_workspace_checks(self): + for path in ("README.md", "tools/pr_ready.py", "launcher/src/main.rs"): + with self.subTest(path=path), mock.patch.object(pr_ready, "affected_here", return_value=[]): + steps = pr_ready.plan(args(quick=True), "origin/main", {path}, PACKAGES, LIBRARIES, DEPENDENCIES) + by_name = {step.name: step for step in steps} + for name in ("clippy", "tests", "doc tests"): + self.assertEqual(by_name[name].status, "skip") + + def test_multiple_touched_crates_share_the_same_check_scope(self): + with mock.patch.object(pr_ready, "affected_here", return_value=[]): + steps = pr_ready.plan(args(), "origin/main", + {"crates/horizon-core/src/lib.rs", "crates/horizon-client/examples/probe.rs"}, PACKAGES, LIBRARIES, DEPENDENCIES) + for step in steps: + if step.name in ("clippy", "tests: horizon-client, horizon-core", "doc tests"): + self.assertIn("-p horizon-client -p horizon-core", step.command) + self.assertIsNone(step.status) + + def test_binary_only_crates_skip_doc_tests(self): + with mock.patch.object(pr_ready, "affected_here", return_value=[]): + steps = pr_ready.plan(args(), "origin/main", {"xtask/src/main.rs"}, PACKAGES, LIBRARIES, DEPENDENCIES) + by_name = {step.name: step for step in steps} + self.assertEqual(by_name["tests: xtask"].command, "cargo test --locked -p xtask --tests") + self.assertIsNone(by_name["tests: xtask"].status) + self.assertEqual(by_name["doc tests"].status, "skip") + with mock.patch.object(pr_ready, "affected_here", return_value=[]): + steps = pr_ready.plan(args(), "origin/main", + {"xtask/src/main.rs", "crates/horizon-core/src/lib.rs"}, PACKAGES, LIBRARIES, DEPENDENCIES) + by_name = {step.name: step for step in steps} + self.assertEqual(by_name["doc tests"].command, "cargo test --locked -p horizon-core --doc") + + def test_clippy_checks_transitive_consumers_but_tests_only_touched_crates(self): + packages = {name: f"crates/{name}" for name in ("core", "world", "client", "unrelated")} + dependencies = {"world": {"core"}, "client": {"world"}, "unrelated": set()} + with mock.patch.object(pr_ready, "affected_here", return_value=[]): + steps = pr_ready.plan(args(), "origin/main", {"crates/core/src/lib.rs"}, + packages, set(packages), dependencies) + by_name = {step.name: step for step in steps} + self.assertEqual(by_name["clippy"].command, + "cargo clippy -p client -p core -p world --all-targets --locked -- -D warnings") + self.assertEqual(by_name["tests: core"].command, "cargo test --locked -p core --tests") + self.assertEqual(by_name["doc tests"].command, "cargo test --locked -p core --doc") + + def test_dependent_scope_handles_cycles_and_multiple_changed_crates(self): + dependencies = {"a": {"b"}, "b": {"a"}, "c": {"b"}, "d": set()} + self.assertEqual(pr_ready.dependent_crates(["a"], dependencies), ["a", "b", "c"]) + self.assertEqual(pr_ready.dependent_crates(["a", "d"], dependencies), ["a", "b", "c", "d"]) + self.assertEqual(pr_ready.dependent_crates([], dependencies), []) + self.assertIsNone(pr_ready.dependent_crates(None, dependencies)) + + def test_metadata_includes_renamed_optional_platform_build_and_dev_consumers(self): + core = str(pr_ready.ROOT / "crates/core") + metadata = {"workspace_members": ["core", "optional", "build", "dev"], "packages": [ + {"id": "core", "name": "core", "manifest_path": core + "/Cargo.toml", + "targets": [{"kind": ["lib"]}], "dependencies": []}, + ]} + for name, kind in (("optional", None), ("build", "build"), ("dev", "dev")): + metadata["packages"].append({ + "id": name, "name": name, "manifest_path": str(pr_ready.ROOT / f"crates/{name}/Cargo.toml"), + "targets": [{"kind": ["bin"]}], "dependencies": [ + {"name": "core", "rename": "alias", "path": core, "kind": kind, + "optional": name == "optional", "target": "cfg(windows)"}, + {"name": "external", "path": None}, + ], + }) + # A path dependency outside this workspace must not become a selectable package. + metadata["packages"].append({"id": "outside", "name": "outside", "manifest_path": "/outside/Cargo.toml"}) + with mock.patch.object(pr_ready.subprocess, "run", return_value=mock.Mock(stdout=json.dumps(metadata))): + directories, libraries, dependencies = pr_ready.packages() + self.assertEqual(set(directories), {"core", "optional", "build", "dev"}) + self.assertEqual(libraries, {"core"}) + self.assertEqual(pr_ready.dependent_crates(["core"], dependencies), ["build", "core", "dev", "optional"]) + def test_scope_warns_on_size_and_mixed_areas(self): self.assertEqual(pr_ready.scope_warnings({"crates/horizon-client/src/game.rs", "tools/test_food_parity.py", "mods/horizon/src/test/java/dev/horizonlab/parity/X.java", @@ -85,7 +182,7 @@ class PrReadyTests(unittest.TestCase): def test_harnesses_are_listed_when_they_cannot_run(self): changed = {"crates/horizon-world/src/food.rs"} with mock.patch.object(pr_ready, "affected_here", return_value=["food", "light"]): - steps = pr_ready.plan(args(quick=True), "origin/main", changed, {"horizon-world": "crates/horizon-world"}) + steps = pr_ready.plan(args(quick=True), "origin/main", changed, {"horizon-world": "crates/horizon-world"}, {"horizon-world"}, {}) harness = steps[-1] self.assertEqual(harness.status, "list") self.assertIn("2 affected, to run in CI (--quick): food, light", harness.detail) @@ -94,7 +191,7 @@ class PrReadyTests(unittest.TestCase): self.assertEqual(by_name["launcher clippy"].status, "skip") self.assertIsNone(by_name["clippy"].status) with mock.patch.object(pr_ready, "affected_here", return_value=["food"]): - steps = pr_ready.plan(args(host="macmini"), "origin/main", changed, {"horizon-world": "crates/horizon-world"}) + steps = pr_ready.plan(args(host="macmini"), "origin/main", changed, {"horizon-world": "crates/horizon-world"}, {"horizon-world"}, {}) self.assertIn("--changed-files", steps[-1].command) self.assertIn("crates/horizon-world/src/food.rs", steps[-1].command) diff --git a/tools/pr_ready.py b/tools/pr_ready.py index 9ec216fea..e3e317ef8 100755 --- a/tools/pr_ready.py +++ b/tools/pr_ready.py @@ -12,6 +12,9 @@ same way when it, horizon-prepare or horizon-theme changed; website/src/tokens.c Cargo.lock changing only with a Cargo.toml; the Python tests and the public-tree check; and the vanilla harnesses the changes can affect, picked as CI's parity job picks them (tools/parity.py --changed). Harnesses run where the vanilla data is (a prepared .local/ here, or the Mac mini); elsewhere they are listed for CI. +Clippy checks all targets of touched crates and their transitive workspace dependents, including examples. +Tests select library, binary and integration targets explicitly, followed by doc tests, so unrelated example +executables are not built and linked again. It prints one Markdown block for the pull request's body (stdout) and exits non-zero when a check fails. Progress, and the end of a failed check's output, go to stderr (--verbose: all of it). @@ -89,7 +92,7 @@ def launcher_rust(path): def touched_crates(paths, packages): """Workspace packages to test for these paths ({name: directory}), or None for the whole workspace.""" - if any(path in ("Cargo.toml", "Cargo.lock") or path.startswith(".cargo/") for path in paths): + if any(path in ROOT_RUST or path.startswith(".cargo/") for path in paths): return None crates = set() for path in paths: @@ -195,14 +198,37 @@ def line_counts(base, changed): def packages(): - """The main workspace's packages and their directories, from Cargo's metadata.""" + """Workspace directories, libraries and path dependencies, including optional/build/dev dependencies.""" try: output = subprocess.run(["cargo", "metadata", "--no-deps", "--format-version", "1"], cwd=ROOT, capture_output=True, text=True, check=True).stdout except (OSError, subprocess.CalledProcessError) as error: sys.exit(f"pr_ready: cargo metadata failed: {getattr(error, 'stderr', error)}") - return {package["name"]: Path(package["manifest_path"]).parent.relative_to(ROOT).as_posix() - for package in json.loads(output)["packages"]} + metadata = json.loads(output) + members = set(metadata["workspace_members"]) + workspace = [package for package in metadata["packages"] if package["id"] in members] + directories = {package["name"]: Path(package["manifest_path"]).parent.relative_to(ROOT).as_posix() + for package in workspace} + libraries = {package["name"] for package in workspace + if any("lib" in target["kind"] for target in package["targets"])} + names_by_path = {Path(package["manifest_path"]).parent.resolve(): package["name"] for package in workspace} + dependencies = {package["name"]: {names_by_path[path] for dependency in package["dependencies"] + if dependency.get("path") and + (path := Path(dependency["path"]).resolve()) in names_by_path} + for package in workspace} + return directories, libraries, dependencies + + +def dependent_crates(crates, dependencies): + """Include every workspace consumer, even through renamed or feature-gated dependencies.""" + if crates is None: + return None + selected = set(crates) + while True: + consumers = {name for name, used in dependencies.items() if selected.intersection(used)} + if consumers <= selected: + return sorted(selected) + selected.update(consumers) def prepare_key(): @@ -248,12 +274,22 @@ def parity_summary(output): return text -def plan(args, base, changed, crates_of): +def plan(args, base, changed, crates_of, libraries, dependencies): """The checks for these changed files, in the order they run.""" rust = any(workspace_rust(path) for path in changed) launcher = any(launcher_rust(path) for path in changed) python = any(path.endswith(".py") or path.startswith(("tests/", "tools/")) for path in changed) crates = touched_crates(changed, crates_of) if rust or TOKENS in changed else [] + # An unrecognised Rust path must not silently skip validation. + if rust and crates == []: + crates = None + selection = "--workspace" if crates is None else " ".join(f"-p {shlex.quote(c)}" for c in crates) + clippy_crates = dependent_crates(crates, dependencies) + clippy_selection = "--workspace" if clippy_crates is None else " ".join( + f"-p {shlex.quote(c)}" for c in clippy_crates) + test_skip = None if crates is None or crates else "no crate touched" + doc_crates = sorted(libraries if crates is None else libraries.intersection(crates)) + doc_selection = "--workspace" if crates is None else " ".join(f"-p {shlex.quote(c)}" for c in doc_crates) no_rust = "no Rust change" steps = [ Step("Cargo.lock", where="git"), @@ -261,10 +297,13 @@ def plan(args, base, changed, crates_of): Step("Python tests", "python3 -m unittest discover -s tests", where="here", skip=None if python else "no Python change", summary=unittest_passed), Step("rustfmt", "cargo fmt --all -- --check", skip=None if rust else no_rust), - Step("clippy", "cargo clippy --workspace --all-targets --locked -- -D warnings", skip=None if rust else no_rust), + Step("clippy", f"cargo clippy {clippy_selection} --all-targets --locked -- -D warnings", + skip=None if rust else no_rust), Step("tests" + (": workspace" if crates is None else f": {', '.join(crates)}" if crates else ""), - "cargo test --locked " + ("--workspace" if crates is None else " ".join(f"-p {c}" for c in crates or [])), - skip=None if crates is None or crates else "no crate touched", summary=tests_passed), + f"cargo test --locked {selection} --tests", + skip=test_skip, summary=tests_passed), + Step("doc tests", f"cargo test --locked {doc_selection} --doc", + skip=test_skip or (None if doc_crates else "no library touched"), summary=tests_passed), Step("tokens.css up to date", "cargo test --locked -p horizon-theme --lib website_tokens_are_up_to_date", skip=None if any(p.startswith(("crates/horizon-theme/", "xtask/")) or p == TOKENS for p in changed) else "theme untouched"), @@ -448,7 +487,9 @@ def main(): parser.error(f"no merge base with {args.base} (git fetch?)") merge_base = git("merge-base", "HEAD", args.base).strip() counts = line_counts(merge_base, changed) - steps = plan(args, args.base, changed, packages() if any(workspace_rust(p) for p in changed) or TOKENS in changed else {}) + crates_of, libraries, dependencies = packages() if any(workspace_rust(p) for p in changed) or TOKENS in changed \ + else ({}, set(), {}) + steps = plan(args, args.base, changed, crates_of, libraries, dependencies) print(f"pr_ready: {len(changed)} files changed since {args.base}; checks on {args.host}", file=sys.stderr, flush=True) lock = steps[0]