Repository navigation
refactor(e2e): clean up test infrastructure code quality and efficiency - #686
Conversation
- fixtures/hooks.py: Remove duplicate _get_own_class_marker() implementation (26 lines) and delegate to markers.resolve_class_marker() which already implements the same MRO-walking logic - infra/constants.py: Remove redundant `import os` inside get_runtime(); os is already imported at module level - infra/gateway.py: Move `import time` to module level and replace time.time() with time.perf_counter() in add_worker() for consistency with the rest of the codebase - infra/gpu_monitor.py: Sort samples once in _compute_stats() instead of re-sorting 7 times per call through _percentile(); use sorted list for min/max instead of separate min()/max() calls - infra/model_specs.py: Cache E2E_MODEL_TP_OVERRIDES JSON parsing at module load via _parse_tp_overrides() instead of re-parsing on every get_model_spec() call - infra/worker.py: Create one gRPC channel before the health check retry loop and reuse it across iterations instead of creating and closing a new channel per attempt; clean up channel in a finally block Signed-off-by: SimonOlin <simon@simolin.dev> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Summary of ChangesHello, 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 focuses on enhancing the code quality and efficiency of the E2E test infrastructure. It introduces six targeted improvements that streamline operations, reduce redundant code, and improve performance, resulting in a net reduction of 15 lines of code. The changes address issues related to code reuse, consistency in utility usage, and overall operational efficiency within the testing framework. Highlights
Changelog
Activity
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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR refactors e2e test infrastructure across six files, improving marker resolution logic, removing redundant code, optimizing timing measurements with monotonic clocks, refactoring percentile computation for pre-sorted data, centralizing environment variable parsing with caching, and optimizing gRPC channel lifecycle management. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a series of well-targeted refactorings across the E2E test infrastructure, enhancing code quality, consistency, and efficiency. The changes include removing duplicated marker resolution logic, eliminating redundant imports and JSON parsing, optimizing statistical calculations by avoiding repeated sorting, and improving gRPC connection handling by reusing channels. All changes are implemented correctly and contribute to a cleaner and more performant test suite. I have reviewed the changes and found no issues.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 887959124e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| def _get_marker(item: pytest.Item, name: str): | ||
| """Get the most specific marker, preferring child class over parent.""" | ||
| return _get_own_class_marker(item, name) or item.get_closest_marker(name) | ||
| return resolve_class_marker(item, name) |
There was a problem hiding this comment.
Switching _get_marker() to resolve_class_marker() changes precedence when a test method has an engine/vendor/gpu marker but its class only inherits a parent class marker: resolve_class_marker() returns the inherited class marker before consulting item.get_closest_marker(), so method-level overrides are ignored. Under E2E_ENGINE/E2E_VENDOR/E2E_GPU_TIER filtering this can incorrectly include/exclude tests in subclass hierarchies where only the parent class carries the broad marker.
Useful? React with 👍 / 👎.
| return None | ||
|
|
||
|
|
||
| _TP_OVERRIDES = _parse_tp_overrides() |
There was a problem hiding this comment.
Caching E2E_MODEL_TP_OVERRIDES at import time freezes the override map for the lifetime of the process, so any later environment updates are silently ignored by get_model_spec(). This is a behavioral regression from the previous per-call lookup and breaks workflows/tests that set or mutate this env var after module import (for example via monkeypatch.setenv) to control GPU parallelism per run.
Useful? React with 👍 / 👎.
…ssing - Extract _make_user_message() helper in test_realtime_ws.py to replace 7 copy-pasted conversation.item.create dict structures, reducing ~70 lines of boilerplate - Remove "HF_HOME" from the env-var pass-through tuple in benchmarks/conftest.py — it was already explicitly set on line 61 when mounting the HF cache directory, causing the variable to be passed twice to Docker - Simplify redundant hasattr+getattr guard in _cleanup_procs() to plain getattr with default, since getattr(p, "proc", p) already handles the missing attribute case Refs: #686 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…ssing - Extract _make_user_message() helper in test_realtime_ws.py to replace 7 copy-pasted conversation.item.create dict structures, reducing ~70 lines of boilerplate - Remove "HF_HOME" from the env-var pass-through tuple in benchmarks/conftest.py — it was already explicitly set on line 61 when mounting the HF cache directory, causing the variable to be passed twice to Docker - Simplify redundant hasattr+getattr guard in _cleanup_procs() to plain getattr with default, since getattr(p, "proc", p) already handles the missing attribute case Refs: #686 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
1 participant
Summary
Six targeted code quality and efficiency improvements across the E2E test infrastructure, reducing net 15 lines while improving consistency and eliminating redundant work.
What changed
e2e_test/fixtures/hooks.py: Removed duplicate_get_own_class_marker()(26 lines) that reimplemented the same MRO-walking logic already inmarkers.resolve_class_marker(). The_get_marker()helper now delegates directly.e2e_test/infra/constants.py: Removed redundantimport osinsideget_runtime()—osis already imported at module level.e2e_test/infra/gateway.py: Movedimport timefrom insideadd_worker()to module level; replacedtime.time()withtime.perf_counter()for consistency with the rest of the codebase.e2e_test/infra/gpu_monitor.py: Changed_percentile()to accept a pre-sorted list and sort once in_compute_stats()instead of re-sorting 7 times per call. Also usesorted_list[0]/sorted_list[-1]for min/max instead of separatemin()/max()traversals.e2e_test/infra/model_specs.py: CachedE2E_MODEL_TP_OVERRIDESJSON parsing at module load via_parse_tp_overrides()instead of re-parsing the env var on everyget_model_spec()call.e2e_test/infra/worker.py: Create one gRPC channel before the health check retry loop and reuse it across iterations (withfinallycleanup) instead of creating+closing a new channel per attempt.Why
These are low-risk housekeeping fixes identified via systematic code review:
time.time()vstime.perf_counter(), inline vs module-level importsTest plan
pytest --collect-onlystill collects tests correctly (hooks.py marker resolution)Summary by CodeRabbit