chexit/horizon-airdef
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Scope PR validation to affected crates and avoid unrelated example builds

Browse Source
This commit is contained in:
rdbtCVS
2026-10-21 00:56:37 +01:00
parent defaa7cd2a
commit a7941a8eca
3 changed files with 154 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CONTRIBUTING.md
+5
View File
@@ -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
tests/test_pr_ready.py
+99 -2
View File
@@ -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)
tools/pr_ready.py
+50 -9
View File
@@ -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]