-
Notifications
You must be signed in to change notification settings - Fork 720
test: move backend unit tests #3958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d4f9edf to
fde944b
Compare
fde944b to
38f618a
Compare
WalkthroughTests for Prometheus utilities were moved from a centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/src/dynamo/trtllm/tests/conftest.py (1)
16-77: Consider consolidating duplicate conftest code across backends.The
pytest_ignore_collectandmake_cli_args_fixtureimplementations are identical across all three backend conftest files (vllm, sglang, trtllm). Consider extracting these into a shared test utilities module to reduce duplication.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/src/dynamo/sglang/tests/test_sglang_prometheus_utils.py(1 hunks)components/src/dynamo/sglang/tests/test_sglang_unit.py(1 hunks)components/src/dynamo/trtllm/tests/conftest.py(1 hunks)components/src/dynamo/trtllm/tests/test_trtllm_prometheus_utils.py(1 hunks)components/src/dynamo/trtllm/tests/test_trtllm_unit.py(1 hunks)components/src/dynamo/vllm/tests/conftest.py(1 hunks)components/src/dynamo/vllm/tests/test_vllm_prometheus_utils.py(1 hunks)components/src/dynamo/vllm/tests/test_vllm_unit.py(1 hunks)tests/unit/test_prometheus_utils.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/unit/test_prometheus_utils.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nnshah1
PR: ai-dynamo/dynamo#1444
File: tests/fault_tolerance/scenarios.py:57-57
Timestamp: 2025-07-01T15:39:56.789Z
Learning: The fault tolerance tests in tests/fault_tolerance/ are designed to run only in the mounted container environment, so hardcoded absolute paths with `/workspace/` prefix are intentional and should not be changed to relative paths.
📚 Learning: 2025-07-01T15:39:56.789Z
Learnt from: nnshah1
PR: ai-dynamo/dynamo#1444
File: tests/fault_tolerance/scenarios.py:57-57
Timestamp: 2025-07-01T15:39:56.789Z
Learning: The fault tolerance tests in tests/fault_tolerance/ are designed to run only in the mounted container environment, so hardcoded absolute paths with `/workspace/` prefix are intentional and should not be changed to relative paths.
Applied to files:
components/src/dynamo/trtllm/tests/test_trtllm_unit.py
🧬 Code graph analysis (6)
components/src/dynamo/vllm/tests/test_vllm_prometheus_utils.py (2)
components/src/dynamo/common/utils/prometheus.py (1)
get_prometheus_expfmt(100-197)components/src/dynamo/sglang/tests/test_sglang_prometheus_utils.py (4)
TestGetPrometheusExpfmt(17-98)mock_generate_latest(42-43)mock_generate_latest(54-55)test_error_handling(89-98)
components/src/dynamo/sglang/tests/test_sglang_prometheus_utils.py (1)
components/src/dynamo/common/utils/prometheus.py (1)
get_prometheus_expfmt(100-197)
components/src/dynamo/trtllm/tests/test_trtllm_prometheus_utils.py (1)
components/src/dynamo/common/utils/prometheus.py (1)
get_prometheus_expfmt(100-197)
components/src/dynamo/trtllm/tests/test_trtllm_unit.py (1)
components/src/dynamo/trtllm/tests/conftest.py (1)
make_cli_args_fixture(38-77)
components/src/dynamo/vllm/tests/test_vllm_unit.py (1)
components/src/dynamo/vllm/tests/conftest.py (1)
make_cli_args_fixture(38-77)
components/src/dynamo/sglang/tests/test_sglang_unit.py (1)
components/src/dynamo/sglang/tests/conftest.py (1)
make_cli_args_fixture(38-77)
🪛 Ruff (0.14.2)
components/src/dynamo/vllm/tests/test_vllm_prometheus_utils.py
40-40: Unused function argument: reg
(ARG001)
components/src/dynamo/sglang/tests/test_sglang_prometheus_utils.py
42-42: Unused function argument: reg
(ARG001)
54-54: Unused function argument: reg
(ARG001)
components/src/dynamo/trtllm/tests/test_trtllm_prometheus_utils.py
43-43: Unused function argument: reg
(ARG001)
components/src/dynamo/vllm/tests/conftest.py
16-16: Unused function argument: config
(ARG001)
components/src/dynamo/trtllm/tests/conftest.py
16-16: Unused function argument: config
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (8)
components/src/dynamo/sglang/tests/test_sglang_unit.py (1)
13-21: LGTM! Test infrastructure updated correctly for new location.The import path change and REPO_ROOT-based path resolution properly support the test file's new location in the backend subdirectory.
components/src/dynamo/trtllm/tests/conftest.py (1)
16-35: LGTM! The pytest hook signature is correct.The
configparameter is part of the standard pytest hook signature forpytest_ignore_collectand must be present even when unused.components/src/dynamo/trtllm/tests/test_trtllm_unit.py (1)
11-20: LGTM! Test infrastructure correctly updated.The import path and REPO_ROOT-based path resolution are properly configured for the test file's new backend-specific location.
components/src/dynamo/vllm/tests/conftest.py (1)
16-77: LGTM! Conftest utilities correctly implemented.The conditional test collection hook and CLI args fixture factory provide proper test infrastructure for the vLLM backend.
components/src/dynamo/sglang/tests/test_sglang_prometheus_utils.py (1)
66-98: LGTM! Test cases properly validate Prometheus filtering.The tests correctly verify metric filtering, exclusion, label preservation, and error handling for the SGLang backend.
components/src/dynamo/vllm/tests/test_vllm_prometheus_utils.py (1)
20-83: LGTM! Prometheus utility tests correctly implemented.The fixture and test cases properly validate metric filtering, exclusion, and error handling for the vLLM backend.
components/src/dynamo/trtllm/tests/test_trtllm_prometheus_utils.py (1)
20-111: LGTM! Comprehensive Prometheus utility test coverage.The fixture and test cases thoroughly validate metric filtering, prefix addition, default behavior, edge cases, and error handling for the TensorRT-LLM backend.
components/src/dynamo/vllm/tests/test_vllm_unit.py (1)
12-20: LGTM! Test infrastructure properly updated.The import path and REPO_ROOT-based path resolution correctly support the test file's relocation to the vLLM backend subdirectory.
components/src/dynamo/sglang/tests/test_sglang_prometheus_utils.py
Outdated
Show resolved
Hide resolved
|
/ok to test 38f618a |
KrishnanPrash
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments about cleaning up conftest.py
Signed-off-by: pvijayakrish <[email protected]>
…ach backend Signed-off-by: pvijayakrish <[email protected]>
249f57d to
8f83c78
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
/ok to test d5d35a8 |
vllm, sglang and trtllm unit tests have been tested both in ci and locally. |
|
/ok to test f9a24c9 |
Overview:
Move the backend unit tests for vLLM, SGLang, and TRTLLM into their respective backend subdirectories to align with the test directory structure.
Summary by CodeRabbit
Tests
Chores