-
Notifications
You must be signed in to change notification settings - Fork 0
fix(coverage): scope Rust evidence to changed packages #1187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4f8c786
e2d7c14
9cb5a40
8ec2c9f
6aa186b
92810bf
94ab7c0
91c16eb
e960321
c267298
80d13a1
3ea59f4
db1802f
2a5ab45
92c4afc
7d66872
fae577e
f71db4b
be6534b
0a88e24
471f34d
5772721
1db47e2
923fa07
047ad58
2b1abb0
3add872
d359776
72a9d72
3da6596
541cadd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1199,7 +1199,18 @@ jobs: | |
| if [ -n "${PR_BASE_SHA:-}" ] && [ -n "${PR_HEAD_SHA:-}" ] \ | ||
| && trusted_git rev-parse --verify --quiet "$PR_BASE_SHA^{commit}" >/dev/null \ | ||
| && trusted_git rev-parse --verify --quiet "$PR_HEAD_SHA^{commit}" >/dev/null; then | ||
| trusted_git diff --name-only --find-renames "$PR_BASE_SHA" "$PR_HEAD_SHA" | ||
| # --name-status (not --name-only) so a detected rename/copy | ||
| # surfaces BOTH its old and new path. --name-only collapses a | ||
| # rename to a single line naming only the destination, so a | ||
| # Rust source file renamed away to a non-Rust extension (or | ||
| # moved into a different Cargo package) would otherwise vanish | ||
| # from every consumer's changed-file inventory on the origin | ||
| # side -- has_changed_rust_files would miss it entirely, and | ||
| # rust_coverage_manifests would credit only the destination | ||
| # package. R/C status lines carry a third tab-separated field | ||
| # (the new path); every other status carries exactly the path. | ||
| trusted_git diff --name-status --find-renames "$PR_BASE_SHA" "$PR_HEAD_SHA" | | ||
| awk -F'\t' 'NF >= 3 { print $2; print $3; next } { print $2 }' | ||
|
Comment on lines
+1212
to
+1213
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
+1212
to
+1213
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| else | ||
| trusted_git ls-files | ||
| fi | ||
|
|
@@ -1218,6 +1229,11 @@ jobs: | |
| return "$rc" | ||
| } | ||
|
|
||
| has_changed_rust_files() { | ||
| changed_files_for_coverage | | ||
| awk '$0 ~ /(^|\/)Cargo\.(toml|lock)$/ || $0 ~ /\.rs$/ { found=1 } END { exit found ? 0 : 1 }' | ||
|
Comment on lines
+1232
to
+1234
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
devin-ai-integration[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| tracked_python_projects_with_tests() { | ||
| trusted_git ls-files 'pyproject.toml' '*/pyproject.toml' 'requirements.txt' '*/requirements.txt' \ | ||
| | while IFS= read -r pyproject_file; do | ||
|
|
@@ -2000,30 +2016,49 @@ jobs: | |
| } | ||
|
|
||
| rust_coverage_manifests() { | ||
| if [ -f Cargo.toml ]; then | ||
| # A repository-wide coverage run is required when the workspace | ||
| # manifest or lockfile changed. Otherwise measure only the | ||
| # changed Rust package(s); building every workspace member at once | ||
| # can exhaust the review runner disk before tests begin. | ||
| if changed_files_for_coverage \ | ||
| | awk '$0 == "Cargo.toml" || $0 == "Cargo.lock" { found=1 } END { exit found ? 0 : 1 }'; then | ||
| printf '%s\n' Cargo.toml | ||
| return 0 | ||
|
seonghobae marked this conversation as resolved.
devin-ai-integration[bot] marked this conversation as resolved.
|
||
| fi | ||
| changed_files_for_coverage \ | ||
| local manifests | ||
| manifests="$(changed_files_for_coverage \ | ||
| | while IFS= read -r changed_path; do | ||
| case "$changed_path" in | ||
| Cargo.toml|Cargo.lock|*.rs) ;; | ||
| Cargo.toml|Cargo.lock|*/Cargo.toml|*/Cargo.lock|*.rs) ;; | ||
| *) continue ;; | ||
| esac | ||
| candidate_dir="$(dirname "$changed_path")" | ||
| while [ "$candidate_dir" != "." ] && [ "$candidate_dir" != "/" ]; do | ||
| while :; do | ||
| if [ -f "${candidate_dir}/Cargo.toml" ]; then | ||
| printf '%s\n' "${candidate_dir}/Cargo.toml" | ||
| if [ "$candidate_dir" = "." ]; then | ||
| printf '%s\n' Cargo.toml | ||
| else | ||
| printf '%s\n' "${candidate_dir}/Cargo.toml" | ||
| fi | ||
| break | ||
|
seonghobae marked this conversation as resolved.
|
||
| fi | ||
| [ "$candidate_dir" = "." ] && break | ||
|
seonghobae marked this conversation as resolved.
|
||
| next_dir="$(dirname "$candidate_dir")" | ||
| if [ "$next_dir" = "$candidate_dir" ]; then | ||
| break | ||
|
seonghobae marked this conversation as resolved.
seonghobae marked this conversation as resolved.
|
||
| fi | ||
| candidate_dir="$next_dir" | ||
| done | ||
| done \ | ||
| | sort -u | ||
| | sort -u)" | ||
| # A root workspace run already covers every member; do not repeat | ||
| # nested package runs when root and member sources changed together. | ||
| if printf '%s\n' "$manifests" \ | ||
| | awk '$0 == "Cargo.toml" { found=1 } END { exit found ? 0 : 1 }'; then | ||
| printf '%s\n' Cargo.toml | ||
| elif [ -n "$manifests" ]; then | ||
| printf '%s\n' "$manifests" | ||
| fi | ||
|
seonghobae marked this conversation as resolved.
|
||
| } | ||
|
|
||
| rust_coverage_fail_under_lines() { | ||
|
|
@@ -2241,7 +2276,7 @@ jobs: | |
| run_r_test_coverage | ||
| fi | ||
|
|
||
| if has_changed_tracked_files 'Cargo.toml' 'Cargo.lock' '*.rs'; then | ||
| if has_changed_rust_files; then | ||
| measured_any=1 | ||
| run_rust_test_coverage | ||
| fi | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| from __future__ import annotations | ||
|
|
||
| import argparse | ||
| import fnmatch | ||
| import tomllib | ||
| from pathlib import Path | ||
| from typing import Any | ||
|
|
@@ -36,18 +37,93 @@ def resolve_minimum_lines(document: dict[str, Any]) -> float | None: | |
| break | ||
| if selected_path is None: | ||
| return None | ||
| return _validate_minimum_lines(selected_path, value) | ||
|
|
||
|
|
||
| def _validate_minimum_lines(path: str, value: Any) -> float: | ||
| """Validate one repository-owned coverage baseline and normalize it.""" | ||
| if isinstance(value, bool) or not isinstance(value, (int, float)): | ||
| raise ValueError(f"{selected_path} must be a number from 0 to 100") | ||
| raise ValueError(f"{path} must be a number from 0 to 100") | ||
| threshold = float(value) | ||
| if not 0 <= threshold <= 100: | ||
| raise ValueError(f"{selected_path} must be between 0 and 100") | ||
| raise ValueError(f"{path} must be between 0 and 100") | ||
| return threshold | ||
|
|
||
|
|
||
| def _relative_posix_path(path: Path, base: Path) -> str | None: | ||
| """Return ``path`` relative to ``base`` as a POSIX string, or None if unrelated.""" | ||
| try: | ||
| return path.relative_to(base).as_posix() | ||
| except ValueError: | ||
| return None | ||
|
|
||
|
|
||
| def _workspace_excludes_package( | ||
| workspace_dir: Path, package_dir: Path, workspace_document: dict[str, Any] | ||
| ) -> bool: | ||
| """Return whether a workspace's ``exclude`` patterns cover the package directory. | ||
|
|
||
| Cargo's own automatic workspace-root discovery walks upward from a | ||
| package's manifest and skips an ancestor workspace that excludes it, | ||
| continuing the search further out rather than treating the excluded | ||
| workspace as authoritative. Mirroring that here keeps an excluded (or | ||
| otherwise independent) package from inheriting a coverage baseline that | ||
| was never configured for it. | ||
| """ | ||
| workspace_table = workspace_document.get("workspace") | ||
| if not isinstance(workspace_table, dict): | ||
| return False | ||
| excludes = workspace_table.get("exclude") | ||
| if not isinstance(excludes, list): | ||
| return False | ||
| relative = _relative_posix_path(package_dir, workspace_dir) | ||
| if relative is None: | ||
| return False | ||
| for pattern in excludes: | ||
| if not isinstance(pattern, str): | ||
| continue | ||
| normalized_pattern = pattern.rstrip("/") | ||
| if relative == normalized_pattern or fnmatch.fnmatch(relative, normalized_pattern): | ||
| return True | ||
| if relative.startswith(f"{normalized_pattern}/"): | ||
| return True | ||
|
Comment on lines
+85
to
+89
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| return False | ||
|
|
||
|
|
||
| def read_minimum_lines(manifest: Path) -> float | None: | ||
| """Read and resolve one Cargo manifest's line-coverage baseline.""" | ||
| """Read a package baseline, falling back to its nearest workspace baseline.""" | ||
| manifest = manifest.resolve() | ||
| document = tomllib.loads(manifest.read_text(encoding="utf-8")) | ||
| return resolve_minimum_lines(document) | ||
| threshold = resolve_minimum_lines(document) | ||
| if threshold is not None: | ||
| return threshold | ||
|
Comment on lines
+95
to
+99
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| package_dir = manifest.parent | ||
| for parent in manifest.parents: | ||
| workspace_manifest = parent / "Cargo.toml" | ||
| if not workspace_manifest.is_file(): | ||
| continue | ||
| workspace_document = tomllib.loads(workspace_manifest.read_text(encoding="utf-8")) | ||
| if "workspace" not in workspace_document: | ||
| continue | ||
| if _workspace_excludes_package(parent, package_dir, workspace_document): | ||
| # This ancestor's workspace explicitly excludes the package, so | ||
| # it is not this package's workspace root. Keep walking further | ||
| # out for an unrelated ancestor workspace that might still | ||
| # legitimately claim it, matching Cargo's own root-discovery | ||
| # rule for excluded members. | ||
| continue | ||
| # This is the package's actual (nearest, non-excluding) Cargo | ||
| # workspace root -- whether the sole enclosing workspace or a | ||
| # nested, independent one. Stop here even when it configures no | ||
| # baseline: crossing this boundary to search an even-more-outer, | ||
| # unrelated workspace would attribute a threshold that was never | ||
| # configured for this package's own workspace. | ||
| workspace_value = _nested_value(workspace_document, METADATA_PATHS[1]) | ||
| if workspace_value is not None: | ||
| return _validate_minimum_lines(METADATA_PATHS[1], workspace_value) | ||
|
Comment on lines
+102
to
+124
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| return None | ||
| return None | ||
|
seonghobae marked this conversation as resolved.
seonghobae marked this conversation as resolved.
|
||
|
|
||
|
|
||
| def main() -> int: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.