From b0aafabdeeb2b126b3e268a621b68c1ae5a32472 Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Tue, 1 Sep 2026 22:30:46 -0700 Subject: [PATCH] ci(lint): gate top-level tests/e2e and litellm files in the diff-scoped lint steps Without :(glob), git matches 'tests/e2e/**/*.py' with * crossing slashes, so the pattern needs at least one directory below tests/e2e and a top-level file never matches. PR #39209 added tests/e2e/test_junit_properties.py with three basedpyright errors and the e2e step printed "No changed tests/e2e Python files; skipping." The ruff format step's 'litellm/**/*.py' skipped litellm/main.py and the other top-level modules the same way. :(glob) makes /**/ match zero or more directories, so both gates now select top-level and nested files alike. A regression test runs the workflow's own pathspecs against a throwaway repo and locks that in for every diff-scoped gate. --- .github/workflows/test-linting.yml | 4 +- Makefile | 4 +- .../test_lint_workflow_diff_gates.py | 54 +++++++++++++++++++ 3 files changed, 58 insertions(+), 4 deletions(-) create mode 100644 tests/test_litellm/test_lint_workflow_diff_gates.py diff --git a/.github/workflows/test-linting.yml b/.github/workflows/test-linting.yml index 9f1283da19e..06d369eabcd 100644 --- a/.github/workflows/test-linting.yml +++ b/.github/workflows/test-linting.yml @@ -113,7 +113,7 @@ jobs: - name: Check ruff format if: steps.changes.outputs.decision != 'skip' run: | - git diff --name-only --diff-filter=ACMR "$GATE_BASE_SHA" HEAD -- 'litellm/**/*.py' | grep -v '^litellm/enterprise/' > "$RUNNER_TEMP/ruff_format_files.txt" || true + git diff --name-only --diff-filter=ACMR "$GATE_BASE_SHA" HEAD -- ':(glob)litellm/**/*.py' | grep -v '^litellm/enterprise/' > "$RUNNER_TEMP/ruff_format_files.txt" || true if [ ! -s "$RUNNER_TEMP/ruff_format_files.txt" ]; then echo "No changed litellm Python files to check with ruff format." exit 0 @@ -172,7 +172,7 @@ jobs: - name: Check tests/e2e basedpyright (zero errors) if: steps.changes.outputs.decision != 'skip' run: | - if git diff --name-only --diff-filter=ACMRD "$GATE_BASE_SHA" HEAD -- 'tests/e2e/**/*.py' | grep -q .; then + if git diff --name-only --diff-filter=ACMRD "$GATE_BASE_SHA" HEAD -- ':(glob)tests/e2e/**/*.py' | grep -q .; then uv run --no-sync basedpyright tests/e2e else echo "No changed tests/e2e Python files; skipping." diff --git a/Makefile b/Makefile index e17fdba3c85..ea15e7da8ab 100644 --- a/Makefile +++ b/Makefile @@ -147,8 +147,8 @@ lint-install: # Diff-scoped format check, mirroring test-linting.yml's "Check ruff format" step: # only the litellm Python files changed vs the base are checked, so a pre-existing # format issue elsewhere doesn't block an unrelated commit. Git pathspecs match -# recursively, so 'litellm/*.py' covers nested modules and the top-level files that -# CI's 'litellm/**/*.py' skips, which makes this target a superset of the CI step. +# recursively, so 'litellm/*.py' covers top-level files and nested modules alike, +# the same set CI's ':(glob)litellm/**/*.py' selects. lint-format-check-changed: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) @files=$$(git diff --name-only --diff-filter=ACMR origin/litellm_internal_staging...HEAD -- 'litellm/*.py' | grep -v '^litellm/enterprise/' || true); \ if [ -z "$$files" ]; then \ diff --git a/tests/test_litellm/test_lint_workflow_diff_gates.py b/tests/test_litellm/test_lint_workflow_diff_gates.py new file mode 100644 index 00000000000..62e67cdaaab --- /dev/null +++ b/tests/test_litellm/test_lint_workflow_diff_gates.py @@ -0,0 +1,54 @@ +from __future__ import annotations + +import re +import shlex +import subprocess +from pathlib import Path +from typing import Final + +import pytest + +WORKFLOW: Final = Path(__file__).resolve().parents[2] / ".github" / "workflows" / "test-linting.yml" +DIFF_GATE: Final = re.compile(r'git diff --name-only --diff-filter=\w+ "\$GATE_BASE_SHA" HEAD -- (.+?) \|') +GATES: Final = tuple(tuple(shlex.split(gate.group(1))) for gate in DIFF_GATE.finditer(WORKFLOW.read_text())) + + +def _git(cwd: Path, *args: str) -> str: + return subprocess.run(["git", *args], cwd=cwd, check=True, capture_output=True, text=True).stdout + + +def _scoped_root(pathspec: str) -> str: + return re.sub(r"^:\([^)]*\)", "", pathspec).split("*", 1)[0] + + +def _changed_files_selected_by(tmp_path: Path, pathspecs: tuple[str, ...], files: tuple[str, ...]) -> frozenset[str]: + _git(tmp_path, "init", "-q", "-b", "main") + _git(tmp_path, "config", "user.email", "t@t") + _git(tmp_path, "config", "user.name", "t") + _git(tmp_path, "commit", "-q", "--allow-empty", "-m", "base") + for name in files: + target = tmp_path / name + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text("x = 1\n") + _git(tmp_path, "add", "-A") + _git(tmp_path, "commit", "-qm", "change") + return frozenset( + _git(tmp_path, "diff", "--name-only", "--diff-filter=ACMRD", "HEAD~1", "HEAD", "--", *pathspecs).split() + ) + + +def test_workflow_still_carries_the_ruff_format_and_e2e_basedpyright_diff_gates() -> None: + assert frozenset(_scoped_root(gate[0]) for gate in GATES) == frozenset({"litellm/", "tests/e2e/"}) + + +@pytest.mark.parametrize("pathspecs", GATES, ids=" ".join) +def test_diff_gate_selects_top_level_and_nested_python_files_only(tmp_path: Path, pathspecs: tuple[str, ...]) -> None: + root = _scoped_root(pathspecs[0]) + top_level = f"{root}top_level_module.py" + nested = f"{root}pkg/sub/nested_module.py" + selected = _changed_files_selected_by( + tmp_path, + pathspecs, + (top_level, nested, f"{root}notes.md", "elsewhere/top_level_module.py", "elsewhere/pkg/nested_module.py"), + ) + assert selected == frozenset({top_level, nested})