[CI] 【Hackathon 9th Sprint No.34】NO.34 功能模块单测补充 #5057
Conversation
|
Thanks for your contribution! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive unit tests for the fastdeploy/scheduler/splitwise_scheduler.py module as part of Hackathon 9th Sprint No.34, achieving 84% test coverage. The tests use stub implementations for external dependencies (Redis, orjson, logging) to enable isolated testing without requiring actual dependencies.
Key Changes
- Creates a new test file
tests/scheduler/test_splitwise_scheduler.pywith 976 lines of test code - Implements stub modules for Redis, orjson, request classes, and logging to avoid external dependencies
- Covers 8 test classes testing configuration, node management, scheduling logic, and background workers
| raise SystemExit() | ||
|
|
||
| def execute(self): | ||
| return None |
There was a problem hiding this comment.
The execute method returns None explicitly. This is unnecessary and adds clutter. Consider removing the return None statement.
| return None | |
| pass |
| def rpop(self, key: str, count: Optional[int] = None) -> Optional[list[Any]]: | ||
| bucket = self.storage.get(key) | ||
| if not bucket: | ||
| return None | ||
| if count is None: | ||
| return [bucket.pop()] | ||
| count = min(count, len(bucket)) | ||
| values = [bucket.pop() for _ in range(count)] | ||
| return values |
There was a problem hiding this comment.
The rpop method has a potential bug when an empty list exists for a key. If bucket is an empty list (which is falsy), the method returns None, but it should attempt to pop from the empty list (which would raise an IndexError) or return an empty list. This inconsistency could lead to incorrect test behavior.
Consider changing line 117 to:
if bucket is None or not bucket:Or better yet, check the bucket state more explicitly to match Redis behavior.
| logger_mod = types.ModuleType("fastdeploy.utils.scheduler_logger") | ||
|
|
||
| def _log(*_args: Any, **_kwargs: Any) -> None: | ||
| return None |
There was a problem hiding this comment.
The _log function explicitly returns None, which is unnecessary in Python. Functions without a return statement implicitly return None. This adds no value and reduces readability. Consider removing the return None statement.
| return None | |
| pass |
| scheduler_pkg.__path__ = [str(PROJECT_ROOT / "fastdeploy" / "scheduler")] | ||
| sys.modules.setdefault("fastdeploy.scheduler", scheduler_pkg) | ||
|
|
||
| _install_stub_modules._installed = True |
There was a problem hiding this comment.
Using function attributes to store state (_install_stub_modules._installed) is an anti-pattern that makes the code harder to understand and test. Consider using a module-level variable or a class-based approach for managing this state.
| original_sleep = self.module.time.sleep | ||
| self.module.time.sleep = lambda *_args, **_kwargs: (_ for _ in ()).throw(SystemExit()) | ||
| try: | ||
| with self.assertRaises(SystemExit): | ||
| reader.run() | ||
| finally: | ||
| self.module.time.sleep = original_sleep |
There was a problem hiding this comment.
This approach to stopping an infinite loop by monkeypatching time.sleep to throw SystemExit is fragile and obscures test intent. Consider refactoring the code under test to make it more testable (e.g., using a flag or iteration limit), or using a proper threading/timeout approach for testing background loops.
| original_sleep = self.module.time.sleep | |
| self.module.time.sleep = lambda *_args, **_kwargs: (_ for _ in ()).throw(SystemExit()) | |
| try: | |
| with self.assertRaises(SystemExit): | |
| reader.run() | |
| finally: | |
| self.module.time.sleep = original_sleep | |
| # NOTE: Requires ResultReader.run to support max_iterations argument. | |
| reader.run(max_iterations=1) |
| class _Writer: | ||
| def __init__(self) -> None: | ||
| self.items: list[tuple[str, list[bytes]]] = [] | ||
|
|
||
| def put(self, key: str, items: list[bytes]) -> None: | ||
| self.items.append((key, items)) |
There was a problem hiding this comment.
The _Writer class is duplicated in multiple test methods (lines 688-693 and 721-726). This code duplication reduces maintainability. Consider extracting it as a shared test helper class at the module or class level to follow the DRY (Don't Repeat Yourself) principle.
| self.assertEqual(picked_prefill.nodeid, "b") | ||
| self.assertEqual(picked_decode.nodeid, "d") | ||
|
|
||
| with self.assertRaises(Exception): |
There was a problem hiding this comment.
Using a bare Exception in assertRaises is too broad and can mask unexpected errors. Consider using a more specific exception type that the code is expected to raise, such as ValueError or a custom exception type, to make the test more precise and maintainable.
| with self.assertRaises(Exception): | |
| with self.assertRaises(ValueError): |
| @@ -0,0 +1,976 @@ | |||
| """Unit tests for :mod:`fastdeploy.scheduler.splitwise_scheduler`. | |||
There was a problem hiding this comment.
Missing copyright header. All source files in this project should include the Apache License 2.0 copyright header at the top of the file, as seen in other files in the codebase.
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc, tb) -> None: # type: ignore[override] | ||
| return None |
There was a problem hiding this comment.
The __exit__ method should return None or a boolean value. While returning None explicitly works, this is typically omitted in __exit__ implementations. Consider just using pass for consistency with Python conventions.
| return None | |
| pass |
| def test_comparisons(self) -> None: | ||
| low = self.module.NodeInfo("a", "prefill", "h", {"transfer_protocol": ["ipc"]}, load=1) | ||
| high = self.module.NodeInfo("b", "prefill", "h", {"transfer_protocol": ["ipc"]}, load=5) | ||
| self.assertTrue(low < high) |
There was a problem hiding this comment.
assertTrue(a < b) cannot provide an informative message. Using assertLess(a, b) instead will give more informative messages.
| self.assertTrue(low < high) | |
| self.assertLess(low, high) |
…-redis-client Add redis info and ping stubs for splitwise scheduler tests
3 participants
Motivation
NO.34 功能模块 fastdeploy/scheduler/splitwise_scheduler.py 单测补充
Modifications
new dir and add tests/scheduler/test_splitwise_scheduler.py
Usage or Command
scheduler/test_splitwise_scheduler.py:Accuracy Tests
tests/scheduler/test_splitwise_scheduler.py: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.