Skip to content

feat(ci): freeze the conftest save/restore inventory so it can only shrink - #37621

Merged
yuneng-berri merged 4 commits into
litellm_internal_stagingfrom
litellm_/conftest-inventory-ratchet
Aug 20, 2026
Merged

feat(ci): freeze the conftest save/restore inventory so it can only shrink#37621
yuneng-berri merged 4 commits into
litellm_internal_stagingfrom
litellm_/conftest-inventory-ratchet

Conversation

@yuneng-berri

@yuneng-berri yuneng-berri commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

#37588 and #37612 have both merged, so this now stands alone against staging.

TLDR

Problem this solves:

  • Six conftests save and restore litellm globals around every test
  • The lists are hand-maintained inventories of leaks the suite knows about
  • Nothing stops them growing, and growth is the cheap fix every time
  • 117 globals today, across six conftests

How it solves it:

  • New TQ007 reports each snapshotted global, one line each
  • Seeds the budget at today's 117, so the inventory can only shrink
  • Adding a global to any of the six now fails the gate
  • Removing one and leaving the ceiling at 117 now fails too, for every TQ rule
  • So the inventory does not just avoid growing, it cannot quietly regrow

User Flow

Before: a leaking global is papered over instead of fixed

  1. A test starts failing because an earlier test left litellm.some_flag set
  2. The quickest green is one more line in the conftest's save/restore list
  3. CI passes, the leak is now permanent, and the next person inherits both the global and the machinery
  4. The list has reached 491 lines this way, which is what makes the suite order-dependent in the first place

After: the shortcut is closed and the real fix is the cheap one

  1. The same PR adds the global to the conftest list
  2. test-quality-gate fails: TQ007: total 55 over limit 54 (this change added 1)
  3. The author gives the consumer an injection seam, or suppresses with a reason that goes on the record
  4. Every removal ratchets the ceiling down, so the number tracks real progress

Relevant issues

Linear ticket

Pre-Submission checklist

  • I have added meaningful tests
  • The handful of test files covering my change pass locally, e.g. uv run pytest tests/test_litellm/<your_test_file>.py -v
  • My PR passes all required CI/CD checks (e.g., lint, schema.d.ts sync check, etc.)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review

Screenshots / Proof of Fix

Before (b4b5611)

  1. Command:
python3 scripts/check_test_quality.py tests/ | grep -c TQ007
  1. Output, the rule does not exist yet:
0

After (6e19208)

The inventory, measured

  1. Command:
python3 scripts/check_test_quality.py tests/ | grep TQ007 | cut -d: -f1 | sort | uniq -c | sort -rn
  1. Output:
  34 tests/test_litellm/conftest.py
  24 tests/local_testing/conftest.py
  19 tests/logging_callback_tests/conftest.py
  16 tests/llm_translation/conftest.py
  16 tests/batches_tests/conftest.py
   8 tests/guardrails_tests/conftest.py

Green at the seeded ceiling

  1. Command:
python3 scripts/test_quality_gate.py
  1. Output:
OK: every TQ rule is within its test-suite ceiling (base origin/litellm_internal_staging)

Red the moment the list grows

  1. Command, adding one name to the _SCALAR_ATTRS tuple tests/batches_tests/conftest.py snapshots through a helper:
sed -i '' 's/^_SCALAR_ATTRS = ($/_SCALAR_ATTRS = (\n    "a_brand_new_global",/' tests/batches_tests/conftest.py
python3 scripts/test_quality_gate.py
  1. Output:
FAIL: TQ-rule totals exceed their limit (base origin/litellm_internal_staging):
  TQ007: total 118 over limit 117 (this change added 1)

Unit tests

  1. Command:
uv run pytest tests/test_litellm/test_check_test_quality.py tests/test_litellm/test_test_quality_gate.py -q
  1. Output:
72 passed, 1 warning in 1.18s

Type

🆕 New Feature
✅ Test

Caveats

  • The rule fires on any conftest.py that stores a value read off litellm into a dict under that attribute's name, whatever the dict is called
  • Names come from the direct keys and from whatever the save loop iterates, including a module-level tuple or dict it names rather than spells out
  • The read may sit a statement above the store, which is the shape most of them use
  • A loop over something the module does not define (dir(litellm)) is skipped rather than guessed at
  • A global saved in two places counts once, so the number is globals and not lines
  • It counts the inventory; shrinking it is the WS1 dependency-injection arc, not this PR

Final Attestation

  • The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR

@greptile-apps

greptile-apps Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds TQ007 to freeze the known conftest snapshot inventory and updates the quality gate so reduced counts must be accompanied by lower ceilings.

  • Detects direct, named, and helper-built litellm snapshot inventories.
  • Seeds the current inventory budget at 117.
  • Enforces one-way ratcheting and adds focused detector and gate tests.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
scripts/check_test_quality.py Adds TQ007 detection, including the named mappings and helper-local snapshot dictionaries raised in the previous threads.
scripts/test_quality_gate.py Enforces ceiling updates after violations are removed and measures base counts using the current checker.
test-quality-budget.json Seeds TQ007 at the documented current inventory count of 117.
tests/test_litellm/test_check_test_quality.py Adds focused coverage for direct, named, deduplicated, suppressible, and helper-built snapshot inventories.
tests/test_litellm/test_test_quality_gate.py Covers the one-way budget update and stale-ceiling detection behavior.

Reviews (8): Last reviewed commit: "feat(ci): fail a branch that clears TQ v..." | Re-trigger Greptile

Comment thread scripts/check_test_quality.py
Comment thread test-quality-budget.json
@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

Good catch: two conftests name their inventory via _SCALAR_DEFAULTS. Resolving it takes the count from 54 to 82.

@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

@greptileai

@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

TQ007 ratchets exactly like TQ001-006 and the other three budgets: make lint-budget-update lowers limits, per CLAUDE.md. Changing it here would fork one rule's semantics.

@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

@greptileai

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

@greptileai

Comment thread scripts/check_test_quality.py Outdated
@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

Right again: batches_tests stores into a local dict in a helper. Matching the shape, not dict names, finds 117 across six conftests.

@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

@greptileai

@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

The seeded exemption is branch-scoped: it reads the merge-base budget, so TQ007 ratchets normally on every branch after this merges.

@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

@greptileai

@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

buildkite e2e has canceled every build org-wide since 12:17Z: 22 builds, 7 branches, none started. Not this PR.

@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

scripts/test_quality_gate.py is not in this diff. The ratchet path shipped in #37588 at 5/5; TQ007 only registers a rule against it.

@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

@greptileai

@yuneng-berri
yuneng-berri force-pushed the litellm_/credential-gated-skip-budget branch from b4b5611 to 04d62ad Compare August 20, 2026 17:12
@yuneng-berri
yuneng-berri force-pushed the litellm_/conftest-inventory-ratchet branch from 6e19208 to c98eaa5 Compare August 20, 2026 17:12
Base automatically changed from litellm_/credential-gated-skip-budget to litellm_internal_staging August 20, 2026 17:59
@yucheng-berri
yucheng-berri requested a review from a team August 20, 2026 17:59
@yuneng-berri
yuneng-berri force-pushed the litellm_/conftest-inventory-ratchet branch from ffea6b9 to f3575d7 Compare August 20, 2026 18:03

@tin-berri tin-berri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New TQ007 rule freezes the conftest global save/restore inventory (117 globals across 6 conftests) so it can only shrink, closing the 'just add another line' shortcut that let it grow to 491 lines. Seeded budget + red-the-moment-it-grows repro is solid. Caveats honestly scope what the rule does and doesn't detect (loop-based saves vs directly spelled names, skips undefined-module loops rather than guessing). CI green.

…e ceiling

A limit that only ever falls is not the same as one that falls when it can.
Clearing violations and leaving the ceiling above the new count let the same
violations return later under a limit nobody moved, so the gate now fails on
that and names `make lint-budget-update` as the fix. It needs both head below
base and head below limit, so headroom already in the base is never blamed on
the branch that happens to run next.

Drops the seeded-rule exemption from the ratchet along with it. Its stated
reason was that the base tree predates a rule introduced on this branch, but
base counts are measured with the current checker, so such a rule is counted at
the base too and its grandfathered total was never at risk of reading as fixed.
Removing the exemption is what lets a newly seeded rule ratchet like the six
that came before it.

The base scan is skipped when the branch touches neither the test tree nor the
checker, since neither count can have moved.
@yuneng-berri
yuneng-berri force-pushed the litellm_/conftest-inventory-ratchet branch from f3575d7 to 990339a Compare August 20, 2026 21:31
@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

Implemented it: the gate now fails a branch that clears violations and leaves the ceiling above the new count, for all seven rules.

@yuneng-berri

Copy link
Copy Markdown
Collaborator Author

@greptileai

@yuneng-berri
yuneng-berri enabled auto-merge (squash) August 20, 2026 21:34
@yuneng-berri
yuneng-berri merged commit 4af6665 into litellm_internal_staging Aug 20, 2026
67 checks passed
@yuneng-berri
yuneng-berri deleted the litellm_/conftest-inventory-ratchet branch August 20, 2026 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants