mirror of
https://github.com/BerriAI/litellm.git
synced 2026-08-28 05:25:59 +00:00
fix(ci): gate backend unit tests on the pull request's own file list (#37550)
detect-backend-changes diffed the event payload's base.sha against the checked-out ref. Those are two different points in time: actions/checkout resolves refs/pull/N/merge, and GitHub recomputes that ref whenever the base branch advances, so the diff picked up whatever landed on staging between the event firing and the job starting. On a recent UI-only pull request three backend commits from staging were attributed to the branch, and every backend shard ran in full Ask the API which files the pull request touches instead. That is the same set the Files changed tab shows, and it is immune to either endpoint moving. The shell body moves into .github/scripts/detect_backend_changes.sh so it can be exercised directly, and the fail-open paths now also cover an API failure, a file list past the API's 3000-entry listing ceiling, and a classifier that prints something unexpected
This commit is contained in:
parent
b7181a8914
commit
eecb226762
5 changed files with 253 additions and 31 deletions
|
|
@ -3,9 +3,18 @@ description: >-
|
|||
Classify the pull request's changed files with .circleci/scripts/classify_changes.sh
|
||||
and expose decision=run|skip. decision=skip means only ui/**, **.md or **.mdx files
|
||||
changed, so callers can short-circuit expensive steps while the job still completes
|
||||
successfully and satisfies its required status check. The decision defaults to run for
|
||||
any non pull_request event or whenever the changed set cannot be resolved, so tests are
|
||||
never skipped when the classification is uncertain.
|
||||
successfully and satisfies its required status check. The file list comes from the
|
||||
pull request itself rather than from a git diff, because the checked-out merge ref is
|
||||
recomputed as the base branch advances and would otherwise attribute the base
|
||||
branch's own commits to the pull request. The decision defaults to run for any non
|
||||
pull_request event or whenever the changed set cannot be resolved, so tests are never
|
||||
skipped when the classification is uncertain.
|
||||
|
||||
inputs:
|
||||
github-token:
|
||||
description: "Token used to list the pull request's files; needs pull-requests: read"
|
||||
required: false
|
||||
default: ${{ github.token }}
|
||||
|
||||
outputs:
|
||||
decision:
|
||||
|
|
@ -18,31 +27,8 @@ runs:
|
|||
- id: classify
|
||||
shell: bash
|
||||
env:
|
||||
BASE_SHA: ${{ github.event.pull_request.base.sha }}
|
||||
run: |
|
||||
set -uo pipefail
|
||||
if [ -z "${BASE_SHA:-}" ]; then
|
||||
echo "detect-backend-changes: not a pull_request event; running job"
|
||||
echo "decision=run" >> "${GITHUB_OUTPUT}"
|
||||
exit 0
|
||||
fi
|
||||
if ! git fetch --no-tags --depth=1 origin "${BASE_SHA}" >/dev/null 2>&1; then
|
||||
echo "detect-backend-changes: could not fetch base ${BASE_SHA}; running job"
|
||||
echo "decision=run" >> "${GITHUB_OUTPUT}"
|
||||
exit 0
|
||||
fi
|
||||
changed="$(git diff --name-only "${BASE_SHA}" HEAD 2>/dev/null)" || {
|
||||
echo "detect-backend-changes: git diff failed; running job"
|
||||
echo "decision=run" >> "${GITHUB_OUTPUT}"
|
||||
exit 0
|
||||
}
|
||||
if [ -z "${changed}" ]; then
|
||||
echo "detect-backend-changes: no changed files vs ${BASE_SHA}; skipping job"
|
||||
echo "decision=skip" >> "${GITHUB_OUTPUT}"
|
||||
exit 0
|
||||
fi
|
||||
echo "detect-backend-changes: changed files vs ${BASE_SHA}:"
|
||||
printf '%s\n' "${changed}" | sed 's/^/ /'
|
||||
decision="$(printf '%s\n' "${changed}" | bash .circleci/scripts/classify_changes.sh backend)" || decision="run"
|
||||
echo "detect-backend-changes: decision=${decision}"
|
||||
echo "decision=${decision}" >> "${GITHUB_OUTPUT}"
|
||||
GH_TOKEN: ${{ inputs.github-token }}
|
||||
REPO: ${{ github.repository }}
|
||||
PR_NUMBER: ${{ github.event.pull_request.number }}
|
||||
CHANGED_FILE_COUNT: ${{ github.event.pull_request.changed_files }}
|
||||
run: bash "${GITHUB_ACTION_PATH}/../../scripts/detect_backend_changes.sh"
|
||||
|
|
|
|||
41
.github/scripts/detect_backend_changes.sh
vendored
Executable file
41
.github/scripts/detect_backend_changes.sh
vendored
Executable file
|
|
@ -0,0 +1,41 @@
|
|||
#!/usr/bin/env bash
|
||||
set -uo pipefail
|
||||
|
||||
readonly API_FILE_CEILING=3000
|
||||
|
||||
decide() {
|
||||
echo "detect-backend-changes: decision=$1"
|
||||
[ -z "${GITHUB_OUTPUT:-}" ] || echo "decision=$1" >>"${GITHUB_OUTPUT}"
|
||||
exit 0
|
||||
}
|
||||
|
||||
run_full() {
|
||||
echo "detect-backend-changes: $1; running job"
|
||||
decide run
|
||||
}
|
||||
|
||||
here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
||||
classify="${here}/../../.circleci/scripts/classify_changes.sh"
|
||||
|
||||
[ -n "${PR_NUMBER:-}" ] || run_full "not a pull_request event"
|
||||
[ -n "${REPO:-}" ] || run_full "no repository in the environment"
|
||||
|
||||
case "${CHANGED_FILE_COUNT:-}" in
|
||||
'' | *[!0-9]*) run_full "the event payload carries no changed_files count" ;;
|
||||
esac
|
||||
[ "${CHANGED_FILE_COUNT}" -le "${API_FILE_CEILING}" ] ||
|
||||
run_full "PR #${PR_NUMBER} changes ${CHANGED_FILE_COUNT} files, past the ${API_FILE_CEILING}-file listing ceiling"
|
||||
|
||||
changed="$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename')" ||
|
||||
run_full "could not list the files on PR #${PR_NUMBER}"
|
||||
[ -n "${changed}" ] || run_full "the API listed no files on PR #${PR_NUMBER}"
|
||||
|
||||
echo "detect-backend-changes: files changed by PR #${PR_NUMBER}:"
|
||||
printf '%s\n' "${changed}" | sed 's/^/ /'
|
||||
|
||||
decision="$(printf '%s\n' "${changed}" | bash "${classify}" backend)" ||
|
||||
run_full "classify_changes.sh failed"
|
||||
case "${decision}" in
|
||||
run | skip) decide "${decision}" ;;
|
||||
*) run_full "classify_changes.sh printed an unexpected decision: ${decision}" ;;
|
||||
esac
|
||||
3
.github/workflows/_test-unit-base.yml
vendored
3
.github/workflows/_test-unit-base.yml
vendored
|
|
@ -60,6 +60,9 @@ jobs:
|
|||
name: Run tests
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: ${{ inputs.job-timeout-minutes }}
|
||||
permissions:
|
||||
contents: read
|
||||
pull-requests: read
|
||||
outputs:
|
||||
decision: ${{ steps.changes.outputs.decision }}
|
||||
|
||||
|
|
|
|||
|
|
@ -23,6 +23,9 @@ jobs:
|
|||
documentation:
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 10
|
||||
permissions:
|
||||
contents: read
|
||||
pull-requests: read
|
||||
|
||||
steps:
|
||||
- uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0
|
||||
|
|
|
|||
189
tests/test_litellm/test_detect_backend_changes.py
Normal file
189
tests/test_litellm/test_detect_backend_changes.py
Normal file
|
|
@ -0,0 +1,189 @@
|
|||
"""Regression tests for the GitHub Actions change-based job gating.
|
||||
|
||||
`.github/scripts/detect_backend_changes.sh` decides whether a pull request's
|
||||
backend unit-test jobs do real work. It asks the API which files the pull
|
||||
request touches and hands them to `classify_changes.sh`. The contract locked in
|
||||
here:
|
||||
|
||||
* a UI-only pull request skips backend jobs even when the checked-out merge
|
||||
ref carries backend commits from the base branch
|
||||
* anything the classification cannot resolve (no pull request, an API
|
||||
failure, a truncated file list, a broken classifier) runs the job
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import shutil
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
SCRIPT = REPO_ROOT / ".github" / "scripts" / "detect_backend_changes.sh"
|
||||
CLASSIFIER = REPO_ROOT / ".circleci" / "scripts" / "classify_changes.sh"
|
||||
|
||||
UI_FILE = "ui/litellm-dashboard/src/components/Teams.tsx"
|
||||
BACKEND_FILE = "litellm/proxy/proxy_server.py"
|
||||
|
||||
|
||||
def _git(cwd: Path, *args: str) -> None:
|
||||
subprocess.run(["git", *args], cwd=cwd, check=True, capture_output=True, text=True)
|
||||
|
||||
|
||||
def _merge_ref_checkout(tmp_path: Path) -> Path:
|
||||
"""A checkout shaped like `refs/pull/N/merge`: a UI-only branch merged into a
|
||||
base tip that has moved ahead by a backend commit since the branch was cut."""
|
||||
work = tmp_path / "work"
|
||||
work.mkdir()
|
||||
_git(work, "init", "-q", "-b", "main")
|
||||
_git(work, "config", "user.email", "t@t")
|
||||
_git(work, "config", "user.name", "t")
|
||||
(work / "seed.txt").write_text("seed\n")
|
||||
_git(work, "add", "-A")
|
||||
_git(work, "commit", "-qm", "base")
|
||||
|
||||
_git(work, "checkout", "-q", "-b", "feature")
|
||||
ui = work / UI_FILE
|
||||
ui.parent.mkdir(parents=True, exist_ok=True)
|
||||
ui.write_text("export const Teams = () => null\n")
|
||||
_git(work, "add", "-A")
|
||||
_git(work, "commit", "-qm", "ui change")
|
||||
|
||||
_git(work, "checkout", "-q", "main")
|
||||
backend = work / BACKEND_FILE
|
||||
backend.parent.mkdir(parents=True, exist_ok=True)
|
||||
backend.write_text("x = 1\n")
|
||||
_git(work, "add", "-A")
|
||||
_git(work, "commit", "-qm", "someone else's backend change")
|
||||
|
||||
_git(work, "merge", "-q", "--no-ff", "-m", "Merge feature into main", "feature")
|
||||
return work
|
||||
|
||||
|
||||
def _scripts_tree(tmp_path: Path, classifier_body: str | None = None) -> Path:
|
||||
"""Copy the scripts into a throwaway tree, preserving their relative layout."""
|
||||
root = tmp_path / "tree"
|
||||
(root / ".github" / "scripts").mkdir(parents=True)
|
||||
(root / ".circleci" / "scripts").mkdir(parents=True)
|
||||
shutil.copy(SCRIPT, root / ".github" / "scripts" / SCRIPT.name)
|
||||
target = root / ".circleci" / "scripts" / CLASSIFIER.name
|
||||
if classifier_body is None:
|
||||
shutil.copy(CLASSIFIER, target)
|
||||
else:
|
||||
target.write_text(classifier_body)
|
||||
target.chmod(0o755)
|
||||
return root
|
||||
|
||||
|
||||
def _run(
|
||||
tmp_path: Path,
|
||||
*,
|
||||
files: list[str],
|
||||
cwd: Path | None = None,
|
||||
pr_number: str = "37540",
|
||||
changed_file_count: str | None = None,
|
||||
gh_exit_code: int = 0,
|
||||
classifier_body: str | None = None,
|
||||
) -> tuple[str, str]:
|
||||
"""Run the script against a stubbed `gh`; returns (decision, stdout)."""
|
||||
bin_dir = tmp_path / "bin"
|
||||
bin_dir.mkdir(exist_ok=True)
|
||||
listing = "".join(f"echo {f}\n" for f in files)
|
||||
stub = bin_dir / "gh"
|
||||
stub.write_text(f"#!/usr/bin/env bash\n{listing}exit {gh_exit_code}\n")
|
||||
stub.chmod(0o755)
|
||||
|
||||
output_file = tmp_path / "github_output"
|
||||
output_file.write_text("")
|
||||
|
||||
env = {k: v for k, v in os.environ.items() if k not in {"GH_TOKEN", "GITHUB_TOKEN"}}
|
||||
env["PATH"] = f"{bin_dir}{os.pathsep}{env['PATH']}"
|
||||
env["GITHUB_OUTPUT"] = str(output_file)
|
||||
env["REPO"] = "BerriAI/litellm"
|
||||
env["PR_NUMBER"] = pr_number
|
||||
env["CHANGED_FILE_COUNT"] = changed_file_count if changed_file_count is not None else str(len(files))
|
||||
|
||||
tree = _scripts_tree(tmp_path, classifier_body)
|
||||
result = subprocess.run(
|
||||
["bash", str(tree / ".github" / "scripts" / SCRIPT.name)],
|
||||
cwd=cwd or tmp_path,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
env=env,
|
||||
check=True,
|
||||
)
|
||||
return output_file.read_text().strip(), result.stdout
|
||||
|
||||
|
||||
def test_ui_only_pr_skips_even_when_the_merge_ref_carries_backend_commits(tmp_path: Path) -> None:
|
||||
"""The bug this replaces: diffing the checked-out merge ref against the event's
|
||||
base sha attributed the base branch's own backend commits to the pull request,
|
||||
so every UI-only PR ran the full backend suite."""
|
||||
work = _merge_ref_checkout(tmp_path)
|
||||
tracked = subprocess.run(
|
||||
["git", "diff", "--name-only", "HEAD~2", "HEAD"],
|
||||
cwd=work,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
check=True,
|
||||
).stdout.split()
|
||||
assert BACKEND_FILE in tracked, "the checkout must contain the base branch's backend commit"
|
||||
|
||||
decision, _ = _run(tmp_path, files=[UI_FILE], cwd=work)
|
||||
assert decision == "decision=skip"
|
||||
|
||||
|
||||
def test_backend_file_in_the_pr_runs(tmp_path: Path) -> None:
|
||||
decision, _ = _run(tmp_path, files=[UI_FILE, BACKEND_FILE])
|
||||
assert decision == "decision=run"
|
||||
|
||||
|
||||
def test_docs_only_pr_skips(tmp_path: Path) -> None:
|
||||
decision, _ = _run(tmp_path, files=["README.md", "docs/my-website/index.mdx"])
|
||||
assert decision == "decision=skip"
|
||||
|
||||
|
||||
def test_non_pull_request_event_runs(tmp_path: Path) -> None:
|
||||
decision, stdout = _run(tmp_path, files=[UI_FILE], pr_number="")
|
||||
assert decision == "decision=run"
|
||||
assert "not a pull_request event" in stdout
|
||||
|
||||
|
||||
def test_api_failure_runs(tmp_path: Path) -> None:
|
||||
decision, stdout = _run(tmp_path, files=[], gh_exit_code=1)
|
||||
assert decision == "decision=run"
|
||||
assert "could not list the files" in stdout
|
||||
|
||||
|
||||
def test_empty_file_list_runs(tmp_path: Path) -> None:
|
||||
decision, stdout = _run(tmp_path, files=[], changed_file_count="0")
|
||||
assert decision == "decision=run"
|
||||
assert "listed no files" in stdout
|
||||
|
||||
|
||||
def test_pr_past_the_listing_ceiling_runs(tmp_path: Path) -> None:
|
||||
"""The API caps its file listing, so a larger PR would be classified from a
|
||||
truncated set and could skip backend jobs it needs."""
|
||||
decision, stdout = _run(tmp_path, files=[UI_FILE], changed_file_count="3001")
|
||||
assert decision == "decision=run"
|
||||
assert "past the 3000-file listing ceiling" in stdout
|
||||
|
||||
|
||||
def test_broken_classifier_runs(tmp_path: Path) -> None:
|
||||
decision, stdout = _run(
|
||||
tmp_path,
|
||||
files=[UI_FILE],
|
||||
classifier_body="#!/usr/bin/env bash\nexit 1\n",
|
||||
)
|
||||
assert decision == "decision=run"
|
||||
assert "classify_changes.sh failed" in stdout
|
||||
|
||||
|
||||
def test_unexpected_classifier_output_runs(tmp_path: Path) -> None:
|
||||
decision, stdout = _run(
|
||||
tmp_path,
|
||||
files=[UI_FILE],
|
||||
classifier_body="#!/usr/bin/env bash\ncat >/dev/null\necho maybe\n",
|
||||
)
|
||||
assert decision == "decision=run"
|
||||
assert "unexpected decision: maybe" in stdout
|
||||
Loading…
Add table
Reference in a new issue