Skip to content

Fix test isolation for test_log_langfuse_v2_handles_null_usage_values#20475

Merged
ishaan-jaff merged 1 commit intomainfrom
litellm_fix_test_log_langfuse_v2_handles_null_usage_values_1017c3a
Feb 5, 2026
Merged

Fix test isolation for test_log_langfuse_v2_handles_null_usage_values#20475
ishaan-jaff merged 1 commit intomainfrom
litellm_fix_test_log_langfuse_v2_handles_null_usage_values_1017c3a

Conversation

@shin-bot-litellm
Copy link
Contributor

Regression Fix

Failing Job: litellm_mapped_tests_integrations
Caused By: Test isolation issue introduced in 1017c3a
Author: @ishaan-jaff

What Broke

Test test_log_langfuse_v2_handles_null_usage_values fails intermittently when running with parallel test execution.

Error:

AssertionError: Expected 'generation' to have been called once. Called 0 times.

Original Commit LOC

The test originally:

  1. Called reset_mock() on setUp's mocks
  2. Tried to reconfigure return_value on mocks that had side_effect configured
  3. side_effect takes precedence over return_value, causing mock chain issues

This Fix

Create fresh mock objects within the test instead of reusing mocks from setUp:

  • Avoids state pollution from setUp's side_effect configuration
  • Makes the test deterministic regardless of setUp's mock configuration
  • Properly configures the mock chain: client.trace() → trace.generation()

This ensures the test is deterministic in parallel execution environments.

Create fresh mock objects within the test instead of reusing mocks
from setUp that have side_effect configured. The setUp's side_effect
on mock_langfuse_client.trace can interfere with return_value settings
when tests try to reset and reconfigure mocks.

Using dedicated mock objects for this test avoids state pollution
from setUp's side_effect configuration and makes the test more
deterministic in parallel execution environments.
@vercel
Copy link

vercel bot commented Feb 5, 2026

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

Project Deployment Actions Updated (UTC)
litellm Ready Ready Preview, Comment Feb 5, 2026 6:32am

Request Review

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 5, 2026

Greptile Overview

Greptile Summary

This PR fixes a test isolation issue in test_log_langfuse_v2_handles_null_usage_values that caused intermittent failures in parallel test execution.

Root cause: The test's setUp method configures mock_langfuse_client.trace.side_effect (line 56), which persists after reset_mock(). When the failing test tried to reconfigure mocks using return_value, the side_effect took precedence, breaking the mock chain and causing generation to never be called.

Solution: Create fresh MagicMock objects scoped to the test instead of reusing setUp's mocks. This eliminates state pollution and ensures the mock chain works correctly: client.trace()trace.generation() → assertions pass.

Changes:

  • Removed reset_mock() calls that were ineffective
  • Created fresh mock_client, mock_trace, mock_generation, and mock_span within the test
  • Properly configured mock chain with return_value instead of fighting side_effect
  • Updated assertions to use the new local mocks instead of self.mock_langfuse_*

Confidence Score: 5/5

  • Safe to merge - this is a focused test fix with no production code changes
  • The fix correctly addresses a well-understood mock isolation issue by replacing shared mocks with test-scoped mocks, eliminating the side_effect vs return_value conflict that caused test flakiness
  • No files require special attention

Important Files Changed

Filename Overview
tests/test_litellm/integrations/test_langfuse.py Replaced shared mocks with fresh test-scoped mocks to fix race condition caused by side_effect precedence over return_value

Sequence Diagram

sequenceDiagram
    participant Test as test_log_langfuse_v2_handles_null_usage_values
    participant Logger as LangFuseLogger
    participant MockClient as mock_client
    participant MockTrace as mock_trace
    participant MockGen as mock_generation
    
    Note over Test: OLD: Used setUp's mocks with side_effect
    Note over Test: NEW: Creates fresh mocks
    
    Test->>Test: Create mock_client, mock_trace, mock_generation
    Test->>Test: Configure chain: client.trace() → trace
    Test->>Test: Configure chain: trace.generation() → generation
    Test->>Logger: Assign mock_client to logger.Langfuse
    
    Test->>Logger: Call _log_langfuse_v2(response_obj with null usage)
    Logger->>MockClient: trace(name, metadata, ...)
    MockClient-->>Logger: Returns mock_trace
    Logger->>MockTrace: generation(usage={prompt_tokens:0, ...})
    MockTrace-->>Logger: Returns mock_generation
    
    Test->>MockClient: assert trace.assert_called()
    Test->>MockTrace: assert generation.assert_called_once()
    Test->>Test: Verify usage values converted None→0
Loading

Copy link
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.

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@ishaan-jaff ishaan-jaff merged commit 0649720 into main Feb 5, 2026
61 of 66 checks passed
jquinter added a commit that referenced this pull request Feb 14, 2026
Fixes test_log_langfuse_v2_handles_null_usage_values flaky test failure
by properly cleaning up sys.modules['langfuse'] in tearDown.

Changes:
- Store original langfuse module in setUp before mocking
- Restore original or remove mock in tearDown to prevent state pollution
- Remove invalid print_verbose parameter from log_event_on_langfuse

Root Cause:
The tearDown method was not cleaning up sys.modules['langfuse'] after
each test, causing mock state to leak between tests. This caused
intermittent failures in CI, especially when tests run in parallel or
in different orders.

Impact:
This test has a long history of flakiness with multiple attempted fixes
(#20475, #17599, #17594, #17591, #17588). The missing sys.modules cleanup
was the underlying issue causing continued failures despite those patches.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
jquinter added a commit that referenced this pull request Feb 15, 2026
Fixes test_log_langfuse_v2_handles_null_usage_values flaky test failure
by properly cleaning up sys.modules['langfuse'] in tearDown.

Changes:
- Store original langfuse module in setUp before mocking
- Restore original or remove mock in tearDown to prevent state pollution
- Remove invalid print_verbose parameter from log_event_on_langfuse

Root Cause:
The tearDown method was not cleaning up sys.modules['langfuse'] after
each test, causing mock state to leak between tests. This caused
intermittent failures in CI, especially when tests run in parallel or
in different orders.

Impact:
This test has a long history of flakiness with multiple attempted fixes
(#20475, #17599, #17594, #17591, #17588). The missing sys.modules cleanup
was the underlying issue causing continued failures despite those patches.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
sameetn pushed a commit to sameetn/litellm that referenced this pull request Feb 16, 2026
Fixes test_log_langfuse_v2_handles_null_usage_values flaky test failure
by properly cleaning up sys.modules['langfuse'] in tearDown.

Changes:
- Store original langfuse module in setUp before mocking
- Restore original or remove mock in tearDown to prevent state pollution
- Remove invalid print_verbose parameter from log_event_on_langfuse

Root Cause:
The tearDown method was not cleaning up sys.modules['langfuse'] after
each test, causing mock state to leak between tests. This caused
intermittent failures in CI, especially when tests run in parallel or
in different orders.

Impact:
This test has a long history of flakiness with multiple attempted fixes
(BerriAI#20475, BerriAI#17599, BerriAI#17594, BerriAI#17591, BerriAI#17588). The missing sys.modules cleanup
was the underlying issue causing continued failures despite those patches.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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.

3 participants