[UT][fix] Add missing get_ascend_config mock to NPUWorker initialization tests#3729
[UT][fix] Add missing get_ascend_config mock to NPUWorker initialization tests#3729wangxiyuan merged 1 commit intovllm-project:mainfrom
Conversation
…lization tests Signed-off-by: gcanlin <canlinguosdu@gmail.com>
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
There was a problem hiding this comment.
Code Review
This pull request correctly adds a missing mock for get_ascend_config to several NPUWorker initialization tests, which is a good fix. However, the setup for this new mock is duplicated across three test methods. To improve maintainability, this duplicated code should be refactored into a common helper method or moved to the setUp method of the test class.
| mock_ascend_config = MagicMock() | ||
| mock_ascend_config.enable_cpu_binding = False | ||
| mock_get_ascend_config.return_value = mock_ascend_config |
There was a problem hiding this comment.
This mock setup logic is duplicated in test_init_npu_worker_with_trust_remote_code (lines 129-131) and test_init_npu_worker_with_custom_cache_dtype (lines 174-176). To improve maintainability and avoid future inconsistencies, consider extracting this logic into a helper method within the TestNPUWorker class and calling it from each of these tests.
…ion tests (vllm-project#3729) ### What this PR does / why we need it? Enable the unit tests that vllm-project#3612 skipped. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? Unit tests. - vLLM main: vllm-project/vllm@17c540a Signed-off-by: gcanlin <canlinguosdu@gmail.com> Signed-off-by: luolun <luolun1995@cmbchina.com>
…ion tests (vllm-project#3729) ### What this PR does / why we need it? Enable the unit tests that vllm-project#3612 skipped. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? Unit tests. - vLLM main: vllm-project/vllm@17c540a Signed-off-by: gcanlin <canlinguosdu@gmail.com> Signed-off-by: hwhaokun <haokun0405@163.com>
…ion tests (vllm-project#3729) ### What this PR does / why we need it? Enable the unit tests that vllm-project#3612 skipped. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? Unit tests. - vLLM main: vllm-project/vllm@17c540a Signed-off-by: gcanlin <canlinguosdu@gmail.com> Signed-off-by: nsdie <yeyifan@huawei.com>
…ion tests (vllm-project#3729) ### What this PR does / why we need it? Enable the unit tests that vllm-project#3612 skipped. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? Unit tests. - vLLM main: vllm-project/vllm@17c540a Signed-off-by: gcanlin <canlinguosdu@gmail.com>
What this PR does / why we need it?
Enable the unit tests that #3612 skipped.
Does this PR introduce any user-facing change?
How was this patch tested?
Unit tests.