From 222b22cf80a86b90ff3f463194ceb396f9bcebbb Mon Sep 17 00:00:00 2001 From: Shreyas-Microsoft Date: Mon, 18 May 2026 16:27:48 +0530 Subject: [PATCH] test: resolve PR #242 review comments Address CodeQL (github-code-quality) and Copilot review comments on PR #242: CodeQL: unused imports / variables - test_queue_service_internals.py: drop redundant `tp = _task_param()` in test_output_cleanup_refuses_broad_prefix; drop unused `_no_op_worker` in test_start_service_runs_and_completes. - test_application_base_init.py: drop unused `import inspect` and unused `app` local in test_init_loads_app_config_when_url_set. - test_agent_speaking_capture.py: drop unused `datetime`/`MagicMock` imports. - test_agent_telemetry.py: drop unused `from datetime import UTC, datetime` (only the literal string ` UTC` is used, not the imports). - test_agent_telemetry_records.py: drop unused `import pytest`. - test_config.py (backend SAS blob): drop unused `pytest` import and unused `default_config` symbol from the import list (still referenced via `blob_config_module.default_config`). Copilot review: order-dependent MagicMock leak - test_async_helper_extra.py and test_helper_extra.py: replace `type(MagicMock()).__name__ = ...` mutation in `_wire_credential` with a dedicated dynamically-created stub class per credential type. Mutating `__name__` on the shared MagicMock class globally renamed that class for any later test that introspected `type(...)`, making the suite order-dependent/flaky. The new stubs scope each credential class name to its own type while preserving `hasattr(cred, 'account_key')` semantics for both account-key and AAD code paths. Verification - Backend unit tests: 585 passing (1 unrelated environmental failure against example.azconfig.io), 93.28% coverage. - Processor unit tests: 812 passing, 87.44% coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../sas/storage/blob/test_async_helper_extra.py | 11 +++++++---- .../src/tests/sas/storage/blob/test_config.py | 3 --- .../tests/sas/storage/blob/test_helper_extra.py | 15 ++++++++++----- .../test_agent_speaking_capture.py | 3 +-- .../unit/libs/base/test_application_base_init.py | 3 +-- .../unit/services/test_queue_service_internals.py | 4 ---- .../src/tests/unit/utils/test_agent_telemetry.py | 1 - .../unit/utils/test_agent_telemetry_records.py | 2 -- 8 files changed, 19 insertions(+), 23 deletions(-) diff --git a/src/backend-api/src/tests/sas/storage/blob/test_async_helper_extra.py b/src/backend-api/src/tests/sas/storage/blob/test_async_helper_extra.py index 9d686158..89154141 100644 --- a/src/backend-api/src/tests/sas/storage/blob/test_async_helper_extra.py +++ b/src/backend-api/src/tests/sas/storage/blob/test_async_helper_extra.py @@ -81,13 +81,16 @@ def _wire_credential(svc_cls, *, account_key=None, account_name="myacct", credential_cls_name="DefaultAzureCredential"): svc = _wire(svc_cls) svc.account_name = account_name + # Use a dedicated stub class per credential type so that ``type(cred).__name__`` + # reflects the desired credential class without mutating the shared ``MagicMock`` + # class metadata (which would leak across tests and make order-dependent failures). if credential_cls_name == "AccountKey": - cred = MagicMock() + cred_cls = type("StorageSharedKeyCredential", (), {}) + cred = cred_cls() cred.account_key = account_key - type(cred).__name__ = "StorageSharedKeyCredential" else: - cred = MagicMock(spec=[]) - type(cred).__name__ = credential_cls_name + cred_cls = type(credential_cls_name, (), {}) + cred = cred_cls() svc.credential = cred return svc diff --git a/src/backend-api/src/tests/sas/storage/blob/test_config.py b/src/backend-api/src/tests/sas/storage/blob/test_config.py index 49f25ea5..92b6fbb4 100644 --- a/src/backend-api/src/tests/sas/storage/blob/test_config.py +++ b/src/backend-api/src/tests/sas/storage/blob/test_config.py @@ -1,12 +1,9 @@ """Tests for libs/sas/storage/blob/config.py.""" -import pytest - from libs.sas.storage.blob import config as blob_config_module from libs.sas.storage.blob.config import ( BlobHelperConfig, create_config, - default_config, get_config, set_config, ) diff --git a/src/backend-api/src/tests/sas/storage/blob/test_helper_extra.py b/src/backend-api/src/tests/sas/storage/blob/test_helper_extra.py index e4a672c5..8fc8d4ed 100644 --- a/src/backend-api/src/tests/sas/storage/blob/test_helper_extra.py +++ b/src/backend-api/src/tests/sas/storage/blob/test_helper_extra.py @@ -66,16 +66,21 @@ def test_delete_container_blobs_present_message_no_force(self, blob_service_mock def _wire_credential(blob_service_mock, *, account_key=None, account_name="myacct", credential_cls_name="DefaultAzureCredential"): - """Make the helper's blob_service_client respond like an Azure SDK client.""" + """Make the helper's blob_service_client respond like an Azure SDK client. + + Uses a dedicated stub class per credential type so that ``type(cred).__name__`` + reflects the desired credential class without mutating the shared ``MagicMock`` + class metadata (which would leak across tests). + """ h = _make_helper(blob_service_mock) h.blob_service_client.account_name = account_name if credential_cls_name == "AccountKey": - cred = MagicMock() + cred_cls = type("StorageSharedKeyCredential", (), {}) + cred = cred_cls() cred.account_key = account_key - type(cred).__name__ = "StorageSharedKeyCredential" else: - cred = MagicMock(spec=[]) - type(cred).__name__ = credential_cls_name + cred_cls = type(credential_cls_name, (), {}) + cred = cred_cls() h.blob_service_client.credential = cred return h diff --git a/src/processor/src/tests/unit/libs/agent_framework/test_agent_speaking_capture.py b/src/processor/src/tests/unit/libs/agent_framework/test_agent_speaking_capture.py index 300eecd8..02d60f5e 100644 --- a/src/processor/src/tests/unit/libs/agent_framework/test_agent_speaking_capture.py +++ b/src/processor/src/tests/unit/libs/agent_framework/test_agent_speaking_capture.py @@ -2,9 +2,8 @@ # Licensed under the MIT License. import asyncio -from datetime import datetime from types import SimpleNamespace -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import AsyncMock from libs.agent_framework.agent_speaking_capture import AgentSpeakingCaptureMiddleware diff --git a/src/processor/src/tests/unit/libs/base/test_application_base_init.py b/src/processor/src/tests/unit/libs/base/test_application_base_init.py index 90454bfe..c42630e4 100644 --- a/src/processor/src/tests/unit/libs/base/test_application_base_init.py +++ b/src/processor/src/tests/unit/libs/base/test_application_base_init.py @@ -5,7 +5,6 @@ from __future__ import annotations -import inspect from unittest.mock import MagicMock, patch import pytest @@ -71,7 +70,7 @@ def test_init_loads_app_config_when_url_set(self, patches_chain, tmp_path): env_file = tmp_path / ".env" env_file.write_text("X=1") _App = _build_concrete() - app = _App(env_file_path=str(env_file)) + _App(env_file_path=str(env_file)) patches_chain["ac_helper"].assert_called_once() # The helper instance had its method invoked helper_instance = patches_chain["ac_helper"].return_value diff --git a/src/processor/src/tests/unit/services/test_queue_service_internals.py b/src/processor/src/tests/unit/services/test_queue_service_internals.py index d5ce627e..565a6754 100644 --- a/src/processor/src/tests/unit/services/test_queue_service_internals.py +++ b/src/processor/src/tests/unit/services/test_queue_service_internals.py @@ -714,7 +714,6 @@ def test_output_cleanup_no_account(self): def test_output_cleanup_refuses_broad_prefix(self): s = _service() - tp = _task_param() # Force output_file_folder to broad path matching "" tp = Analysis_TaskParam( process_id="p1", @@ -784,9 +783,6 @@ def test_start_service_runs_and_completes(self): s._ensure_queues_exist = AsyncMock() s._control_watcher_loop = AsyncMock() - async def _no_op_worker(self_, worker_id): - return None - # Patch worker loop to immediate return s._worker_loop = lambda wid: asyncio.sleep(0) # type: ignore[assignment] _run(s.start_service()) diff --git a/src/processor/src/tests/unit/utils/test_agent_telemetry.py b/src/processor/src/tests/unit/utils/test_agent_telemetry.py index e2c97b68..2c5cfc03 100644 --- a/src/processor/src/tests/unit/utils/test_agent_telemetry.py +++ b/src/processor/src/tests/unit/utils/test_agent_telemetry.py @@ -2,7 +2,6 @@ # Licensed under the MIT License. import asyncio -from datetime import UTC, datetime from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch diff --git a/src/processor/src/tests/unit/utils/test_agent_telemetry_records.py b/src/processor/src/tests/unit/utils/test_agent_telemetry_records.py index 5a3200c2..4bdbfd75 100644 --- a/src/processor/src/tests/unit/utils/test_agent_telemetry_records.py +++ b/src/processor/src/tests/unit/utils/test_agent_telemetry_records.py @@ -8,8 +8,6 @@ import asyncio from unittest.mock import AsyncMock, MagicMock, patch -import pytest - import utils.agent_telemetry as at from utils.agent_telemetry import ProcessStatus, TelemetryManager