test: enforce pytest markers on components/src tests#7796
Conversation
WalkthroughThis PR adds module-level pytest markers to standardize test categorization across multiple test suites in memory, multimodal, common utilities, frontend, and TRT-LLM modules. Additionally, it enhances the pytest marker reporting utility with comprehensive module stubbing, multiple test directory support, and improved collection behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
components/src/dynamo/frontend/tests/test_sglang_tool_calls.py (1)
23-28: Declare the SGLang test model inpytestmarkas well.This module pulls
Qwen/Qwen3-0.6Bthroughget_tokenizer(MODEL), so it should advertise that dependency withpytest.mark.model(MODEL)too. Otherwisepredownload_modelsstill has to discover it at runtime.Suggested change
+MODEL = "Qwen/Qwen3-0.6B" + pytestmark = [ pytest.mark.unit, pytest.mark.sglang, pytest.mark.gpu_0, pytest.mark.pre_merge, + pytest.mark.model(MODEL), ] - -MODEL = "Qwen/Qwen3-0.6B"As per coding guidelines, use
pytest.mark.model("org/model-name")to declare HF models used; thepredownload_modelsfixture reads these markers to download only what's needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/frontend/tests/test_sglang_tool_calls.py` around lines 23 - 28, The pytest module-level marker list `pytestmark` is missing the HF model marker for the SGLang test; add pytest.mark.model(MODEL) to the pytestmark array so the predownload_models fixture knows to download the tokenizer/model used by get_tokenizer(MODEL). Update the pytestmark declaration (the symbol pytestmark and the constant MODEL referenced by get_tokenizer) to include pytest.mark.model("Qwen/Qwen3-0.6B") or pytest.mark.model(MODEL) so the test advertises its HF dependency.components/src/dynamo/frontend/tests/test_vllm_processor_unit.py (1)
15-20: Add the HF model marker while you're centralizingpytestmark.The tokenizer fixture loads
Qwen/Qwen3-0.6BviaAutoTokenizer.from_pretrained(MODEL), so this module should also declarepytest.mark.model(MODEL). Otherwisepredownload_modelscannot prefetch it for this pre-merge unit path.Suggested change
+MODEL = "Qwen/Qwen3-0.6B" + pytestmark = [ pytest.mark.unit, pytest.mark.vllm, pytest.mark.gpu_0, pytest.mark.pre_merge, + pytest.mark.model(MODEL), ] - -MODEL = "Qwen/Qwen3-0.6B"As per coding guidelines, use
pytest.mark.model("org/model-name")to declare HF models used; thepredownload_modelsfixture reads these markers to download only what's needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/frontend/tests/test_vllm_processor_unit.py` around lines 15 - 20, The pytest module-level markers list pytestmark needs to include the HF model marker so predownload_models can fetch the tokenizer model; update the pytestmark list (the pytestmark variable) to include pytest.mark.model(MODEL) (or pytest.mark.model("Qwen/Qwen3-0.6B") if MODEL is not available) so the tokenizer fixture that calls AutoTokenizer.from_pretrained(MODEL) will be predownloaded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/src/dynamo/common/tests/multimodal/test_embedding_transfer.py`:
- Around line 24-30: The module-level pytest marker list (pytestmark)
incorrectly includes pytest.mark.gpu_1; remove pytest.mark.gpu_1 from the
pytestmark list and instead add pytest.mark.gpu_1 only to the specific test
functions that require GPU (e.g., annotate the NIXL/CUDA tests with
`@pytest.mark.gpu_1` above their def), leaving CPU-only tests with their existing
markers; update the pytestmark variable and the individual test decorators
accordingly so gpu_1 is not applied module-wide.
In `@tests/report_pytest_markers.py`:
- Around line 560-570: The shim _FakeBaseModel defines model_config as a shared
class dict which leaks mutations across subclasses; modify
_FakeBaseModel.__init_subclass__ to assign a fresh dict to cls.model_config (or
replace the class attribute with an immutable default) so each subclass of
_FakeBaseModel gets its own model_config; ensure the test replaces
_pydantic.BaseModel with this fixed _FakeBaseModel so subclass mutations won’t
affect other tests.
In `@tests/utils/test_mock_gpu_alloc.py`:
- Around line 17-26: Remove the pytest.mark.nightly marker from the pytestmark
list in this module so the local-only profiler helper is not accidentally
included in scheduled/nightly runs; keep the existing pytest.mark.skipif that
prevents CI execution and, if needed, replace nightly with a dedicated
registered marker (e.g., pytest.mark.local or pytest.mark.local_only) and add
that marker to pytest.ini instead of reusing pytest.mark.nightly — update the
pytestmark variable to only include skipif and unit (or the new registered
local-only marker) and ensure the marker name referenced in pytest.ini matches
the one you add.
---
Nitpick comments:
In `@components/src/dynamo/frontend/tests/test_sglang_tool_calls.py`:
- Around line 23-28: The pytest module-level marker list `pytestmark` is missing
the HF model marker for the SGLang test; add pytest.mark.model(MODEL) to the
pytestmark array so the predownload_models fixture knows to download the
tokenizer/model used by get_tokenizer(MODEL). Update the pytestmark declaration
(the symbol pytestmark and the constant MODEL referenced by get_tokenizer) to
include pytest.mark.model("Qwen/Qwen3-0.6B") or pytest.mark.model(MODEL) so the
test advertises its HF dependency.
In `@components/src/dynamo/frontend/tests/test_vllm_processor_unit.py`:
- Around line 15-20: The pytest module-level markers list pytestmark needs to
include the HF model marker so predownload_models can fetch the tokenizer model;
update the pytestmark list (the pytestmark variable) to include
pytest.mark.model(MODEL) (or pytest.mark.model("Qwen/Qwen3-0.6B") if MODEL is
not available) so the tokenizer fixture that calls
AutoTokenizer.from_pretrained(MODEL) will be predownloaded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3cd4a7be-fdd1-4890-ae46-21feec73fbd1
📒 Files selected for processing (13)
components/src/dynamo/common/tests/memory/test_multimodal_embedding_cache_manager.pycomponents/src/dynamo/common/tests/multimodal/test_async_encoder_cache.pycomponents/src/dynamo/common/tests/multimodal/test_embedding_transfer.pycomponents/src/dynamo/common/utils/tests/test_inject_reasoning_content.pycomponents/src/dynamo/common/utils/tests/test_label_injecting_collector.pycomponents/src/dynamo/frontend/tests/test_sglang_processor_api.pycomponents/src/dynamo/frontend/tests/test_sglang_processor_unit.pycomponents/src/dynamo/frontend/tests/test_sglang_tool_calls.pycomponents/src/dynamo/frontend/tests/test_vllm_processor_unit.pycomponents/src/dynamo/trtllm/tests/test_dynamic_flags.pycomponents/src/dynamo/trtllm/tests/test_trtllm_additional_metrics.pytests/report_pytest_markers.pytests/utils/test_mock_gpu_alloc.py
7b079a8 to
5ce7750
Compare
d4b4a39 to
b76716e
Compare
b76716e to
6779178
Compare
6779178 to
07369eb
Compare
Extend report_pytest_markers.py to scan both tests/ and components/src/ by default, so component tests are also validated for required markers (Lifecycle, Test Type, Hardware). Add missing pytestmark declarations to 11 component test files that lacked them. Also fix the script's exit logic: return 0 when no markers are missing, instead of propagating pytest collection errors (exit code 2) that are expected in environments without full runtime dependencies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…/src coverage The marker report script could only collect 57/660+ tests due to missing dependency stubs (torch, vllm, sglang, pydantic, etc). Add comprehensive stubs and a pydantic BaseModel shim so the validator covers all test files. Also add missing markers to test_mock_gpu_alloc.py. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- test_kv_transfer_perf_skipped_when_no_transfer: prometheus get_sample_value returns None (not 0.0) for unobserved histograms — accept both - test_structured_output_detection_keys: replace deprecated ast.Str with ast.Constant for Python 3.12 compatibility Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The file-level pytestmark included asyncio which broke the sync TestRingBuffer tests. The async test classes already have asyncio markers at the class/method level. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add stubs for PIL, vllm parsers, mistral_common, typing_extensions (with real TypedDict shim), and vllm EC connector real classes. Propagate pytest exit code for real failures but tolerate collection errors (exitcode 2) since different environments have different deps. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Merge duplicate pytestmark blocks in test_embedding_transfer.py - Remove duplicate vllm.inputs entry in STUB_MODULES - Update test_mock_gpu_alloc.py docs to reflect nightly marker rationale Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Merge duplicate pytestmark blocks in test_embedding_transfer.py - Remove file-level gpu_1 (per-test gpu markers already handle it) - Add auto-stub fallback finder for components/src test deps - Add skip/skipif exclusion, multi-path --tests support Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
_AutoStubFinder uses the deprecated find_module / load_module MetaPathFinder API. Python 3.12 only invokes find_spec on meta-path finders, so the class is never reached at runtime; auto.stubbed is always empty. The two follow-on monkey-patches (pydantic BaseModel and typing_extensions re-exports) are guarded on that empty set, making them dead code as well. Removing the class, its registration, and the dead-guarded shims: - tests/report_pytest_markers.py: -59 lines - drop unused 'from importlib.machinery import ModuleSpec' import The explicit STUB_MODULES list and DependencyStubber pre-stub loop continue to handle the modules they always handled. Pre-commit hook behavior is unchanged from this PR's prior state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f1fe0f2 to
c4bf63e
Compare
…lidator The pre-commit hook `pytest-marker-report` was failing on CI's `ubuntu-latest` runner with `ModuleNotFoundError` for torch / PIL / fsspec / sglang / vllm / msgspec because those packages aren't in the hook's `additional_dependencies` venv. The previous MagicMock-based stubs are fundamentally incompatible with class-level pydantic / typing work — `MagicMock(spec=str)` auto-spec'd through attribute chains breaks ForwardRef compilation, classmethod descriptor binding, and pydantic schema generation. Replace with real-class-returning module stubs: - `_StubModule` resolves unknown attributes to permissive real classes via `_make_stub_class`. Submodule access prefers `sys.modules` so `pkg.sub` returns the submodule when both exist. - `_make_stub_class` returns a class with permissive __init__/__call__/__getattr__/__init_subclass__ and a `__get_pydantic_core_schema__` that returns any_schema, so `class Foo(stub.X)`, `field: stub.X` in BaseModels, and `stub.X(...)` instance attribute chains all work at collection time. - Real `ModuleSpec` on stubs so `importlib.util.find_spec()` (called from `tests/conftest.py` framework auto-skip) doesn't raise ValueError. - Post-stubbing fix-ups: default `BaseModel.model_config = ConfigDict(arbitrary_types_allowed=True)` and set `vllm.__version__` so `from vllm import __version__` works. - Extend `STUB_MODULES` by ~95 leaf entries covering everything dynamo imports transitively that isn't in additional_dependencies. Also add the missing `gpu_0` Hardware marker to two CPU-only unit test files surfaced by the now-comprehensive collection pass: test_graceful_shutdown.py and test_vllm_instrumented_scheduler.py. Verified in a lean pre-commit venv that mirrors CI: 1871 tests checked, 0 missing, 0 collection errors, hook Passed.
generate_locally was refactored into a thin async wrapper around
_generate_locally_impl; the ("json", "json_object", ...) tuple the test
walks for now lives in the impl. Pointing inspect.getsource at the
wrapper produced an empty detection_keys set and tripped the
self.assertTrue(detection_keys, ...) guard.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
lgtm. Thank you!
PR ai-dynamo#7796 (`test: enforce pytest markers on components/src tests`, merged 2026-04-29 18:06 UTC) made `tests/report_pytest_markers.py` strict for tests under `components/src/`. Three test files merged in PRs whose CI ran *before* ai-dynamo#7796 went strict are missing the required Lifecycle / Test Type / Hardware markers and now fail the `pytest-marker-report` pre-commit hook on every PR rebased onto current main: * components/src/dynamo/common/tests/test_video_protocol.py * components/src/dynamo/common/tests/test_video_utils.py * components/src/dynamo/common/tests/test_audio_protocol.py Adds the standard `pytestmark = [pytest.mark.unit, pytest.mark.gpu_0, pytest.mark.pre_merge]` module-level marker block to each — same combination used by `components/src/dynamo/common/tests/configuration/ test_utils.py` and other CPU-only protocol/utility unit tests in the same crate. No test logic changed. This is technically out-of-scope for the Gemma 4 parser PR but is included here so this PR's CI can go green; the underlying breakage otherwise blocks every external-contributor PR currently rebased on main. Happy to split into a separate PR if maintainers prefer. Signed-off-by: David Oy <david.oy@baseten.co>
The module-level _install_stubs() in test_worker.py polluted sys.modules['dynamo.runtime.logging'] (and other dynamo.* leaves) with empty stubs during pytest collection. Because pytest collected test_worker.py before test_vllm_logging.py, the latter's `from dynamo.runtime.logging import VllmColorFormatter` then resolved to the empty stub and failed with "(unknown location)", breaking the strict pytest-marker pre-commit hook added in #7796. Use a normal `from dynamo.common.backend.worker import ...` like every other test in the repo. No stubbing, no cross-test contamination. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Why
We aren't running all unit tests in
components/src/because sometimes we forget to add pytest markers. This PR enforces that to have better test coverage.CI was silently skipping pytest marker enforcement on
components/src/tests because the pre-commit hook's lean venv could not import test modules that pull intorch,vllm,sglang,PIL, etc.What Change
components/src/test trees.ForwardRefresolution, and class inheritance.gpu_0hardware marker to two flagged test files surfaced by the validator.Test Plan
pre-commit clean && pre-commit run pytest-marker-report --all-files— Passed, 1871 tests checked, 0 missing.