[CI]【Hackathon 9th Sprint No.35】功能模块 fastdeploy/engine/ common_engine.py 单测补充#5592
[CI]【Hackathon 9th Sprint No.35】功能模块 fastdeploy/engine/ common_engine.py 单测补充#5592kesmeey wants to merge 0 commit intoPaddlePaddle:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the unit tests for fastdeploy/engine/common_engine.py, replacing integration-style tests that start real worker processes with lightweight, CI-safe unit tests using extensive mocking. The new tests are designed to be faster, more isolated, and safer for continuous integration environments.
Key Changes:
- Tests are rewritten from ~850 lines to ~420 lines with improved focus and clarity
- Heavy use of mocking to avoid starting real processes, sockets, or threads
- Three test classes cover initialization/queues, task insertion, and health/environment checks
tests/engine/test_common_engine.py
Outdated
| for p in self._patchers: | ||
| p.stop() | ||
|
|
||
| def _make_request(self, request_id: str, prompt_len: int = 5, role: str | None = None) -> Request: |
There was a problem hiding this comment.
The type hint syntax str | None (PEP 604) requires either Python 3.10+ or the import from __future__ import annotations. Many files in this codebase use from __future__ import annotations for compatibility.
To ensure compatibility with Python 3.9, add this import at the top of the file:
from __future__ import annotationsAlternatively, use the older syntax Optional[str] from the typing module.
tests/engine/test_common_engine.py
Outdated
| self.assertIn("NCCL_ALGO=Ring", value) | ||
|
|
||
| def test_check_health_returns_tuple(self): | ||
| healthy, message = self.engine.check_health(time_interval_threashold=30) |
There was a problem hiding this comment.
The parameter name time_interval_threashold contains a typo. It should be spelled time_interval_threshold (with 'h' after 's').
This appears to match the actual API signature in the EngineService.check_health method, so if this typo exists in the production code, it should be fixed there for better code quality.
| healthy, message = self.engine.check_health(time_interval_threashold=30) | |
| healthy, message = self.engine.check_health(time_interval_threshold=30) |
tests/engine/test_common_engine.py
Outdated
| max_num_seqs=8, | ||
| max_num_batched_tokens=128, | ||
| splitwise_role=splitwise_role, | ||
| scheduler=MagicMock(return_value=MagicMock()), |
There was a problem hiding this comment.
The helper function _make_minimal_cfg uses MagicMock(return_value=MagicMock()) for scheduler_config.scheduler. This creates a mock that returns another mock when called, which may hide bugs if the actual scheduler interface changes.
Consider using a more explicit mock with defined behavior, or at least add a comment explaining why this nested mock structure is needed. This would make the test more maintainable and easier to understand.
| scheduler=MagicMock(return_value=MagicMock()), | |
| # Use a simple mock scheduler that returns None when called. | |
| # If the scheduler interface changes, this will cause test failures, | |
| # making the test more robust than a double-mock. | |
| scheduler=MagicMock(return_value=None), |
tests/engine/test_common_engine.py
Outdated
| eng._finalizer.detach() | ||
| except Exception: | ||
| pass | ||
| """ |
There was a problem hiding this comment.
The PR title format is inconsistent with the convention stated in the coding guidelines. It should follow the format [TAG]Title without Chinese characters or extra details.
For example, instead of:
[CI]【Hackathon 9th Sprint No.35】功能模块 fastdeploy/engine/ common_engine.py 单测补充
Consider:
[CI] Add unit tests for fastdeploy/engine/common_engine.py
This makes the title more concise, professional, and accessible to the international contributor community.
tests/engine/test_common_engine.py
Outdated
| eng._finalizer.detach() | ||
| except Exception: | ||
| pass | ||
| """ |
There was a problem hiding this comment.
The PR description lacks sufficient detail about the changes being made. According to the coding guidelines, the description should explain:
- WHY these modifications are being made
- WHAT problem is being solved
Currently, all sections are marked "No need" which provides no context for reviewers. Please provide:
- A brief explanation of why the tests were rewritten (e.g., "Previous tests were integration-style and slow; new tests use heavy mocking for faster CI execution")
- What specific functionality is now being tested
- Any notable changes in test approach or coverage
tests/engine/test_common_engine.py
Outdated
| with self.assertRaises(EngineError) as ctx: | ||
| self.engine.insert_tasks([bad_req]) | ||
|
|
||
| self.assertIn("required resources is exceed the limit", str(ctx.exception)) |
There was a problem hiding this comment.
The assertion message contains a grammatical error. The string "required resources is exceed the limit" should be "required resources exceed the limit" (no "is" needed with plural subject and no article before "exceed").
If this error message comes from the actual EngineError implementation, it should be fixed there. If this is test-specific, the assertion should match the actual error message format from the production code.
| self.assertIn("required resources is exceed the limit", str(ctx.exception)) | |
| self.assertIn("required resources exceed the limit", str(ctx.exception)) |
tests/engine/test_common_engine.py
Outdated
| cache_instance.get_server_port.return_value = 9100 | ||
| mock_cache_queue.return_value = cache_instance | ||
|
|
||
| engine = EngineService(cfg, start_queue=True, use_async_llm=False) |
There was a problem hiding this comment.
Variable engine is not used.
| engine = EngineService(cfg, start_queue=True, use_async_llm=False) | |
| EngineService(cfg, start_queue=True, use_async_llm=False) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5592 +/- ##
==========================================
Coverage ? 62.32%
==========================================
Files ? 329
Lines ? 41702
Branches ? 6369
==========================================
Hits ? 25991
Misses ? 13733
Partials ? 1978
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@kesmeey 辛苦修改代码风格问题后更新最新代码后重新触发～ |
kesmeey
force-pushed
the
35
branch
2 times, most recently
from
cfc546f to
bcc8d35
Compare
Motivation
No need
Modifications
No need
Usage or Command
No need
Accuracy Tests
No need
Checklist
[FDConfig],[APIServer],[Engine],[Scheduler],[PD Disaggregation],[Executor],[Graph Optimization],[Speculative Decoding],[RL],[Models],[Quantization],[Loader],[OP],[KVCache],[DataProcessor],[BugFix],[Docs],[CI],[Optimization],[Feature],[Benchmark],[Others],[XPU],[HPU],[GCU],[DCU],[Iluvatar],[Metax]]pre-commitbefore commit.releasebranch, make sure the PR has been submitted to thedevelopbranch, then cherry-pick it to thereleasebranch with the[Cherry-Pick]PR tag.