-
Notifications
You must be signed in to change notification settings - Fork 343
test(genai): make backend chosen for tests configurable #1408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR makes the backend selection for GenAI integration tests configurable via the TEST_VERTEXAI environment variable, allowing tests to run against Google AI (default), Vertex AI, or both backends.
Key Changes
- Added
backend_configpytest fixture that parametrizes tests based onTEST_VERTEXAIenvironment variable - Updated integration tests to accept and use
backend_configparameter - Added model name normalization for Vertex AI (strips
models/prefix) - Implemented
create_context_cacheutility function for context caching - Enhanced thinking/reasoning block handling in chat history parsing
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
libs/genai/tests/conftest.py |
Added pytest_generate_tests hook and backend_config fixture for dynamic test parametrization |
libs/genai/tests/integration_tests/test_standard.py |
Dynamically generates test classes for each backend configuration |
libs/genai/tests/integration_tests/test_chat_models.py |
Added backend_config parameter to most integration tests and new context caching/multimodal tests |
libs/genai/tests/integration_tests/test_llms.py |
Added client cleanup in async test |
libs/genai/tests/unit_tests/test_chat_models.py |
Updated tests to verify thinking block handling and model normalization |
libs/genai/tests/unit_tests/test_imports.py |
Added create_context_cache to expected imports |
libs/genai/langchain_google_genai/chat_models.py |
Added thinking block preservation, model name normalization, and empty tool message handling |
libs/genai/langchain_google_genai/utils.py |
New utility module with create_context_cache function |
libs/genai/langchain_google_genai/__init__.py |
Exported create_context_cache |
libs/genai/Makefile |
Updated help text with backend selection instructions |
.github/CONTRIBUTING.md |
Documented backend selection options for integration tests |
Comments suppressed due to low confidence (8)
libs/genai/langchain_google_genai/chat_models.py:536
- This comment appears to contain commented-out code.
# def generate_placeholder_thoughts(value: int) -> str:
# """Placeholder tool."""
# pass
#
# model = ChatGoogleGenerativeAI(
# model="gemini-3-pro-preview"
# ).bind_tools([generate_placeholder_thoughts])
#
# response = model.invoke("Generate a placeholder tool invocation.")
libs/genai/tests/unit_tests/test_chat_models.py:2963
- Variable TestModel is not used.
class TestModel(BaseModel):
libs/genai/langchain_google_genai/chat_models.py:2217
- Overly complex 'del' method.
def __del__(self) -> None:
libs/genai/langchain_google_genai/chat_models.py:63
- Import of 'ModelProfileRegistry' is not used.
from langchain_core.language_models import (
LangSmithParams,
LanguageModelInput,
ModelProfile,
ModelProfileRegistry,
is_openai_data_block,
)
libs/genai/langchain_google_genai/chat_models.py:75
- Import of 'types' is not used.
from langchain_core.messages import content as types
libs/genai/langchain_google_genai/chat_models.py:836
- 'except' clause does nothing but pass and there is no explanatory comment.
except AttributeError:
libs/genai/langchain_google_genai/chat_models.py:1121
- 'except' clause does nothing but pass and there is no explanatory comment.
except AttributeError:
libs/genai/tests/unit_tests/test_chat_models.py:354
- This statement is unreachable.
assert system_instruction is None
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ttl: str | None = None, | ||
| expire_time: str | None = None, | ||
| tools: list[BaseTool | type[BaseModel] | dict | Callable] | None = None, | ||
| tool_choice: _ToolChoiceType | bool | None = None, |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tool_choice parameter is defined in the function signature and documented but is never used in the function body. This is a dead parameter that serves no purpose.
Looking at the implementation (lines 149-193), only model, messages, ttl, expire_time, and tools are used. The tool_choice parameter should either be:
- Removed if it's not needed for context caching, or
- Implemented if it should be passed to the cache creation
Since context caching typically needs to know which tools to use but tool choice is a runtime decision, option 1 (removing it) is likely correct.
| for backend_name, backend_config in _get_backend_configs(): | ||
|
|
||
| class _TestGeminiFlashStandardBase(ChatModelIntegrationTests): | ||
| @property | ||
| def chat_model_class(self) -> type[BaseChatModel]: | ||
| return ChatGoogleGenerativeAI | ||
|
|
||
| @property | ||
| def chat_model_params(self) -> dict: | ||
| # Skip if Vertex AI is requested but GOOGLE_CLOUD_PROJECT is not set | ||
| if backend_config.get("vertexai") and not backend_config.get("project"): | ||
| pytest.skip( | ||
| "Vertex AI tests require GOOGLE_CLOUD_PROJECT env var to be set" | ||
| ) | ||
| return { | ||
| "model": _FLASH_MODEL, | ||
| "rate_limiter": rate_limiter, | ||
| **backend_config, | ||
| } | ||
|
|
||
| @property | ||
| def supports_image_inputs(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_image_urls(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_image_tool_message(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_pdf_inputs(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_audio_inputs(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_json_mode(self) -> bool: | ||
| return True | ||
|
|
||
| @pytest.mark.xfail( | ||
| not _has_multimodal_secrets(), | ||
| reason=( | ||
| "Multimodal tests require integration secrets (user agent to fetch " | ||
| "external resources)" | ||
| ), | ||
| run=False, | ||
| ) | ||
| def test_audio_inputs(self, model: BaseChatModel) -> None: | ||
| """Skip audio tests in PR context - requires external resource fetching.""" | ||
| super().test_audio_inputs(model) | ||
|
|
||
| @pytest.mark.xfail( | ||
| not _has_multimodal_secrets(), | ||
| reason=( | ||
| "Multimodal tests require integration secrets (user agent to fetch " | ||
| "external resources)" | ||
| ), | ||
| run=False, | ||
| ) | ||
| def test_pdf_inputs(self, model: BaseChatModel) -> None: | ||
| """Skip PDF tests in PR context - requires external resource fetching.""" | ||
| super().test_pdf_inputs(model) | ||
|
|
||
| class _TestGeminiProStandardBase(ChatModelIntegrationTests): | ||
| @property | ||
| def chat_model_class(self) -> type[BaseChatModel]: | ||
| return ChatGoogleGenerativeAI | ||
|
|
||
| @property | ||
| def chat_model_params(self) -> dict: | ||
| # Skip if Vertex AI is requested but GOOGLE_CLOUD_PROJECT is not set | ||
| if backend_config.get("vertexai") and not backend_config.get("project"): | ||
| pytest.skip( | ||
| "Vertex AI tests require GOOGLE_CLOUD_PROJECT env var to be set" | ||
| ) | ||
| return { | ||
| "model": _PRO_MODEL, | ||
| "rate_limiter": rate_limiter, | ||
| **backend_config, | ||
| } | ||
|
|
||
| @property | ||
| def supports_image_inputs(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_image_urls(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_pdf_inputs(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_audio_inputs(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_json_mode(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_image_tool_message(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_pdf_tool_message(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supported_usage_metadata_details( | ||
| self, | ||
| ) -> dict[ | ||
| Literal["invoke", "stream"], | ||
| list[ | ||
| Literal[ | ||
| "audio_input", | ||
| "audio_output", | ||
| "reasoning_output", | ||
| "cache_read_input", | ||
| "cache_creation_input", | ||
| ] | ||
| ], | ||
| ]: | ||
| return {"invoke": [], "stream": []} | ||
|
|
||
| # Create backend-specific test classes | ||
| flash_class_name = f"TestGeminiFlashStandard{backend_name}" | ||
| pro_class_name = f"TestGeminiProStandard{backend_name}" | ||
|
|
||
| # Add classes to module globals so pytest can discover them | ||
| globals()[flash_class_name] = type( | ||
| flash_class_name, (_TestGeminiFlashStandardBase,), {} | ||
| ) | ||
| def test_pdf_inputs(self, model: BaseChatModel) -> None: | ||
| """Skip PDF tests in PR context - requires external resource fetching.""" | ||
| super().test_pdf_inputs(model) | ||
|
|
||
|
|
||
| class TestGeminiProStandard(ChatModelIntegrationTests): | ||
| @property | ||
| def chat_model_class(self) -> type[BaseChatModel]: | ||
| return ChatGoogleGenerativeAI | ||
|
|
||
| @property | ||
| def chat_model_params(self) -> dict: | ||
| return { | ||
| "model": _PRO_MODEL, | ||
| "rate_limiter": rate_limiter, | ||
| } | ||
|
|
||
| @property | ||
| def supports_image_inputs(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_image_urls(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_pdf_inputs(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_audio_inputs(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_json_mode(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_image_tool_message(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supports_pdf_tool_message(self) -> bool: | ||
| return True | ||
|
|
||
| @property | ||
| def supported_usage_metadata_details( | ||
| self, | ||
| ) -> dict[ | ||
| Literal["invoke", "stream"], | ||
| list[ | ||
| Literal[ | ||
| "audio_input", | ||
| "audio_output", | ||
| "reasoning_output", | ||
| "cache_read_input", | ||
| "cache_creation_input", | ||
| ] | ||
| ], | ||
| ]: | ||
| return {"invoke": [], "stream": []} | ||
| globals()[pro_class_name] = type(pro_class_name, (_TestGeminiProStandardBase,), {}) |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop-level class definitions create a critical scoping issue. Both _TestGeminiFlashStandardBase and _TestGeminiProStandardBase are redefined in each iteration of the loop, causing them to be overwritten. This means all dynamically created test classes will reference the same base class instances from the final loop iteration, not separate instances for each backend.
This will likely cause all test classes to share the same backend_config from the last iteration, breaking the intended multi-backend testing functionality.
Suggested fix: Define the base classes outside the loop and use closures or factory functions to create backend-specific instances:
def _make_flash_test_class(backend_config: dict) -> type:
class TestBase(ChatModelIntegrationTests):
@property
def chat_model_params(self) -> dict:
if backend_config.get("vertexai") and not backend_config.get("project"):
pytest.skip("Vertex AI tests require GOOGLE_CLOUD_PROJECT env var to be set")
return {
"model": _FLASH_MODEL,
"rate_limiter": rate_limiter,
**backend_config,
}
# ... rest of the properties
return TestBase
for backend_name, backend_config in _get_backend_configs():
globals()[f"TestGeminiFlashStandard{backend_name}"] = _make_flash_test_class(backend_config)
globals()[f"TestGeminiProStandard{backend_name}"] = _make_pro_test_class(backend_config)|
|
||
|
|
||
| @pytest.mark.flaky(retries=3, delay=1) | ||
| def test_streaming_function_call_arguments() -> None: |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two test functions (test_streaming_function_call_arguments and test_multimodal_function_response) are missing the backend_config fixture parameter that all other integration tests in this file use. This is inconsistent with the PR's goal of making backend selection configurable.
The tests explicitly hardcode Vertex AI configuration (lines 2607-2612 and 2706-2711), which means they won't respect the TEST_VERTEXAI environment variable and won't be parametrized like other tests.
Suggested fix: Either:
- Add
backend_configparameter and skip if not using Vertex AI, or - Document why these tests must be Vertex AI-only and exclude them from the parametrization (e.g., using a different test marker)
|
|
||
|
|
||
| @pytest.mark.flaky(retries=3, delay=1) | ||
| def test_multimodal_function_response() -> None: |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test function (test_multimodal_function_response) is missing the backend_config fixture parameter that all other integration tests in this file use. This is inconsistent with the PR's goal of making backend selection configurable.
The test explicitly hardcodes Vertex AI configuration (lines 2706-2711), which means it won't respect the TEST_VERTEXAI environment variable and won't be parametrized like other tests.
Suggested fix: Either:
- Add
backend_configparameter and skip if not using Vertex AI, or - Document why this test must be Vertex AI-only and exclude it from the parametrization (e.g., using a different test marker)
No description provided.