Repository navigation
fix(test/perf): migrate performance_benchmarks.performance_test.py off LLMInterface (#7041) - #7131
Merged
Merged
Conversation
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
…f LLMInterface (#7041) Completes the #3185 LLMInterface-retirement cleanup for the last remaining file flagged in #6983's sweep. Pure migration — no behavioral changes. Migration applied ----------------- - ``from llm_interface import LLMInterface`` → ``from services.llm_service import LLMService`` - ``class TestLLMInterfacePerformance`` → ``class TestLLMServicePerformance`` (matches the post-#3185 canonical name; class docstring documents the rename) - Setup: dropped ``with patch("src.llm_interface.global_config_manager")`` config-manager mock dance — LLMService initializes from a ``ProviderRegistry`` rather than a global config, so the patch was moot. ``self.llm = LLMService()`` instantiates bare. - Method calls: ``await self.llm.generate_response(prompt)`` → ``await self.llm.chat(messages=[{"role":"user","content":prompt}])`` with ``response.content`` for verification (LLMResponse contract). - Internal mocks: ``patch.object(self.llm, "_make_ollama_request")`` → ``patch.object(self.llm, "chat")`` patching the public surface rather than the internal Ollama-routing path that LLMInterface had. Mock returns ``MagicMock(content=..., error=None, model=...)`` matching LLMResponse shape. - Orchestrator-test mocks: ``patch.object(self.orchestrator, "llm_interface")`` → ``patch.object(self.orchestrator, "llm_service")`` to match the post-#6983 attribute rename, with ``mock_llm.generate_response.return_value = "..."`` swapped to ``mock_llm.chat.return_value = MagicMock(content="...", error=None)``. - ``src.*`` mock paths: dropped the ``src.`` prefix in 6 sites (#6987 territory). The ``src/`` package never existed; ``patch()`` would have failed with ``ModuleNotFoundError`` at first call. Verification ------------ $ grep -nE "from llm_interface\\b|src\\." performance_benchmarks.performance_test.py (empty) $ python3 -m py_compile performance_benchmarks.performance_test.py (clean) Out of scope (filing follow-ups) -------------------------------- The file has additional pre-existing breakage I did **not** fix because it predates #3185: - ``from enhanced_memory_manager import EnhancedMemoryManager`` (line 18) — that module no longer exists; only ``enhanced_memory_manager_async.py`` does. The import has been broken since the async migration. - ``from knowledge_base import KnowledgeBase`` (line 20) — module exists but its import chain references a missing ``models.task_context`` package. - The non-standard filename ``.performance_test.py`` means pytest's default glob doesn't pick up these tests, so the file isn't runnable in CI without explicit ``--collect`` config. These three are tracked in a follow-up issue rather than expanded scope here. The migration that this PR was filed to deliver (the LLM-call parts) is done; the benchmark's actual runtime correctness needs a separate effort (Ollama access + benchmark redesign). Closes #7041 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mrveiss
pushed a commit
that referenced
this pull request
May 7, 2026
Promotes the existing private ``_build_mock_response`` in ``tests/fixtures/mocks.py`` to a public, fully-parameterized ``make_llm_response`` factory. The migration tests I authored across PRs #7101 / #7118 / #7131 each rolled their own LLMResponse stub — three different patterns for the same shape. Combined with 4 pre-existing patterns elsewhere, that's 7 ad-hoc mock styles. Why this matters ---------------- The factory's primary value: returns the **real** ``LLMResponse`` from ``llm_interface_pkg.models`` when importable. If a future field rename or type change happens on that dataclass, every test that uses the factory fails at fixture-construction — instead of silently producing wrong-shape mocks the way ``MagicMock(content=...)`` would. This is the class of drift documented in ``feedback_verify_return_shape_in_method_migration.md`` (#7069 lesson). Changes ------- - ``tests/fixtures/mocks.py``: promoted ``_build_mock_response`` → public ``make_llm_response`` with keyword-only args (``content``, ``error``, ``model``, ``provider``). Underscore alias retained as a back-compat one-liner — no callers break. - ``tests/fixtures/__init__.py``: re-exported. - ``tests/fixtures/test_make_llm_response.py``: 7 contract tests pinning the factory's behavior — most importantly the ``isinstance(LLMResponse)`` check that catches future drift. - Migrated my 2 in-repo callers off ``class _StubResponse``: - api/chat_generate_ai_response_test.py - orchestration/workflow_documentation_summary_test.py Both now import ``from tests.fixtures import make_llm_response`` and wrap it in a same-name pytest fixture so the existing test code doesn't need rewriting. Out of scope (deliberately not in this PR) ------------------------------------------ The 4 pre-existing ad-hoc patterns (each has different historical reasons — keeping the migration surface small): - tests/orchestration/test_subagent_dispatcher_reflection.py:31 (``_make_llm_response`` factory) - tests/api/test_knowledge_grounding.py (5 inline ``MagicMock(content=...)``) These can migrate incrementally; the canonical helper is now available when someone touches those files. PR #7131 (perf benchmarks, still open) deliberately not touched — amending an open PR mid-flight invites conflicts. It can migrate after this lands. Verification ------------ $ python3 -m pytest \ tests/fixtures/test_make_llm_response.py \ api/chat_generate_ai_response_test.py \ orchestration/workflow_documentation_summary_test.py -xvs ============================== 19 passed in 2.58s =============================== - 7 new contract tests (factory behavior pinned) - 5 existing api/chat tests still green via canonical fixture - 7 existing workflow_documentation tests still green via canonical fixture Closes #7134 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mrveiss
added a commit
that referenced
this pull request
May 7, 2026
…7139) Promotes the existing private ``_build_mock_response`` in ``tests/fixtures/mocks.py`` to a public, fully-parameterized ``make_llm_response`` factory. The migration tests I authored across PRs #7101 / #7118 / #7131 each rolled their own LLMResponse stub — three different patterns for the same shape. Combined with 4 pre-existing patterns elsewhere, that's 7 ad-hoc mock styles. Why this matters ---------------- The factory's primary value: returns the **real** ``LLMResponse`` from ``llm_interface_pkg.models`` when importable. If a future field rename or type change happens on that dataclass, every test that uses the factory fails at fixture-construction — instead of silently producing wrong-shape mocks the way ``MagicMock(content=...)`` would. This is the class of drift documented in ``feedback_verify_return_shape_in_method_migration.md`` (#7069 lesson). Changes ------- - ``tests/fixtures/mocks.py``: promoted ``_build_mock_response`` → public ``make_llm_response`` with keyword-only args (``content``, ``error``, ``model``, ``provider``). Underscore alias retained as a back-compat one-liner — no callers break. - ``tests/fixtures/__init__.py``: re-exported. - ``tests/fixtures/test_make_llm_response.py``: 7 contract tests pinning the factory's behavior — most importantly the ``isinstance(LLMResponse)`` check that catches future drift. - Migrated my 2 in-repo callers off ``class _StubResponse``: - api/chat_generate_ai_response_test.py - orchestration/workflow_documentation_summary_test.py Both now import ``from tests.fixtures import make_llm_response`` and wrap it in a same-name pytest fixture so the existing test code doesn't need rewriting. Out of scope (deliberately not in this PR) ------------------------------------------ The 4 pre-existing ad-hoc patterns (each has different historical reasons — keeping the migration surface small): - tests/orchestration/test_subagent_dispatcher_reflection.py:31 (``_make_llm_response`` factory) - tests/api/test_knowledge_grounding.py (5 inline ``MagicMock(content=...)``) These can migrate incrementally; the canonical helper is now available when someone touches those files. PR #7131 (perf benchmarks, still open) deliberately not touched — amending an open PR mid-flight invites conflicts. It can migrate after this lands. Verification ------------ $ python3 -m pytest \ tests/fixtures/test_make_llm_response.py \ api/chat_generate_ai_response_test.py \ orchestration/workflow_documentation_summary_test.py -xvs ============================== 19 passed in 2.58s =============================== - 7 new contract tests (factory behavior pinned) - 5 existing api/chat tests still green via canonical fixture - 7 existing workflow_documentation tests still green via canonical fixture Closes #7134 Co-authored-by: t <t@t> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
mrveiss
pushed a commit
that referenced
this pull request
May 7, 2026
unittest.mock.patch("src.X.Y") would raise ModuleNotFoundError on entry
because autobot-backend has no src/ package — pytest's pythonpath is
'autobot-backend autobot_shared' (per pytest.ini). Every one of these
patches silently fails the test it's supposed to instrument:
* The patched function is never replaced
* The test exercises the real implementation
* Tests pass on stale-mock-luck — i.e., the real return value happens
to satisfy the assertion
* Real regressions on the patched code paths go undetected
Mechanical fix: drop the 'src.' prefix from all 43 patch targets across
15 test files. The remaining path resolves directly via pythonpath:
patch("src.worker_node.get_redis_client")
→ patch("worker_node.get_redis_client") # autobot-backend/worker_node.py
11 unique top-level packages affected, all verified to resolve:
advanced_rag_optimizer, code_embedding_generator, config,
desktop_streaming_manager, enhanced_security_layer, event_manager,
hardware_acceleration, secure_command_executor, user_management,
utils, worker_node
Severity P1: security-critical files affected (enhanced_security_layer,
auth_rbac, secure_command_executor, encryption_service).
Acceptance criteria status:
- ✅ All 48 → 43 sites updated to working module paths (5 already fixed
incidentally by PR #7131 in performance_benchmarks.performance_test.py
during #7041 LLMInterface migration; 43 remain in this PR)
- ✅ Verified target resolution: every top-level package after 'src.'
resolves to autobot-backend/<pkg>(.py|/) on pytest's pythonpath
- ⚠ Pre-commit hook deferred to #7173 (rule prevention is its own scope
and follow-up; this PR focuses on fixing the existing data)
- ⚠ Test verification surfaced expected pre-existing brokenness:
11 of 15 test files have collection-time ImportError on
autobot_shared.redis_management.config → autobot_shared.network_constants
(NetworkConstants moved to constants.network_constants per
`network_constants.py:16`). This is the SAME error on origin/Dev_new_gui
— NOT a regression caused by this fix. Pre-existing brokenness
was already preventing those tests from running on either branch.
Some tests that DO collect now fail on real assertions (e.g.
SecurityPolicy.dangerous_patterns no longer exists) — these are
"real regressions to address" exactly as #6987 AC4 anticipated.
Tracking those follow-ups separately would balloon scope; the
correct path is to fix the import + the broken assertions in
follow-up issues. The fix itself (mock paths now resolve) is
improvement that lets future regressions actually surface.
Closes #6987 (rule + broken-test follow-ups deferred to #7173 + the
existing import errors are pre-existing on Dev_new_gui base, not this
PR's scope)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mrveiss
pushed a commit
that referenced
this pull request
May 7, 2026
unittest.mock.patch("src.X.Y") would raise ModuleNotFoundError on entry
because autobot-backend has no src/ package — pytest's pythonpath is
'autobot-backend autobot_shared' (per pytest.ini). Every one of these
patches silently fails the test it's supposed to instrument:
* The patched function is never replaced
* The test exercises the real implementation
* Tests pass on stale-mock-luck — i.e., the real return value happens
to satisfy the assertion
* Real regressions on the patched code paths go undetected
Mechanical fix: drop the 'src.' prefix from all 43 patch targets across
15 test files. The remaining path resolves directly via pythonpath:
patch("src.worker_node.get_redis_client")
→ patch("worker_node.get_redis_client") # autobot-backend/worker_node.py
11 unique top-level packages affected, all verified to resolve:
advanced_rag_optimizer, code_embedding_generator, config,
desktop_streaming_manager, enhanced_security_layer, event_manager,
hardware_acceleration, secure_command_executor, user_management,
utils, worker_node
Severity P1: security-critical files affected (enhanced_security_layer,
auth_rbac, secure_command_executor, encryption_service).
Acceptance criteria status:
- ✅ All 48 → 43 sites updated to working module paths (5 already fixed
incidentally by PR #7131 in performance_benchmarks.performance_test.py
during #7041 LLMInterface migration; 43 remain in this PR)
- ✅ Verified target resolution: every top-level package after 'src.'
resolves to autobot-backend/<pkg>(.py|/) on pytest's pythonpath
- ⚠ Pre-commit hook deferred to #7173 (rule prevention is its own scope
and follow-up; this PR focuses on fixing the existing data)
- ⚠ Test verification surfaced expected pre-existing brokenness:
11 of 15 test files have collection-time ImportError on
autobot_shared.redis_management.config → autobot_shared.network_constants
(NetworkConstants moved to constants.network_constants per
`network_constants.py:16`). This is the SAME error on origin/Dev_new_gui
— NOT a regression caused by this fix. Pre-existing brokenness
was already preventing those tests from running on either branch.
Some tests that DO collect now fail on real assertions (e.g.
SecurityPolicy.dangerous_patterns no longer exists) — these are
"real regressions to address" exactly as #6987 AC4 anticipated.
Tracking those follow-ups separately would balloon scope; the
correct path is to fix the import + the broken assertions in
follow-up issues. The fix itself (mock paths now resolve) is
improvement that lets future regressions actually surface.
Closes #6987 (rule + broken-test follow-ups deferred to #7173 + the
existing import errors are pre-existing on Dev_new_gui base, not this
PR's scope)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mrveiss
added a commit
that referenced
this pull request
May 7, 2026
…#7176) unittest.mock.patch("src.X.Y") would raise ModuleNotFoundError on entry because autobot-backend has no src/ package — pytest's pythonpath is 'autobot-backend autobot_shared' (per pytest.ini). Every one of these patches silently fails the test it's supposed to instrument: * The patched function is never replaced * The test exercises the real implementation * Tests pass on stale-mock-luck — i.e., the real return value happens to satisfy the assertion * Real regressions on the patched code paths go undetected Mechanical fix: drop the 'src.' prefix from all 43 patch targets across 15 test files. The remaining path resolves directly via pythonpath: patch("src.worker_node.get_redis_client") → patch("worker_node.get_redis_client") # autobot-backend/worker_node.py 11 unique top-level packages affected, all verified to resolve: advanced_rag_optimizer, code_embedding_generator, config, desktop_streaming_manager, enhanced_security_layer, event_manager, hardware_acceleration, secure_command_executor, user_management, utils, worker_node Severity P1: security-critical files affected (enhanced_security_layer, auth_rbac, secure_command_executor, encryption_service). Acceptance criteria status: - ✅ All 48 → 43 sites updated to working module paths (5 already fixed incidentally by PR #7131 in performance_benchmarks.performance_test.py during #7041 LLMInterface migration; 43 remain in this PR) - ✅ Verified target resolution: every top-level package after 'src.' resolves to autobot-backend/<pkg>(.py|/) on pytest's pythonpath - ⚠ Pre-commit hook deferred to #7173 (rule prevention is its own scope and follow-up; this PR focuses on fixing the existing data) - ⚠ Test verification surfaced expected pre-existing brokenness: 11 of 15 test files have collection-time ImportError on autobot_shared.redis_management.config → autobot_shared.network_constants (NetworkConstants moved to constants.network_constants per `network_constants.py:16`). This is the SAME error on origin/Dev_new_gui — NOT a regression caused by this fix. Pre-existing brokenness was already preventing those tests from running on either branch. Some tests that DO collect now fail on real assertions (e.g. SecurityPolicy.dangerous_patterns no longer exists) — these are "real regressions to address" exactly as #6987 AC4 anticipated. Tracking those follow-ups separately would balloon scope; the correct path is to fix the import + the broken assertions in follow-up issues. The fix itself (mock paths now resolve) is improvement that lets future regressions actually surface. Closes #6987 (rule + broken-test follow-ups deferred to #7173 + the existing import errors are pre-existing on Dev_new_gui base, not this PR's scope) Co-authored-by: t <t@t> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes the last #3185-related cleanup flagged by PR #7036's sweep. Pure migration of the LLM call surface — no behavioral changes.
Migration applied
Out of scope (follow-up needed)
The file has 3 pre-existing breakage sites that predate #3185 — I did not fix them in this PR:
Filing as a separate tracker. The migration this PR delivers (the LLM-call parts) is done; the benchmark's runtime correctness needs Ollama access + benchmark redesign which is its own effort.
Verification
```
$ grep -nE 'from llm_interface\b|src\.' performance_benchmarks.performance_test.py
(empty — clean)
$ python3 -m py_compile performance_benchmarks.performance_test.py
(clean)
```
Closes #7041