mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-17 04:34:25 +00:00
docs(agents): keep directory rules in nested AGENTS.md and pin integration tests to CircleCI (#46796)
* docs(agents): keep directory rules in nested AGENTS.md and pin integration tests to CircleCI * docs(agents): scope the CircleCI rule to the files it covers and ask agents to read nested AGENTS.md * ci(coverage): reject GITHUB_FILES integration contracts run from a test-unit.yml shard * ci(coverage): report integration ownership findings under their own header * ci(coverage): flag GITHUB_FILES contracts run from any non-dedicated workflow * docs(integration): say a pre-merge run is no reason to route an integration file to GitHub * ci(coverage): tie each dedicated integration workflow to the directory it owns
This commit is contained in:
parent
0dac61a555
commit
57e69ce713
5 changed files with 141 additions and 21 deletions
58
.github/scripts/assert_ci_coverage.py
vendored
58
.github/scripts/assert_ci_coverage.py
vendored
|
|
@ -32,6 +32,7 @@ IGNORE_ARG_RE: Final = re.compile(r"--ignore(?:-glob)?[= ](\S+)")
|
|||
DOCKERFILE_TOKEN_RE = re.compile(r"[A-Za-z0-9_./-]*Dockerfile[A-Za-z0-9_.-]*")
|
||||
COMMENT_RE = re.compile(r"^\s*#.*$", re.MULTILINE)
|
||||
GLOB_CHARS = frozenset("*?")
|
||||
DEDICATED_INTEGRATION_WORKFLOWS: Final = MappingProxyType({"image-scan.yml": "tests/integration/database/image/"})
|
||||
|
||||
# Trees whose jobs are sharded with no catch-all bucket, so every child that holds
|
||||
# tests has to be named by some shard or it runs nowhere. A child listed here is
|
||||
|
|
@ -560,6 +561,17 @@ def _integration_github_files(runner: pathlib.Path) -> frozenset[str]:
|
|||
return frozenset(ast.literal_eval(values))
|
||||
|
||||
|
||||
def _owns(workflow: str, relative_path: str) -> bool:
|
||||
owned_root: Final = DEDICATED_INTEGRATION_WORKFLOWS.get(workflow)
|
||||
return owned_root is not None and relative_path.startswith(owned_root)
|
||||
|
||||
|
||||
def _tokens_outside_owner(relative_path: str, workflow_tokens: Mapping[str, frozenset[str]]) -> frozenset[str]:
|
||||
return frozenset[str]().union(
|
||||
*(tokens for workflow, tokens in workflow_tokens.items() if not _owns(workflow, relative_path))
|
||||
)
|
||||
|
||||
|
||||
def _integration_ownership(repo_root: pathlib.Path = REPO_ROOT) -> tuple[frozenset[str], tuple[Finding, ...]]:
|
||||
runner: Final = repo_root / "tests/integration/run.py"
|
||||
if not runner.exists():
|
||||
|
|
@ -611,15 +623,20 @@ def _integration_ownership(repo_root: pathlib.Path = REPO_ROOT) -> tuple[frozens
|
|||
)
|
||||
!= 1
|
||||
)
|
||||
gha_tokens: Final = _invoked_test_tokens(
|
||||
scalar
|
||||
for path in (repo_root / ".github/workflows").glob("*.y*ml")
|
||||
for scalar in _scalars(yaml.safe_load(path.read_text()), path.name)
|
||||
workflow_tokens: Final = MappingProxyType(
|
||||
{
|
||||
path.name: _invoked_test_tokens(_scalars(yaml.safe_load(path.read_text()), path.name))
|
||||
for path in (repo_root / ".github/workflows").glob("*.y*ml")
|
||||
}
|
||||
)
|
||||
gha_tokens: Final = frozenset[str]().union(*workflow_tokens.values())
|
||||
circle_owned: Final = (
|
||||
frozenset(str(path.relative_to(repo_root)) for path in integration_root.rglob("test_*.py")) - github_files
|
||||
)
|
||||
findings: Final = (
|
||||
tuple(
|
||||
Finding(path, "integration contract is also selected by GitHub Actions")
|
||||
for path in paths
|
||||
Finding(path, "integration file outside GITHUB_FILES is selected by GitHub Actions")
|
||||
for path in sorted(circle_owned)
|
||||
if any(_token_covers(token, path) for token in gha_tokens)
|
||||
)
|
||||
+ tuple(
|
||||
|
|
@ -632,6 +649,11 @@ def _integration_ownership(repo_root: pathlib.Path = REPO_ROOT) -> tuple[frozens
|
|||
for path in sorted(github_files)
|
||||
if not (repo_root / path).is_file()
|
||||
)
|
||||
+ tuple(
|
||||
Finding(path, "GitHub-owned integration contract runs outside a dedicated integration workflow")
|
||||
for path in sorted(github_files)
|
||||
if any(_token_covers(token, path) for token in _tokens_outside_owner(path, workflow_tokens))
|
||||
)
|
||||
)
|
||||
browser_commands: Final = tuple(
|
||||
scalar.value
|
||||
|
|
@ -688,13 +710,9 @@ def main() -> int:
|
|||
scalars = _all_scalars()
|
||||
|
||||
integration_paths, ownership_findings = _integration_ownership()
|
||||
test_findings = (
|
||||
_uncovered_tests(
|
||||
allowlist,
|
||||
_invoked_selections(scalars)
|
||||
+ (Selection(included=integration_paths, ignored=frozenset()),),
|
||||
)
|
||||
+ ownership_findings
|
||||
test_findings = _uncovered_tests(
|
||||
allowlist,
|
||||
_invoked_selections(scalars) + (Selection(included=integration_paths, ignored=frozenset()),),
|
||||
)
|
||||
dockerfile_findings = _uncovered_dockerfiles(allowlist, _built_dockerfile_tokens(scalars))
|
||||
stale_findings = _stale_allowlist_paths(allowlist, test_files=_test_files(), dockerfiles=_dockerfiles())
|
||||
|
|
@ -711,13 +729,25 @@ def main() -> int:
|
|||
test_findings,
|
||||
"Add each to a job's test path, or list it in .github/ci-coverage-allowlist.yml with a reason.",
|
||||
)
|
||||
if ownership_findings:
|
||||
browser_remedy = (
|
||||
" CircleCI's browser group owns the integrationCritical Playwright contracts, so no GitHub workflow may "
|
||||
"select them and the default Playwright config must exclude them."
|
||||
if any("browser" in finding.detail or "Playwright" in finding.detail for finding in ownership_findings)
|
||||
else ""
|
||||
)
|
||||
_report(
|
||||
"integration contracts with the wrong CI owner",
|
||||
ownership_findings,
|
||||
"Follow tests/integration/AGENTS.md, which says which CI job owns each integration file." + browser_remedy,
|
||||
)
|
||||
if dockerfile_findings:
|
||||
_report(
|
||||
"Dockerfiles that no CI job builds",
|
||||
dockerfile_findings,
|
||||
"Build each in a workflow, or list it in .github/ci-coverage-allowlist.yml with a reason.",
|
||||
)
|
||||
if stale_findings or test_findings or dockerfile_findings:
|
||||
if stale_findings or test_findings or ownership_findings or dockerfile_findings:
|
||||
return 1
|
||||
|
||||
_write(
|
||||
|
|
|
|||
|
|
@ -29,7 +29,7 @@ A test must only fail when litellm code changes. Never pin facts we don't own (a
|
|||
|
||||
`tests/unit/` mirrors `litellm/` in a parallel path (see `tests/unit/AGENTS.md`). Name tests `test_<filename>.py`, but always match the existing test file in the directory you touch — many provider dirs use longer descriptive names (e.g. `test_anthropic_chat_transformation.py`) to avoid ambiguity across sibling folders. For bug fixes, extend the existing mapped test file rather than creating a new one. Only create a new test file for a new feature (provider, endpoint, or transformation module) that has no mapped test yet, following that directory's naming convention (or `test_<filename>.py` if you're the first test there). One focused regression test beats many shallow ones
|
||||
|
||||
End-to-end tests belong in `tests/e2e/` and must follow the harness conventions documented in that directory's `AGENTS.md`
|
||||
End-to-end tests belong in `tests/e2e/`. A rule about one directory's files, like its harness or which CI runs it, goes in that directory's `AGENTS.md`, never only in a README or here. Read a directory's `AGENTS.md` before editing files in it
|
||||
|
||||
When creating PRs, target the repository's current default branch for both internal and external / OSS contributions. Check it with `python3 scripts/default_branch.py --branch` instead of assuming a branch name or relying on cached `origin/HEAD`
|
||||
|
||||
|
|
@ -72,7 +72,7 @@ When referencing or running models (coding, QA'ing, writing docs, writing tests,
|
|||
|
||||
Always pull before starting any work. The checkout or worktree may be sitting on a stale branch
|
||||
|
||||
If you're an internal contributor, when creating a new PR, the typical flow is to branch off the repository's current default branch and create a branch prefixed with litellm_. Do not create a branch prefixed with claude/ and generally do not have / in your branch names
|
||||
Internal contributors branch a new PR off the repository's current default branch, with a branch name prefixed with litellm_
|
||||
|
||||
Do not add `Co-Authored-By: Claude` or any Claude attribution to commit messages. Never use a `claude/` prefix or put a `/` in a branch name. Do not add "Generated with Claude Code" (or any similar attribution) to PR descriptions or comments. Do not create a new PR/branch off the existing PR to fix/add something that is related and could've just been committed directly to the existing PR's branch
|
||||
|
||||
|
|
|
|||
|
|
@ -25,3 +25,15 @@ By the domain a user would name: `pricing`, `spend`, `routing`, `mcp`. A file on
|
|||
directory that a `GROUPS` entry in `run.py` selects. There is no manifest and no `covers` marker on new
|
||||
tests. A product bug the test exposes is `pytest.skip("BUG: <symptom>")` at the top of the body, not a
|
||||
fix in the test and not a deletion. Needs no network, proxy, DB or Redis: `tests/unit`
|
||||
|
||||
CircleCI's integration groups run every file here except the OCI and sandbox tests, which no CI runs, and
|
||||
the files in `GITHUB_FILES` in `run.py`. CircleCI runs on main, not on pull requests, so a new file here
|
||||
first runs after its PR merges. Wanting a file to run before merge is no reason to change CI, and a new
|
||||
dedicated workflow for it is the same mistake as a shared shard. Write the file, leave `GITHUB_FILES` and
|
||||
every workflow alone, and tell the user it first runs on main. `GITHUB_FILES` is only for a file that
|
||||
needs what CircleCI's groups cannot give it, like the images `image-scan.yml` builds or the plain pytest
|
||||
run CircleCI's `roi-database` job gives `test_roi_observed.py`. Add a file there only in that case,
|
||||
together with its dedicated job, and list a new dedicated GitHub workflow with the directory it owns in
|
||||
`DEDICATED_INTEGRATION_WORKFLOWS` in `.github/scripts/assert_ci_coverage.py`. The `ci-coverage` check
|
||||
fails a file here that a GitHub workflow runs unless it is in `GITHUB_FILES` and that workflow owns its
|
||||
directory (#46780)
|
||||
|
|
|
|||
|
|
@ -4,7 +4,7 @@ These tests exercise a running gateway, PostgreSQL and Redis with an owned local
|
|||
|
||||
The ROI database contracts in `database/test_roi_observed.py` run as plain pytest outside `run_integration.sh` in CircleCI's `roi-database` Postgres job. The job uploads coverage with the `roi-postgres` flag. These contracts own temporary databases and script only the external provider transport
|
||||
|
||||
The Redis cluster IAM contracts in `sdk/test_redis_cluster_iam_auth.py` start their own `redis-server` and mock the cloud credential clients, so they run in the GitHub Actions `redis-compat` matrix against every supported redis-py major rather than in `run_integration.sh`
|
||||
The Redis cluster IAM contracts in `sdk/test_redis_cluster_iam_auth.py` start their own `redis-server` and mock the cloud credential clients, so CircleCI's `sdk` group installs `redis-server` before it runs them
|
||||
|
||||
The `cost` group is driven by the `CostTrackingTestCase` literals under `cost_calculation/cases/<provider>.py` (gathered by `cost_calculation/catalog.py`), with the scripted cost map in `cost_calculation/cost_map.py`. Each case has a name, contract ID, cost-map model, optional deployment overrides, request body, tagged response and exact or recount expectations. Request bodies use `$MODEL` for the registered proxy model, while responses use `$REQUEST_ID` for the per-run scenario ID. To add a case, add a cost-map entry when the model is new, add the request body and exact provider response data, and add hand-computed expected values. The upstream serves each stored response for any path under `/<scenario_id>`, while the test-owned cost map is served over loopback through `LITELLM_MODEL_COST_MAP_URL`
|
||||
|
||||
|
|
|
|||
|
|
@ -9,6 +9,8 @@ the question neither covers: whether the job that globs a file then deselects it
|
|||
"""
|
||||
|
||||
import importlib.util
|
||||
import shutil
|
||||
import subprocess
|
||||
import sys
|
||||
from pathlib import Path
|
||||
from typing import Final
|
||||
|
|
@ -71,17 +73,27 @@ def test_integration_groups_require_exclusive_scheduled_circleci_owner(tmp_path:
|
|||
workflow.write_text(yaml.safe_dump({"jobs": {"tests": {"steps": [{"run": "pytest tests/integration"}]}}}))
|
||||
_, findings = coverage._integration_ownership(tmp_path)
|
||||
assert [(finding.subject, finding.detail) for finding in findings] == [
|
||||
(test_path, "integration contract is also selected by GitHub Actions")
|
||||
(test_path, "integration file outside GITHUB_FILES is selected by GitHub Actions")
|
||||
]
|
||||
github_path: Final = "tests/integration/management/test_github_contract.py"
|
||||
github_path: Final = "tests/integration/database/image/test_github_contract.py"
|
||||
(tmp_path / github_path).parent.mkdir(parents=True)
|
||||
(tmp_path / github_path).write_text("def test_contract(): pass\n")
|
||||
runner: Final = tmp_path / "tests/integration/run.py"
|
||||
runner.write_text(runner.read_text() + f"GITHUB_FILES: Final = frozenset({{{github_path!r}}})\n")
|
||||
workflow.write_text(yaml.safe_dump({"jobs": {"tests": {"steps": [{"run": f"pytest {github_path}"}]}}}))
|
||||
workflow.unlink()
|
||||
dedicated: Final = tmp_path / ".github/workflows/image-scan.yml"
|
||||
dedicated.write_text(yaml.safe_dump({"jobs": {"tests": {"steps": [{"run": f"pytest {github_path}"}]}}}))
|
||||
github_owned, github_findings = coverage._integration_ownership(tmp_path)
|
||||
assert github_owned == frozenset({test_path, github_path})
|
||||
assert github_findings == ()
|
||||
workflow.write_text(yaml.safe_dump({"jobs": {}}))
|
||||
per_pr: Final = tmp_path / ".github/workflows/test-mcp-interactions.yml"
|
||||
per_pr.write_text(yaml.safe_dump({"jobs": {"mcp": {"steps": [{"run": f"pytest {github_path}"}]}}}))
|
||||
_, shared_findings = coverage._integration_ownership(tmp_path)
|
||||
assert [(finding.subject, finding.detail) for finding in shared_findings] == [
|
||||
(github_path, "GitHub-owned integration contract runs outside a dedicated integration workflow")
|
||||
]
|
||||
per_pr.unlink()
|
||||
dedicated.write_text(yaml.safe_dump({"jobs": {}}))
|
||||
_, missing_invocation = coverage._integration_ownership(tmp_path)
|
||||
assert [(finding.subject, finding.detail) for finding in missing_invocation] == [
|
||||
(github_path, "GitHub-owned integration contract has no invoking job")
|
||||
|
|
@ -94,6 +106,72 @@ def test_integration_groups_require_exclusive_scheduled_circleci_owner(tmp_path:
|
|||
assert circle_invocation == ()
|
||||
|
||||
|
||||
def _write_integration_tree(root: Path, github_path: str) -> None:
|
||||
contracts: Final = root / "tests/integration/management"
|
||||
contracts.mkdir(parents=True)
|
||||
(contracts / "test_contract.py").write_text("def test_contract(): pass\n")
|
||||
(root / github_path).parent.mkdir(parents=True, exist_ok=True)
|
||||
(root / github_path).write_text("def test_contract(): pass\n")
|
||||
(root / "tests/integration/run.py").write_text(
|
||||
"from types import MappingProxyType\nfrom typing import Final\n"
|
||||
'GROUPS: Final = MappingProxyType({"management": ("management",)})\n'
|
||||
f"GITHUB_FILES: Final = frozenset({{{github_path!r}}})\n"
|
||||
)
|
||||
(root / ".circleci").mkdir()
|
||||
(root / ".circleci/config.yml").write_text(
|
||||
yaml.safe_dump(
|
||||
{
|
||||
"jobs": {
|
||||
"integration_contracts": {
|
||||
"steps": [{"run": {"command": "bash .circleci/scripts/run_integration.sh management"}}]
|
||||
}
|
||||
},
|
||||
"workflows": {"integration": {"jobs": [{"integration_contracts": {"suite": "management"}}]}},
|
||||
}
|
||||
)
|
||||
)
|
||||
(root / ".github/workflows").mkdir(parents=True)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("github_path", "workflow_name", "exit_code"),
|
||||
[
|
||||
("tests/integration/database/image/test_github_contract.py", "image-scan.yml", 0),
|
||||
("tests/integration/database/image/test_github_contract.py", "test-unit.yml", 1),
|
||||
("tests/integration/management/test_github_contract.py", "image-scan.yml", 1),
|
||||
],
|
||||
)
|
||||
def test_census_fails_when_a_github_owned_contract_runs_outside_its_dedicated_workflow(
|
||||
tmp_path: Path, github_path: str, workflow_name: str, exit_code: int
|
||||
) -> None:
|
||||
script: Final = tmp_path / ".github/scripts/assert_ci_coverage.py"
|
||||
script.parent.mkdir(parents=True)
|
||||
shutil.copy(_MODULE_PATH, script)
|
||||
_write_integration_tree(tmp_path, github_path)
|
||||
(tmp_path / ".github/workflows" / workflow_name).write_text(
|
||||
yaml.safe_dump({"jobs": {"tests": {"steps": [{"run": f"pytest {github_path}"}]}}})
|
||||
)
|
||||
result: Final = subprocess.run([sys.executable, "-I", str(script)], capture_output=True, text=True, check=False)
|
||||
assert result.returncode == exit_code, result.stdout
|
||||
assert "Playwright" not in result.stdout
|
||||
|
||||
|
||||
def test_an_integration_file_outside_every_group_cannot_run_on_a_github_shard(tmp_path: Path) -> None:
|
||||
_write_integration_tree(tmp_path, "tests/integration/database/image/test_github_contract.py")
|
||||
ungrouped: Final = "tests/integration/ci_probe/test_ci_probe.py"
|
||||
(tmp_path / ungrouped).parent.mkdir(parents=True)
|
||||
(tmp_path / ungrouped).write_text("def test_probe(): pass\n")
|
||||
_, before = coverage._integration_ownership(tmp_path)
|
||||
(tmp_path / ".github/workflows/test-unit.yml").write_text(
|
||||
yaml.safe_dump({"jobs": {"mcp": {"steps": [{"run": f"pytest {ungrouped}"}]}}})
|
||||
)
|
||||
_, after = coverage._integration_ownership(tmp_path)
|
||||
assert [(finding.subject, finding.detail) for finding in after if finding.subject == ungrouped] == [
|
||||
(ungrouped, "integration file outside GITHUB_FILES is selected by GitHub Actions")
|
||||
]
|
||||
assert not [finding for finding in before if finding.subject == ungrouped]
|
||||
|
||||
|
||||
def test_circleci_postgres_suites_count_their_test_path_as_invoked() -> None:
|
||||
config: Final = {
|
||||
"workflows": {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue