test(models): exercise real connect() path in test_connect#6394
Open
anxkhn wants to merge 1 commit into
Open
Conversation
test_connect patched google_llm.Gemini.connect (the method under test) at the class level, so the real connect() body never ran and the only assertion checked the test's own mock return value, a tautology that could not fail on any regression. Patch the _live_api_client boundary instead and let the real connect() run, matching how the neighboring connect tests (test_connect_with_custom_headers, test_connect_without_custom_headers, test_connect_forwards_thinking_config) already work. Assert the live client is called with the request model and live_connect_config, and that connect() yields a GeminiLlmConnection wrapping the session with the expected api_backend and model_version. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to Issue or Description of Change
2. Or, if no issue exists, describe the change:
Problem:
test_connectpatched the method it is meant to exercise. It didmock.patch("google.adk.models.google_llm.Gemini.connect", ...)at the classlevel, so the real
connect()body never ran and the only assertion wasconnection is mock_connection, i.e. it checked that the test's own mockreturned the mock the test itself created. That is a tautology: it passes no
matter how
connect()actually behaves, so the live-connect path(
live_connect_configassembly, the_live_api_client.aio.live.connect(...)call, and
GeminiLlmConnectionconstruction) has effectively no coverage fromthe test named after it.
Solution:
Patch the
_live_api_clientboundary instead and let the realconnect()bodyrun, matching how the neighboring connect tests already work
(
test_connect_with_custom_headers,test_connect_without_custom_headers,test_connect_forwards_thinking_config). The test now asserts the live clientis called once with the request
modelandlive_connect_config, and thatconnect()yields aGeminiLlmConnectionwrapping the live session with theexpected
_api_backendand_model_version. This is a test-only change; noproduction code is modified.
Testing Plan
Unit Tests:
To confirm the rewritten test is no longer tautological, I temporarily
introduced three regressions in
connect()and each was caught: yielding theraw session instead of a
GeminiLlmConnection, passing a freshLiveConnectConfig()instead of the request's, and setting a wrongmodel_version. The old version of the test passed all three.Manual End-to-End (E2E) Tests:
Not applicable. This is a unit-test-only change to an existing test; it exercises
Gemini.connect()with the live API client mocked at its boundary and adds nonew user-facing behaviour.
Checklist