Skip to content

test: merge three stranded twins into the files that shadow them - #37600

Merged
yuneng-berri merged 2 commits into
litellm_internal_stagingfrom
litellm_/merge-mirror-twins
Aug 20, 2026
Merged

test: merge three stranded twins into the files that shadow them#37600
yuneng-berri merged 2 commits into
litellm_internal_stagingfrom
litellm_/merge-mirror-twins

Conversation

@yuneng-berri

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

Copy link
Copy Markdown
Contributor

#37595 has merged, so this now stands alone against staging and holds only the four files it could not move.

TLDR

Problem this solves:

  • Four files could not move: each shares a name with a live test
  • Three of them collide with nothing inside that live file
  • So their tests are extra coverage, not a competing version
  • They have never run, which is why one assertion had drifted

How it solves it:

  • Appends those classes to the live file of the same name
  • 156 collected tests become 196, and all 196 pass
  • Leaves the fourth, where the twin was rewritten past it

User Flow

Before: two files named the same thing, one of which is invisible

  1. Someone debugging OCI key handling opens tests/test_litellm/llms/oci/chat/test_oci_chat_transformation.py and finds no coverage of CRLF or escaped newlines in oci_key
  2. That coverage exists, in a file of the identical name under tests/litellm/, which no job runs
  3. They either write it again or conclude it is untested
  4. Meanwhile the DeepSeek copy asserts reasoning_effort="none" produces no thinking key, and nothing notices that the handler has mapped it to {"type": "disabled"} for some time

After: one file per name, and it holds everything

  1. The same file now carries TestOCIKeyNormalization, TestOCIValidateEnvironment, TestOCIGetCompleteUrl and TestOCIImageUrlTransformation
  2. Those 13 cases run on every PR in the llms shard
  3. The DeepSeek test asserts the disabled mapping the handler documents, so it now pins the behaviour instead of contradicting it
  4. tests/litellm/ is down to a single file, and the allowlist says exactly why that one is different

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

Nothing collided

The merge is only safe if no name in the stranded file already exists in the live one, since a later definition would shadow the earlier silently.

  1. Command:
python3 - <<'PY'
import ast
for rel in ("llms/oci/chat/test_oci_chat_transformation.py",
            "proxy/management_endpoints/test_common_utils.py",
            "llms/deepseek/chat/test_deepseek_chat_transformation.py"):
    stale = ast.parse(open("tests/litellm/" + rel).read())
    live = ast.parse(open("tests/test_litellm/" + rel).read())
    live_names = {getattr(n, "name", None) for n in live.body}
    stale_names = [getattr(n, "name", None) for n in stale.body if getattr(n, "name", None)]
    print(rel)
    print("   collisions:", sorted(n for n in stale_names if n in live_names))
    print("   unique    :", sorted(n for n in stale_names if n not in live_names))
PY
  1. Output:
llms/oci/chat/test_oci_chat_transformation.py
   collisions: []
   unique    : ['TestOCIGetCompleteUrl', 'TestOCIImageUrlTransformation', 'TestOCIKeyNormalization', 'TestOCIValidateEnvironment', 'config']
proxy/management_endpoints/test_common_utils.py
   collisions: []
   unique    : ['TestHasNonEmptyValue', 'TestUpdateMetadataFieldsPremiumCheck']
llms/deepseek/chat/test_deepseek_chat_transformation.py
   collisions: []
   unique    : ['TestDeepSeekThinkingParams']

Before

  1. Command:
FILES=(tests/test_litellm/llms/oci/chat/test_oci_chat_transformation.py tests/test_litellm/proxy/management_endpoints/test_common_utils.py tests/test_litellm/llms/deepseek/chat/test_deepseek_chat_transformation.py)
uv run pytest "${FILES[@]}" --collect-only -q | tail -1
  1. Output:
156 tests collected in 0.21s

After

40 more tests, all green

  1. Command:
uv run pytest "${FILES[@]}" --collect-only -q | tail -1
uv run pytest "${FILES[@]}" -q | tail -1
  1. Output:
196 tests collected in 0.23s
196 passed, 72 warnings in 22.20s

The one assertion that changed, and why

The handler decides this deliberately, and says so:

# Otherwise fall back to reasoning_effort: "none" disables, anything else enables
elif reasoning_effort is not None:
    optional_params["thinking"] = {"type": "disabled" if reasoning_effort == "none" else "enabled"}
  1. Command, on the unmerged file:
uv run pytest tests/litellm/llms/deepseek -q
  1. Output:
E       AssertionError: assert 'thinking' not in {'thinking': {'type': 'disabled'}}
FAILED tests/litellm/llms/deepseek/chat/test_deepseek_chat_transformation.py::TestDeepSeekThinkingParams::test_map_reasoning_effort_none_does_not_enable_thinking
  1. Command, after asserting the mapping instead of the key's absence:
uv run pytest tests/test_litellm/llms/deepseek -q
  1. Output:
25 passed, 1 warning in 0.53s

The OCI suite no longer reads credentials from the environment

validate_environment falls back to os.environ for every OCI credential, so these tests would find credentials they never passed on a machine with OCI configured. A fixture drops the seven variables for the appended classes and for TestOCIChatConfig, which had the same dependency already.

  1. Command:
uv run pytest tests/test_litellm/llms/oci/chat/test_oci_chat_transformation.py -q
OCI_USER=ocid1.user.oc1..fake OCI_REGION=eu-frankfurt-1 OCI_TENANCY=ocid1.tenancy.oc1..fake OCI_FINGERPRINT=aa:bb OCI_KEY=x \
  uv run pytest tests/test_litellm/llms/oci/chat/test_oci_chat_transformation.py -q
  1. Output, the same either way:
83 passed, 1 warning in 0.24s
83 passed, 1 warning in 0.22s

The pre-existing file gives 2 failed, 68 passed under the second command.

The shards they live in still collect

  1. Command:
uv run pytest tests/test_litellm/llms/oci tests/test_litellm/llms/deepseek tests/test_litellm/proxy/management_endpoints --collect-only -q | tail -1
  1. Output:
3219 tests collected in 3.49s

No new lint

  1. Command, the merged OCI file against its own pre-merge version:
uv run ruff check tests/test_litellm/llms/oci/chat/test_oci_chat_transformation.py | grep -oE "[A-Z][0-9]{3}" | sort | uniq -c
git show HEAD:tests/test_litellm/llms/oci/chat/test_oci_chat_transformation.py > /tmp/base_oci.py
uv run ruff check /tmp/base_oci.py | grep -oE "[A-Z][0-9]{3}" | sort | uniq -c
  1. Output, identical, so the four are pre-existing:
   4 I001
   4 I001

What is left

  1. Command:
find tests/litellm -type f
  1. Output:
tests/litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py

Type

🧹 Refactoring
✅ Test

Caveats

  • Stacked on test: run the 30 test files stranded in the second mirror #37595; merge that first
  • No live test was edited, only appended to
  • The single changed assertion follows the handler's documented mapping
  • TestOCIChatConfig gains the env fixture too; it had the same latent dependency before this branch
  • The last file needs an endpoint-by-endpoint call, so it stays exempt with that written down

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 consolidates previously uncollected duplicate tests into the active test tree and updates the coverage allowlist. The follow-up adds an environment-isolation fixture that clears every OCI environment fallback used by the affected credential-validation and URL-generation paths.

  • Merges DeepSeek, OCI, and management-endpoint tests into their actively collected counterparts.
  • Updates the DeepSeek disabled-thinking expectation to match current behavior.
  • Removes the merged duplicate files and narrows the coverage allowlist.
  • Isolates OCI tests from ambient credentials and region configuration.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
tests/test_litellm/llms/oci/chat/test_oci_chat_transformation.py Adds the merged OCI coverage and clears all seven environment variables consulted by OCI credential resolution, resolving the prior nondeterminism.
tests/test_litellm/llms/deepseek/chat/test_deepseek_chat_transformation.py Consolidates the stranded DeepSeek tests and aligns the disabled-thinking assertion with the handler’s current mapping.
tests/test_litellm/proxy/management_endpoints/test_common_utils.py Consolidates helper and premium-field behavior tests into the actively collected suite.
.github/ci-coverage-allowlist.yml Removes the three consolidated test files from the allowlist and documents why the sole remaining duplicate is still exempt.

Reviews (2): Last reviewed commit: "test(oci): stop the OCI suite reading cr..." | Re-trigger Greptile

Comment thread tests/test_litellm/llms/oci/chat/test_oci_chat_transformation.py
@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
Contributor Author

Fixed the ambient OCI env issue with a fixture, and applied it to TestOCIChatConfig which had the same latent dependency. @greptileai

Base automatically changed from litellm_/second-mirror-resolution to litellm_internal_staging August 20, 2026 17:59
@yucheng-berri
yucheng-berri requested a review from a team August 20, 2026 17:59
The second mirror's last four files each share a filename with a live test, so
the previous commit could not move them. Three of the four turn out to be plain
additions: their classes collide with nothing in the live file, so the tests are
extra coverage that has sat unrun rather than a competing version of anything.

Appending them takes the three files from 156 collected tests to 196, and all
196 pass. The 40 recovered are 13 OCI cases covering key normalization,
credential validation, complete-URL building and image-url transformation, 15
management-endpoint cases covering empty-value handling and the premium check,
and 12 DeepSeek thinking-parameter cases.

One assertion had to change. test_map_reasoning_effort_none_does_not_enable_thinking
asserted that reasoning_effort='none' leaves no thinking key, while the handler
maps it to {'type': 'disabled'} on purpose, documented in map_openai_params as
the OpenAI-style way to ask for thinking off. The test's stated intent holds,
since disabled does not enable anything, so it now asserts the disabled mapping
instead of the key's absence. Two imports moved to module scope for the
appended code, and no live test was touched.

test_discoverable_endpoints.py is the one left. Its twin grew from 1268 lines
to 9434, 25 of its assertions fail against today's code, and only 5 of its 19
tests have no counterpart, so deciding what survives that rewrite is a
judgement about the endpoints rather than a merge. The allowlist now holds
exactly that file and that reasoning.
validate_environment falls back to os.environ for every OCI credential and only
defaults the region when OCI_REGION is unset, so on a machine with OCI
configured the missing-credential test finds credentials it never passed and the
default-region test builds a URL for the ambient region. The suite then passes
or fails depending on who runs it.

A fixture drops the seven OCI variables for the four classes this branch added
and for TestOCIChatConfig, which had the same dependency before any of this and
fails the same way: with OCI_USER and friends exported, two of its cases fail on
origin/litellm_internal_staging today.

  clean env:        83 passed
  ambient OCI env:  83 passed

Same numbers either way, where the pre-existing file gave 68 passed / 2 failed
under the second.
@yuneng-berri
yuneng-berri force-pushed the litellm_/merge-mirror-twins branch from 496c6e9 to 4fc09cc 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.

Merges 3 of 4 stranded never-run test files into their live twins (156 -> 196 collected tests, all passing), with an AST-based collision check proving no name clashes before merging. The one real behavior fix (DeepSeek reasoning_effort='none' mapping) is justified against the handler's own documented mapping rather than guessed. Correctly adds an env-var-clearing fixture for the appended OCI tests since validate_environment falls back to os.environ. Fourth file left out with a stated reason (twin was rewritten past it). CI green.

@yuneng-berri
yuneng-berri merged commit 8a18e24 into litellm_internal_staging Aug 20, 2026
68 checks passed
@yuneng-berri
yuneng-berri deleted the litellm_/merge-mirror-twins branch August 20, 2026 21:25
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