fix(planner/tests): scope test_advisory_mode stubs so they don't leak

[dmitry-tokarev-nv]

Summary

test_advisory_mode.py installed stubs for dynamo._core, dynamo.runtime, dynamo.llm, and dynamo.common.forward_pass_metrics into sys.modules at module-import (pytest collection) time. Because it's collected alphabetically before the other planner/profiler tests, those later modules resolved their imports against the stubs — stubs that don't expose ModelType, QueuedRequestMetrics, etc. — which tripped the pytest.skip(...) guards added in the same PR (#8244).

Net effect on the dynamo-planner-test-cpu image with the CI command:

• Before feat(planner): add advisory mode for scaling decisions #8244 (commit 71af9675): 335 passed, 1 skipped
• After feat(planner): add advisory mode for scaling decisions #8244 (current main, e.g. c388483a): 153 passed, 13 skipped

Fix: move the stub injection into a scope="module", autouse=True fixture that

• skips any module already imported (don't shadow reals),
• uses pytest.MonkeyPatch().setitem(sys.modules, ...),
• calls mp.undo() on teardown so sys.modules is clean for sibling test modules.

Unmasking those tests also surfaced two pre-existing issues in TestRefreshWorkerInfoFromConnector (added in #8042, never exercised): the test built NativePlannerBase with environment="kubernetes", which calls config.load_kube_config() and registers Prometheus Enum metrics globally. Patched KubernetesAPI, PlannerPrometheusMetrics, and DYN_PARENT_DGD_K8S_NAME inside _make_planner to sandbox them.

pytest parallelism

CI runners expose 8 cores, but -n auto is slow because per-worker startup/isolation dominates for a 392-test suite. Measured on this PR's planner-test image (392 passed, 1 skipped on every run):

workers	AMD64 (CI)	ARM64 (CI)	local (laptop)
-n 0	163s	226s	73s
-n 2	109s	157s	58s
-n 4	—	—	62s
-n auto (8)	110s	159s	61s

Switched planner's CI override to cpu_parallel_mode: '2' for both pre-merge and post-merge. Two workers match auto on CI and beat it locally; -n 0 is ~50% slower.

Verification

Local run of the exact CI pytest command against the c388483a planner-test-cpu image with this fix + the TestRefreshWorkerInfoFromConnector mocks:

=========== 392 passed, 1 skipped, 74 deselected in 57.89s ===========

vs. same image unpatched: 153 passed, 13 skipped, 11 deselected. The single remaining skip is the legitimate test_virtual_connector.py:48 guard.

Test plan

• pre-merge CI: planner-test on amd64 and arm64 reports 392 passed / 1 skipped.
• No new skips introduced for test_advisory_mode.py or test_load_based_scaling.py.
• Planner tests complete well under the 30-minute timeout with -n 2.

🤖 Generated with Claude Code

[coderabbitai]

Walkthrough

A test module's dependency stubbing was refactored from global sys.modules manipulation at import time to a pytest autouse, module-scoped fixture using MonkeyPatch. The fixture conditionally injects stubs for absent modules and restores state after tests. Top-level stub-related imports were removed, allowing the planner config import to occur without relying on global side effects.

Changes

Cohort / File(s)	Summary
Test dependency stubbing components/src/dynamo/planner/tests/unit/test_advisory_mode.py	Refactored Rust/IO-heavy module stubs from immediate sys.modules.setdefault(...) calls at import time to a pytest autouse, module-scoped fixture that conditionally injects stubs via MonkeyPatch and restores original state afterward. Removed top-level sys, types, and MagicMock imports.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: moving test stubs into a module-scoped fixture to prevent them from leaking to other tests.
Description check	✅ Passed	The pull request description is comprehensive, detailed, and follows the expected structure with clear sections covering summary, root cause, solution, performance analysis, and verification.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

components/src/dynamo/planner/tests/unit/test_advisory_mode.py (1)

23-25: Hoist fixture-local imports to the top of the file.

sys, types, and unittest.mock.MagicMock are standard-library imports with no optional-dependency concern, so they should live at module top per the repo Python guideline. Unlike the lazy PlannerConfig imports inside test methods (which must be deferred until this fixture installs stubs), these have no such constraint.

♻️ Proposed refactor

+import sys
+import types
+from unittest.mock import MagicMock
+
 import pytest
 
 from dynamo.planner.config.defaults import SLAPlannerDefaults
@@
 `@pytest.fixture`(scope="module", autouse=True)
 def _stub_heavy_deps():
     # Stub optional Rust/IO-heavy dynamo modules when absent so planner.config
     # imports used below can resolve. Scoped to this module and torn down after
     # so sibling test modules see the real modules (regression: `#8244` left
     # these stubs in sys.modules and caused later tests to skip).
-    import sys
-    import types
-    from unittest.mock import MagicMock
-
     stubs = {

As per coding guidelines (.ai/python-guidelines.md): "keep all imports at the file top (no imports inside functions/classes) and avoid import-hiding patterns … unless it's explicitly for optional dependencies".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@components/src/dynamo/planner/tests/unit/test_advisory_mode.py` around lines
23 - 25, Move the standard-library imports sys, types and the unittest.mock
import MagicMock out of any fixture- or function-local scope and place them at
the top of the module; locate the fixture or test functions where sys, types, or
MagicMock are currently imported (e.g., the imports referenced as sys, types,
MagicMock) and hoist those three import statements to module-level, leaving
deferred imports like PlannerConfig inside tests untouched.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@components/src/dynamo/planner/tests/unit/test_advisory_mode.py`:
- Around line 23-25: Move the standard-library imports sys, types and the
unittest.mock import MagicMock out of any fixture- or function-local scope and
place them at the top of the module; locate the fixture or test functions where
sys, types, or MagicMock are currently imported (e.g., the imports referenced as
sys, types, MagicMock) and hoist those three import statements to module-level,
leaving deferred imports like PlannerConfig inside tests untouched.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 11d1108d-8a74-474e-aa83-ce17df1e8ffa

📥 Commits

Reviewing files that changed from the base of the PR and between 47cfbd4 and 18ad902.

📒 Files selected for processing (1)

• components/src/dynamo/planner/tests/unit/test_advisory_mode.py