refactor(e2e): unify RouterInstance into Gateway class, split conftest.py into modular fixtures#16671
refactor(e2e): unify RouterInstance into Gateway class, split conftest.py into modular fixtures#16671
Conversation
Summary of ChangesHello @slin1237, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the end-to-end (e2e) testing infrastructure by centralizing router management within an enhanced Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly refactors and improves the e2e test infrastructure. Unifying RouterInstance into the Gateway class and splitting the large conftest.py into modular fixtures are great steps towards better maintainability and readability. The addition of cloud mode to the Gateway class is also a valuable enhancement. I've provided a couple of suggestions to further improve the code. One addresses repetitive logic in the Gateway class for handling cloud providers, and the other fixes an inconsistency in the return value of the setup_backend fixture, which could cause test failures. Overall, this is a high-quality contribution.
| if cloud_backend == "openai": | ||
| worker_url = "https://api.openai.com" | ||
| api_key = os.environ.get("OPENAI_API_KEY") | ||
| if not api_key: | ||
| raise ValueError("OPENAI_API_KEY environment variable required") | ||
| self._env = os.environ.copy() | ||
| self._env["OPENAI_API_KEY"] = api_key | ||
| elif cloud_backend == "xai": | ||
| worker_url = "https://api.x.ai" | ||
| api_key = os.environ.get("XAI_API_KEY") | ||
| if not api_key: | ||
| raise ValueError("XAI_API_KEY environment variable required") | ||
| self._env = os.environ.copy() | ||
| self._env["XAI_API_KEY"] = api_key | ||
| else: | ||
| raise ValueError(f"Unsupported cloud backend: {cloud_backend}") |
There was a problem hiding this comment.
This block contains repetitive logic for handling different cloud backends. It can be refactored to be more DRY by using a configuration dictionary. This will make it easier to add or modify cloud backends in the future.
provider_configs = {
"openai": {
"worker_url": "https://api.openai.com",
"api_key_env": "OPENAI_API_KEY",
},
"xai": {
"worker_url": "https://api.x.ai",
"api_key_env": "XAI_API_KEY",
},
}
if cloud_backend not in provider_configs:
raise ValueError(f"Unsupported cloud backend: {cloud_backend}")
config = provider_configs[cloud_backend]
worker_url = config["worker_url"]
api_key_env = config["api_key_env"]
api_key = os.environ.get(api_key_env)
if not api_key:
raise ValueError(f"{api_key_env} environment variable required")
self._env = os.environ.copy()
self._env[api_key_env] = api_key…t.py into modular fixtures Add cloud mode support to Gateway class and remove the duplicate RouterInstance dataclass from backends.py. The Gateway class now supports four modes: 1. Regular mode: worker URLs + model path 2. PD mode: prefill/decode workers 3. IGW mode: empty start, add workers via API 4. Cloud mode: OpenAI/xAI runtimes Also clarify the distinction between: - Cloud runtimes: Where models run (openai, xai) - History backends: Gateway plugin for storage (memory, oracle) Backward compatibility maintained for e2e_response_api migration. Split the 1056-line conftest.py into focused modules: - conftest.py (192 lines): Path setup, logging, imports - fixtures/hooks.py (358 lines): Pytest collection hooks, GPU validation - fixtures/pool.py (194 lines): Model pool fixtures - fixtures/setup_backend.py (401 lines): Backend setup fixtures - fixtures/markers.py (57 lines): Marker helper utilities Also marked legacy fixtures (ports.py, router_manager.py) as deprecated for removal during e2e_response_api migration.
Add cloud mode support to Gateway class and remove the duplicate RouterInstance dataclass from backends.py. The Gateway class now supports four modes:
Also clarify the distinction between:
Backward compatibility maintained for e2e_response_api migration.
Split the 1056-line conftest.py into focused modules:
Also marked legacy fixtures (ports.py, router_manager.py) as deprecated for removal during e2e_response_api migration.
Checklist
Review Process
/tag-run-ci-label,/rerun-failed-ci,/tag-and-rerun-ci) or contact authorized users to do so.