Skip to content

Conversation

@jonpspri
Copy link
Contributor

Summary

This PR completes the test warning cleanup effort started in #1344 and #1342, achieving a fully warning-free test suite when running make test.

Changes

Core Code:

  • Added explicit type hints for session registry attributes (_session_message, _cleanup_task, DB return types)
  • Added done_callback to fire-and-forget asyncio futures to prevent ResourceWarning
  • Implemented proper logging handler cleanup in LoggingService.shutdown()
  • Replaced deprecated Pydantic v1 .dict() with v2 .model_dump()
  • Fixed line continuation formatting in content_moderation plugin

Tests:

  • Fixed AsyncMock usage in session_registry tests with proper SSETransport fixtures
  • Improved mock typing with cast() to prevent type warnings
  • Removed duplicate test code by consolidating mock fixtures
  • Added pytest-integration-mark dependency

Result

All tests now pass without warnings, improving test output clarity and ensuring proper resource cleanup during test execution.

Copilot AI review requested due to automatic review settings October 29, 2025 13:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request improves test reliability and code quality by addressing mock-related warnings and type safety issues. The changes fix unawaited coroutine warnings in tests, improve type annotations, and update deprecated API usage.

  • Replaces AsyncMock with MagicMock or new_callable=AsyncMock where appropriate to prevent unawaited coroutine warnings
  • Adds proper type annotations for optional attributes and return types
  • Updates deprecated Pydantic .dict() to .model_dump() calls
  • Improves test assertions and removes unused imports

Reviewed Changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
uv.lock Adds pytest-integration-mark dependency and reorders prometheus dependencies
pyproject.toml Adds pytest-integration-mark>=0.2.0 to dev dependencies
tests/unit/mcpgateway/test_reverse_proxy.py Fixes AsyncMock usage to prevent unawaited coroutines and improves test structure
tests/unit/mcpgateway/test_display_name_uuid_features.py Adds warning suppression for SQLAlchemy integrity error test
tests/unit/mcpgateway/services/test_logging_service_comprehensive.py Replaces AsyncMock with real async functions and adds proper assertions
tests/unit/mcpgateway/routers/test_teams.py Updates datetime usage to timezone-aware and removes unused variable
tests/unit/mcpgateway/federation/test_discovery.py Removes commented code and fixes monkeypatch usage
tests/unit/mcpgateway/cache/test_session_registry_extended.py Adds shared mock fixture and improves type safety with casts
plugins/content_moderation/content_moderation.py Updates Pydantic API and improves code formatting
mcpgateway/transports/streamablehttp_transport.py Updates deprecated .dict() to .model_dump()
mcpgateway/services/logging_service.py Adds future callback for fire-and-forget pattern and storage handler cleanup
mcpgateway/cache/session_registry.py Adds type annotations for optional attributes and improves type safety

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

msg_json = json.dumps(str(message))

self._session_message: Dict[str, Any] = {"session_id": session_id, "message": msg_json}
self._session_message: Dict[str, Any] | None = {"session_id": session_id, "message": msg_json}
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type annotation on assignment is incorrect. The variable _session_message is being assigned a dict value {\"session_id\": session_id, \"message\": msg_json}, but the annotation indicates it could be None. This should be just self._session_message = {\"session_id\": session_id, \"message\": msg_json} without the type annotation, as the type was already declared at initialization (line 188).

Suggested change
self._session_message: Dict[str, Any] | None = {"session_id": session_id, "message": msg_json}
self._session_message = {"session_id": session_id, "message": msg_json}

Copilot uses AI. Check for mistakes.
Comment on lines +491 to +492
import asyncio
from unittest.mock import AsyncMock
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Import order violates convention. The asyncio import should come before from unittest.mock import AsyncMock since standard library imports should be alphabetically ordered or at least consistently ordered.

Suggested change
import asyncio
from unittest.mock import AsyncMock
from unittest.mock import AsyncMock
import asyncio

Copilot uses AI. Check for mistakes.

async def mock_get_team(team_id, current_user, db):
service = TeamManagementService(db)
_ = TeamManagementService(db)
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating an unused instance of TeamManagementService. This line creates a service instance but doesn't use it, which is wasteful. If the instantiation is not needed for side effects, this line should be removed entirely.

Suggested change
_ = TeamManagementService(db)

Copilot uses AI. Check for mistakes.
self.loop,
)
# Add a done callback to catch any exceptions without blocking
future.add_done_callback(lambda f: f.exception() if not f.cancelled() else None)
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The done callback silently ignores exceptions. While this may be intentional for the fire-and-forget pattern, the lambda should at least log the exception when it occurs for debugging purposes, similar to how exceptions are handled elsewhere in this module.

Copilot uses AI. Check for mistakes.
import mcpgateway.services.logging_service as ls
# Reset global handler
# First-Party
import mcpgateway.services.logging_service as ls
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Module 'mcpgateway.services.logging_service' is imported with both 'import' and 'import from'.

Copilot uses AI. Check for mistakes.
@jonpspri jonpspri force-pushed the chore/test-warning-cleanup branch from c53ceda to 619a5b2 Compare October 30, 2025 10:37
This completes the test warning cleanup effort started in IBM#1344 and IBM#1342,
achieving a fully warning-free test suite when running 'make test'.

Changes include:
- Added explicit type hints for session registry attributes
- Fixed AsyncMock usage in session_registry tests with proper SSETransport fixtures
- Added done_callback to fire-and-forget asyncio futures to prevent warnings
- Implemented proper logging handler cleanup in LoggingService.shutdown()
- Replaced deprecated Pydantic v1 .dict() with v2 .model_dump()
- Fixed line continuation formatting in content_moderation plugin
- Added pytest-integration-mark dependency to pyproject.toml

All tests now pass without warnings, improving test output clarity and
ensuring proper resource cleanup during test execution.

Signed-off-by: Jonathan Springer <jonpspri@gmail.com>
@crivetimihai crivetimihai force-pushed the chore/test-warning-cleanup branch from 619a5b2 to e977579 Compare October 30, 2025 22:55
@crivetimihai crivetimihai merged commit 10f984b into IBM:main Oct 30, 2025
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants