Skip to content

Fix mock test#21475

Merged
Sameerlite merged 2 commits intomainfrom
litellm_fix_mock_test
Feb 18, 2026
Merged

Fix mock test#21475
Sameerlite merged 2 commits intomainfrom
litellm_fix_mock_test

Conversation

@Sameerlite
Copy link
Copy Markdown
Contributor

Relevant issues

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have Added testing in the tests/litellm/ directory, Adding at least 1 test is a hard requirement - see details
  • My PR passes all unit tests on make test-unit
  • My PR's scope is as isolated as possible, it only solves 1 specific problem
  • I have requested a Greptile review by commenting @greptileai and received a Confidence Score of at least 4/5 before requesting a maintainer review

CI (LiteLLM team)

CI status guideline:

  • 50-55 passing tests: main is stable with minor issues.
  • 45-49 passing tests: acceptable but needs attention
  • <= 40 passing tests: unstable; be careful with your merges and assess the risk.
  • Branch creation CI run
    Link:

  • CI run for the last commit
    Link:

  • Merge / cherry-pick CI run
    Links:

Type

🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test

Changes

@vercel
Copy link
Copy Markdown

vercel bot commented Feb 18, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
litellm Ready Ready Preview, Comment Feb 18, 2026 1:50pm

Request Review

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Feb 18, 2026

Greptile Summary

This PR makes two changes: (1) replaces prisma.Json() with safe_dumps() in _rotate_master_key for serializing litellm_params and model_info before create_many(), and (2) adds proxy_logging_obj mocks to three test functions in test_object_permission_loading.py to fix test failures caused by the log_db_metrics decorator.

  • The test mock additions for proxy_logging_obj are correct and necessary — the log_db_metrics decorator imports proxy_logging_obj from proxy_server at runtime to log DB metrics, so tests must mock it.
  • The prisma.Json()safe_dumps() change in _rotate_master_key is problematic: the existing regression test test_rotate_master_key_model_data_valid_for_prisma explicitly asserts that these fields must be prisma.Json instances (not JSON strings), and will fail with this change. The PR does not include evidence that the fix works (no test updates, no passing test output).
  • The PR description is empty — no linked issues, no explanation of what was broken or why safe_dumps is the correct replacement.

Confidence Score: 2/5

  • This PR will break the existing test test_rotate_master_key_model_data_valid_for_prisma and may introduce a runtime regression in _rotate_master_key.
  • The test mocking changes are safe, but the prisma.Json() to safe_dumps() change directly contradicts an existing regression test that validates the type must be prisma.Json. The PR provides no evidence the change works and does not update the conflicting test.
  • litellm/proxy/management_endpoints/key_management_endpoints.py — the safe_dumps change at lines 3099-3100 needs verification against create_many() and the existing test needs updating.

Important Files Changed

Filename Overview
litellm/proxy/management_endpoints/key_management_endpoints.py Replaces prisma.Json() with safe_dumps() in _rotate_master_key, changing type from dict wrapper to JSON string — breaks existing test test_rotate_master_key_model_data_valid_for_prisma and may cause runtime issues with create_many().
tests/test_litellm/proxy/auth/test_object_permission_loading.py Adds necessary mocks for proxy_logging_obj to fix test failures caused by the log_db_metrics decorator importing and calling service hooks at runtime. The mocking approach is correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[_rotate_master_key] --> B[Fetch models from DB]
    B --> C[Decrypt model list]
    C --> D[Re-encrypt with new key via _add_model_to_db]
    D --> E[model_dump with exclude_none]
    E --> F{"Serialize litellm_params & model_info"}
    F -->|Before PR| G["prisma.Json(dict) — dict wrapper"]
    F -->|After PR| H["safe_dumps(dict) — JSON string"]
    G --> I[create_many in transaction]
    H --> I
    I --> J[Delete old models + Insert new models]

    style G fill:#90EE90
    style H fill:#FFB6C1
Loading

Last reviewed commit: 610bf00

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +3099 to +3100
_dumped["litellm_params"] = safe_dumps(_dumped["litellm_params"])
_dumped["model_info"] = safe_dumps(_dumped["model_info"])
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.

Breaking existing test and likely runtime behavior

Replacing prisma.Json(...) with safe_dumps(...) changes the type from a prisma.Json wrapper (which wraps a Python dict) to a JSON string. The existing test test_rotate_master_key_model_data_valid_for_prisma in tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py at lines 5716-5724 explicitly asserts that these fields must be prisma.Json instances, not JSON strings:

assert isinstance(model_data["litellm_params"], prisma.Json), (
    f"litellm_params should be prisma.Json for create_many(), got {type(model_data['litellm_params'])}"
)

This change will cause that test to fail. Additionally, Prisma's create_many() for Json fields may not accept raw JSON strings the same way create() does — the original code used prisma.Json() specifically for create_many() compatibility.

If the intent is to remove the prisma import dependency here, the existing test also needs to be updated, and you should verify that create_many() actually accepts JSON strings for Json fields (or use json.loads(safe_dumps(...)) to get back a dict).

Context Used: Rule from dashboard - What: Ensure that any PR claiming to fix an issue includes evidence that the issue is resolved, such... (source)

@Sameerlite Sameerlite merged commit a358b7a into main Feb 18, 2026
63 of 84 checks passed
@ishaan-berri ishaan-berri deleted the litellm_fix_mock_test branch March 26, 2026 22:29
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.

1 participant