Skip to content

test(models): re-enable finish_reason unknown-maps-to-OTHER litellm test#6303

Open
anxkhn wants to merge 1 commit into
google:mainfrom
anxkhn:test/reenable-litellm-finish-reason-other
Open

test(models): re-enable finish_reason unknown-maps-to-OTHER litellm test#6303
anxkhn wants to merge 1 commit into
google:mainfrom
anxkhn:test/reenable-litellm-finish-reason-other

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 5, 2026

Copy link
Copy Markdown

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

  • N/A (test-only hardening, no behavior change)

2. Or, if no issue exists, describe the change:

Problem:
test_finish_reason_unknown_maps_to_other in
tests/unittests/models/test_litellm.py is skipped with
@pytest.mark.skip(reason="LiteLLM finish_reason mapping behaviour changed").
That skip was added during the v2.0.0 GA transition, and the reason is now stale.
LiteLlm._map_finish_reason ends with:

return _FINISH_REASON_MAPPING.get(finish_reason_str, types.FinishReason.OTHER)

so any finish_reason that is not in _FINISH_REASON_MAPPING (which only has
length, stop, tool_calls, function_call, content_filter)
deterministically maps to FinishReason.OTHER, independent of LiteLLM's own
normalization. The test deliberately builds a mock choice/response that returns a
raw "totally_unknown_reason" to bypass LiteLLM's normalization and exercise
ADK's own fallback, and asserts the result is OTHER. This skipped test is the
only coverage of the _map_finish_reason -> OTHER fallback branch in the litellm
suite, so the skip has left a live production branch untested.

Solution:
Remove the single @pytest.mark.skip(...) line above the test. No production
change. The test passes as-is against current code, restoring coverage of the
OTHER fallback. I chose deletion over refreshing the mock because the extracted
test body already drives generate_content_async ->
_model_response_to_generate_content_response -> _map_finish_reason -> OTHER
correctly against the current source; no mock detail has drifted.

Diff (1 file, -1 line):

-@pytest.mark.skip(reason="LiteLLM finish_reason mapping behaviour changed")
 @pytest.mark.asyncio
 async def test_finish_reason_unknown_maps_to_other(
     mock_acompletion, lite_llm_instance

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

pytest results (with uv run --no-sync):

  • Re-enabled test (was skipped, now runs):
    pytest tests/unittests/models/test_litellm.py::test_finish_reason_unknown_maps_to_other -q
    -> 1 passed.
  • Neighborhood:
    pytest tests/unittests/models/test_litellm.py -q -k "finish_reason or map_finish"
    -> 10 passed.
  • Full file:
    pytest tests/unittests/models/test_litellm.py -q
    -> 318 passed (previously 317 passed, 1 skipped; the delta is exactly this
    re-enabled test).

I also confirmed the test genuinely guards the branch: temporarily changing the
production fallback from OTHER to STOP made this test fail
(AssertionError: - OTHER + STOP); reverting restored green. So it is an
effective regression test, not a vacuous one.

Manual End-to-End (E2E) Tests:

N/A. This is a test-only change (removal of a stale skip marker); there is no
runtime or user-facing behavior to test manually.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas. (N/A: a
    one-line skip-marker removal; the test already carries an explanatory
    docstring and comments.)
  • I have added tests that prove my fix is effective or that my feature works.
    (Re-enables the sole existing coverage of the _map_finish_reason -> OTHER
    fallback.)
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end. (N/A: test-only change.)
  • Any dependent changes have been merged and published in downstream modules.
    (None.)

Additional context

The mapping table and fallback live in src/google/adk/models/lite_llm.py
(_FINISH_REASON_MAPPING and _map_finish_reason). The test reaches the fallback
through LiteLlm.generate_content_async ->
_model_response_to_generate_content_response, which reads
first_choice.get("finish_reason") and passes it to _map_finish_reason.

test_finish_reason_unknown_maps_to_other was skipped during the v2.0.0 GA
transition with reason "LiteLLM finish_reason mapping behaviour changed".
That reason is now stale: _map_finish_reason ends with
_FINISH_REASON_MAPPING.get(finish_reason_str, types.FinishReason.OTHER), so
any unmapped finish_reason deterministically maps to OTHER regardless of
LiteLLM's own normalization. The test bypasses that normalization with a mock
returning a raw "totally_unknown_reason" and asserts the OTHER fallback.

This skipped test is the only coverage of the _map_finish_reason -> OTHER
fallback branch in the litellm suite, so the skip left a live production
branch untested. Removing the skip restores that coverage with no production
change; the test passes as-is.
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