Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an integration testing framework for Microsoft Agents applications. It provides infrastructure for setting up and testing agent applications with support for multiple hosting environments (initially aiohttp), sample applications, and client interactions.
Key changes:
- Integration testing framework with decorator-based test setup supporting service URLs, sample applications, or raw aiohttp apps
- Client libraries for sending messages to agents (
AgentClient) and receiving responses (ResponseClient) - Environment abstraction for hosting agents with an initial aiohttp implementation
- Sample application structure with a quickstart example demonstrating basic agent functionality
Reviewed Changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 28 comments.
Show a summary per file
| File | Description |
|---|---|
| dev/integration/src/tests/test_quickstart.py | Adds integration test for quickstart sample functionality |
| dev/integration/src/samples/quickstart_sample.py | Implements a basic agent sample with greeting and echo handlers |
| dev/integration/src/samples/init.py | Exports QuickstartSample class |
| dev/integration/src/environments/aiohttp_environment.py | Provides aiohttp-based hosting environment for agents |
| dev/integration/src/environments/init.py | Exports AiohttpEnvironment class |
| dev/integration/src/core/sample.py | Defines abstract base class for sample applications |
| dev/integration/src/core/integration_fixtures.py | Provides pytest fixtures for integration testing |
| dev/integration/src/core/integration.py | Implements decorator for integration test setup with multiple strategies |
| dev/integration/src/core/environment.py | Defines abstract base class for hosting environments |
| dev/integration/src/core/client/response_client.py | Implements client for receiving and managing agent responses |
| dev/integration/src/core/client/bot_response.py | Provides alternative bot response server implementation |
| dev/integration/src/core/client/auto_client.py | Contains commented-out auto-client functionality |
| dev/integration/src/core/client/agent_client.py | Implements client for sending activities to agents |
| dev/integration/src/core/client/init.py | Exports client classes |
| dev/integration/src/core/application_runner.py | Provides base class for running applications in separate threads |
| dev/integration/src/core/init.py | Exports core integration testing components |
| dev/integration/requirements.txt | Adds aioresponses dependency |
| dev/integration/env.TEMPLATE | Template for environment configuration |
Comments suppressed due to low confidence (1)
dev/integration/src/environments/aiohttp_environment.py:2
- Import of 'E' is not used.
from tkinter import E
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,74 @@ | |||
|
|
|||
| from tkinter import E | |||
There was a problem hiding this comment.
Unused import of 'E' from 'tkinter'. This appears to be a leftover import that should be removed as tkinter is not used in this file.
| from tkinter import E |
| self._prev_stdout = None | ||
| self._service_endpoint = service_endpoint | ||
| self._activities_list = [] | ||
| self._activities_list_lock = [] |
There was a problem hiding this comment.
The lock should be a Lock() instance, not an empty list. This will cause the with statement on line 53 to fail since lists don't support the context manager protocol. Change to self._activities_list_lock = Lock().
| self._activities_list_lock = [] | |
| self._activities_list_lock = Lock() |
| async def test_quickstart_functionality(self, agent_client, response_client): | ||
| await agent_client.send("hi") | ||
| await asyncio.sleep(2) | ||
| response = (await response_client.pop())[0] |
There was a problem hiding this comment.
The ResponseClient class does not have a pop() method defined. This will cause an AttributeError at runtime.
| response = (await response_client.pop())[0] | |
| response = (await response_client.get())[0] |
| target_cls.setup_method = setup_method | ||
| target_cls.teardown_method = teardown_method | ||
|
|
||
| target_cls = Integration._with_response_server(target_cls, host_response) |
There was a problem hiding this comment.
Reference to Integration class should be _Integration as that's the actual class name defined on line 16.
| def setup_method(self): | ||
| self._environment = environment_cls(sample_cls.get_config()) | ||
| await self._environment.__aenter__() | ||
|
|
||
| self._sample = sample_cls(self._environment) | ||
| await self._sample.__aenter__() | ||
|
|
||
| def teardown_method(self): |
There was a problem hiding this comment.
Missing async keyword for setup_method. This function uses await so it must be defined as async def setup_method(self):.
| def setup_method(self): | |
| self._environment = environment_cls(sample_cls.get_config()) | |
| await self._environment.__aenter__() | |
| self._sample = sample_cls(self._environment) | |
| await self._sample.__aenter__() | |
| def teardown_method(self): | |
| async def setup_method(self): | |
| self._environment = environment_cls(sample_cls.get_config()) | |
| await self._environment.__aenter__() | |
| self._sample = sample_cls(self._environment) | |
| await self._sample.__aenter__() | |
| async def teardown_method(self): |
| import os | ||
| import json | ||
| import asyncio | ||
| from typing import Any, Optional, cast |
There was a problem hiding this comment.
Import of 'Any' is not used.
| from typing import Any, Optional, cast | |
| from typing import Optional, cast |
| import asyncio | ||
| import json | ||
| import sys | ||
| import threading |
There was a problem hiding this comment.
Import of 'threading' is not used.
| import threading |
|
|
||
| from aiohttp import web, ClientSession | ||
| from aiohttp.web import Request, Response | ||
| import aiohttp_security |
There was a problem hiding this comment.
Import of 'aiohttp_security' is not used.
| import aiohttp_security |
| @@ -0,0 +1,151 @@ | |||
| from typing import Optional, TypeVar, Union, Callable, Any | |||
There was a problem hiding this comment.
Import of 'Union' is not used.
| from typing import Optional, TypeVar, Union, Callable, Any | |
| from typing import Optional, TypeVar, Callable, Any |
| @@ -0,0 +1,38 @@ | |||
| from typing import TypeVar, Any, AsyncGenerator, Callable | |||
There was a problem hiding this comment.
Import of 'Any' is not used.
Import of 'TypeVar' is not used.
| from typing import TypeVar, Any, AsyncGenerator, Callable | |
| from typing import AsyncGenerator, Callable |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 24 out of 29 changed files in this pull request and generated 17 comments.
Comments suppressed due to low confidence (5)
dev/integration/src/core/integration.py:72
- Variable setup_method is not used.
async def setup_method(self):
dev/integration/src/core/integration.py:79
- Variable teardown_method is not used.
async def teardown_method(self):
dev/integration/src/environments/aiohttp_environment.py:2
- Import of 'E' is not used.
from tkinter import E
dev/integration/src/tests/core/client/test_agent_client.py:10
- Keyword argument 'base_url' is not a supported parameter name of AgentClient.init.
return AgentClient(base_url="")
dev/integration/src/tests/core/client/test_agent_client.py:16
- Keyword argument 'base_url' is not a supported parameter name of AgentClient.init.
client = AgentClient(base_url="https://example.com")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,19 @@ | |||
| from re import A | |||
There was a problem hiding this comment.
Unused import 'A' from the 're' module. This import should be removed as it serves no purpose.
| from re import A |
| async def setup_method(self): | ||
| self._environment = environment_cls(sample_cls.get_config()) | ||
| await self._environment.__aenter__() | ||
|
|
||
| self._sample = sample_cls(self._environment) | ||
| await self._sample.__aenter__() |
There was a problem hiding this comment.
Missing assignment of setup_method and teardown_method to target_cls. The decorated methods should be assigned like target_cls.setup_method = setup_method and target_cls.teardown_method = teardown_method before calling _with_response_client, similar to the from_service_url method.
| def decorator(target_cls: T) -> T: | ||
|
|
||
| async def setup_method(self): | ||
| self._environment = environment_cls(sample_cls.get_config()) |
There was a problem hiding this comment.
sample_cls.get_config() is an async method but is being called without await. This should be await sample_cls.get_config() to properly retrieve the configuration.
| self._environment = environment_cls(sample_cls.get_config()) | |
| self._environment = environment_cls(await sample_cls.get_config()) |
| authorization=self.authorization, | ||
| **self.agents_sdk_config |
There was a problem hiding this comment.
Reference to undefined attribute self.agents_sdk_config. This attribute is not defined in the init_env method or anywhere in the class. Either define this attribute or remove the unpacking.
| authorization=self.authorization, | |
| **self.agents_sdk_config | |
| authorization=self.authorization |
| async def __aenter__(self) -> None: | ||
|
|
||
| if self._thread: | ||
| raise RuntimeError("Server is already running") | ||
|
|
||
| self._thread = Thread(target=self._start_server, daemon=True) | ||
| self._thread.start() | ||
|
|
There was a problem hiding this comment.
The __aenter__ method should return self (or the ApplicationRunner instance) to support the context manager protocol properly. Change return type to 'ApplicationRunner' and add return self at the end.
| async def __aenter__(self) -> None: | |
| if self._thread: | |
| raise RuntimeError("Server is already running") | |
| self._thread = Thread(target=self._start_server, daemon=True) | |
| self._thread.start() | |
| async def __aenter__(self) -> "ApplicationRunner": | |
| if self._thread: | |
| raise RuntimeError("Server is already running") | |
| self._thread = Thread(target=self._start_server, daemon=True) | |
| self._thread.start() | |
| return self |
|
|
||
| import aiohttp.web | ||
|
|
||
| from .application_runner import ApplicationRunner |
There was a problem hiding this comment.
Import of 'ApplicationRunner' is not used.
| from .application_runner import ApplicationRunner |
|
|
||
| from .application_runner import ApplicationRunner | ||
| from .environment import Environment | ||
| from .client import AgentClient, ResponseClient |
There was a problem hiding this comment.
Import of 'AgentClient' is not used.
| from .client import AgentClient, ResponseClient | |
| from .client import ResponseClient |
| Sample | ||
| ) | ||
|
|
||
| from ._common import SimpleRunner, OtherSimpleRunner |
There was a problem hiding this comment.
Import of 'OtherSimpleRunner' is not used.
| from ._common import SimpleRunner, OtherSimpleRunner | |
| from ._common import SimpleRunner |
| @@ -0,0 +1,7 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
| from src.core import integration | ||
|
|
There was a problem hiding this comment.
Import of 'integration' is not used.
| from src.core import integration |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 24 out of 29 changed files in this pull request and generated 17 comments.
Comments suppressed due to low confidence (1)
dev/integration/src/environments/aiohttp_environment.py:2
- Import of 'E' is not used.
from tkinter import E
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,74 @@ | |||
|
|
|||
| from tkinter import E | |||
There was a problem hiding this comment.
Unused import E from the tkinter module should be removed. This import is not used anywhere in the code and tkinter is unrelated to the aiohttp environment implementation.
| from tkinter import E |
| app = {} | ||
| runner = SimpleRunner(app) | ||
| async with runner as r: | ||
| sleep(0.1) |
There was a problem hiding this comment.
Using synchronous sleep() in an async test function blocks the event loop. Replace with await asyncio.sleep(0.1) to properly yield control in the async context.
| app = {} | ||
| runner = SimpleRunner(app) | ||
| async with runner as r: | ||
| sleep(0.1) |
There was a problem hiding this comment.
Using synchronous sleep() in an async test function blocks the event loop. Replace with await asyncio.sleep(0.1) to properly yield control in the async context.
| app = {} | ||
| runner = SimpleRunner(app) | ||
| async with runner as r: | ||
| sleep(0.1) |
There was a problem hiding this comment.
Using synchronous sleep() in an async test function blocks the event loop. Replace with await asyncio.sleep(0.1) to properly yield control in the async context.
| async def setup_method(self): | ||
| self._environment = environment_cls(sample_cls.get_config()) | ||
| await self._environment.__aenter__() | ||
|
|
||
| self._sample = sample_cls(self._environment) | ||
| await self._sample.__aenter__() |
There was a problem hiding this comment.
The sample_cls.get_config() is an async classmethod (as seen in sample.py), but it's being called without await. This should be await sample_cls.get_config() to properly retrieve the configuration.
| import sys | ||
| from io import StringIO | ||
| from typing import Optional | ||
| from threading import Lock |
There was a problem hiding this comment.
Import of 'Lock' is not used.
|
|
||
| @pytest.fixture | ||
| def agent_client(self) -> AgentClient: | ||
| return AgentClient(base_url="") |
There was a problem hiding this comment.
Keyword argument 'base_url' is not a supported parameter name of AgentClient.init.
| async def context_manager(): | ||
| with aioresponses() as mocked: | ||
| mocked.get("https://example.com/service-url", payload={"serviceUrl": "https://service.example.com"}) | ||
| client = AgentClient(base_url="https://example.com") |
There was a problem hiding this comment.
Keyword argument 'base_url' is not a supported parameter name of AgentClient.init.
| return ResponseClient(base_url="") | ||
|
|
||
| @pytest.fixture | ||
| def service_url(self): | ||
| with aioresponses() as mocked: | ||
| mocked.get("https://example.com/service-url", payload={"serviceUrl": "https://service.example.com"}) | ||
| client = ResponseClient(base_url="https://example.com") |
There was a problem hiding this comment.
Keyword argument 'base_url' is not a supported parameter name of ResponseClient.init.
| return ResponseClient(base_url="") | |
| @pytest.fixture | |
| def service_url(self): | |
| with aioresponses() as mocked: | |
| mocked.get("https://example.com/service-url", payload={"serviceUrl": "https://service.example.com"}) | |
| client = ResponseClient(base_url="https://example.com") | |
| return ResponseClient("") | |
| @pytest.fixture | |
| def service_url(self): | |
| with aioresponses() as mocked: | |
| mocked.get("https://example.com/service-url", payload={"serviceUrl": "https://service.example.com"}) | |
| client = ResponseClient("https://example.com") |
| return ResponseClient(base_url="") | ||
|
|
||
| @pytest.fixture | ||
| def service_url(self): | ||
| with aioresponses() as mocked: | ||
| mocked.get("https://example.com/service-url", payload={"serviceUrl": "https://service.example.com"}) | ||
| client = ResponseClient(base_url="https://example.com") |
There was a problem hiding this comment.
Keyword argument 'base_url' is not a supported parameter name of ResponseClient.init.
| return ResponseClient(base_url="") | |
| @pytest.fixture | |
| def service_url(self): | |
| with aioresponses() as mocked: | |
| mocked.get("https://example.com/service-url", payload={"serviceUrl": "https://service.example.com"}) | |
| client = ResponseClient(base_url="https://example.com") | |
| return ResponseClient("") | |
| @pytest.fixture | |
| def service_url(self): | |
| with aioresponses() as mocked: | |
| mocked.get("https://example.com/service-url", payload={"serviceUrl": "https://service.example.com"}) | |
| client = ResponseClient("https://example.com") |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 25 out of 30 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (1)
dev/integration/src/environments/aiohttp_environment.py:2
- Import of 'E' is not used.
from tkinter import E
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._service_url = service_url | ||
|
|
||
| async def teardown_method(self): | ||
| self._service_url = service_url |
There was a problem hiding this comment.
The teardown_method is redundantly reassigning the service_url that was already set in setup_method. This assignment serves no purpose in cleanup and should be removed.
| self._service_url = service_url | |
| pass |
| yield response_client | ||
|
|
||
| @pytest.fixture | ||
| def create_response_client(self) -> Callable[None, ResponseClient]: |
There was a problem hiding this comment.
The type annotation Callable[None, ResponseClient] is incorrect. For a callable with no arguments, use Callable[[], ResponseClient] instead. None is not a valid argument specification.
| def create_response_client(self) -> Callable[None, ResponseClient]: | |
| def create_response_client(self) -> Callable[[], ResponseClient]: |
| app = {} | ||
| runner = OtherSimpleRunner(app) | ||
| async with runner as r: | ||
| sleep(0.1) |
There was a problem hiding this comment.
Using synchronous sleep() in an async test blocks the event loop. Replace with await asyncio.sleep(0.1) to properly yield control in async context.
| app = {} | ||
| runner = SimpleRunner(app) | ||
| async with runner: | ||
| sleep(0.1) |
There was a problem hiding this comment.
Using synchronous sleep() in an async test blocks the event loop. Replace with await asyncio.sleep(0.1) to properly yield control in async context.
| async def _handle_conversation(self, request: Request) -> Response: | ||
| try: | ||
| body = await request.text() | ||
| activity = Activity.model_validate(body) |
There was a problem hiding this comment.
The body from request.text() is a string, but Activity.model_validate() expects a dict. This should be Activity.model_validate_json(body) or parse the JSON first with json.loads(body) then validate.
| return cast(Activity, activity_or_text) | ||
|
|
||
| async def send_activity(self, activity_or_text: Activity | str, timeout: Optional[float] = None) -> str: | ||
| timeout = timeout or self._default_timeout |
There was a problem hiding this comment.
Variable timeout is not used.
| return content | ||
|
|
||
| async def send_expect_replies(self, activity_or_text: Activity | str, timeout: Optional[float] = None) -> list[Activity]: | ||
| timeout = timeout or self._default_timeout |
There was a problem hiding this comment.
Variable timeout is not used.
| body = await request.text() | ||
| activity = Activity.model_validate(body) | ||
|
|
||
| conversation_id = activity.conversation.id if activity.conversation else None |
There was a problem hiding this comment.
Variable conversation_id is not used.
| conversation_id = activity.conversation.id if activity.conversation else None |
| @@ -0,0 +1,65 @@ | |||
| import json | |||
| from contextlib import contextmanager | |||
There was a problem hiding this comment.
Import of 'contextmanager' is not used.
| from contextlib import contextmanager |
| import re | ||
|
|
There was a problem hiding this comment.
Import of 're' is not used.
| import re |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 27 out of 33 changed files in this pull request and generated 16 comments.
Comments suppressed due to low confidence (8)
dev/integration/src/core/client/agent_client.py:1
- Import of 'os' is not used.
import os
dev/integration/src/core/aiohttp/aiohttp_environment.py:2
- Import of 'run_app' is not used.
from aiohttp.web import Request, Response, Application, run_app
dev/integration/src/core/integration.py:10
- Import of 'Union' is not used.
from typing import (
Optional,
TypeVar,
Union,
Callable,
Any,
AsyncGenerator,
)
dev/integration/src/core/client/response_client.py:6
- Import of 'Thread' is not used.
Import of 'Event' is not used.
from threading import Lock, Thread, Event
dev/integration/src/tests/test_framework/core/client/test_agent_client.py:2
- Import of 'contextmanager' is not used.
from contextlib import contextmanager
dev/integration/src/tests/test_framework/core/client/test_agent_client.py:3
- Import of 're' is not used.
import re
dev/integration/src/tests/test_framework/core/test_integration_from_service_url.py:4
- Import of 'copy' is not used.
dev/integration/src/tests/test_quickstart.py:2 - Import of 'asyncio' is not used.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,12 @@ | |||
| import pytest | |||
| import asyncio | |||
There was a problem hiding this comment.
The 'asyncio' module is imported but not used in this file. Remove this unused import.
| import pytest | ||
| import asyncio | ||
| import requests | ||
| from copy import copy |
There was a problem hiding this comment.
The 'copy' function is imported but not used in this file. Remove this unused import.
dev/integration/env.TEMPLATE
Outdated
| aioresponses | ||
| microsoft-agents-activity | ||
| microsoft-agents-hosting-core No newline at end of file |
There was a problem hiding this comment.
This file appears to be misnamed or incorrectly formatted. Based on the naming pattern 'env.TEMPLATE' and comparison with 'dev/integration/src/tests/env.TEMPLATE', this should likely contain environment variable templates (e.g., KEY=value format) rather than package names. The content looks like it belongs in a requirements file instead.
| aioresponses | |
| microsoft-agents-activity | |
| microsoft-agents-hosting-core | |
| # Example environment variable template | |
| # Copy this file to .env and fill in the values as needed | |
| API_KEY= | |
| DATABASE_URL= | |
| DEBUG=false |
| def create_runner(self) -> ApplicationRunner: | ||
| """Create an application runner for the environment.""" |
There was a problem hiding this comment.
The abstract method signature for 'create_runner' in the base class returns 'ApplicationRunner' without parameters, but the concrete implementation in 'AiohttpEnvironment' requires 'host' and 'port' parameters. This creates an inconsistent interface. Consider adding these parameters to the base class signature or using a configuration object.
| def create_runner(self) -> ApplicationRunner: | |
| """Create an application runner for the environment.""" | |
| def create_runner(self, host: str, port: int) -> ApplicationRunner: | |
| """Create an application runner for the environment. | |
| Args: | |
| host (str): The host address to bind the runner. | |
| port (int): The port number to bind the runner. | |
| """ |
| @@ -0,0 +1,130 @@ | |||
| import os | |||
There was a problem hiding this comment.
The 'os' module is imported but not used anywhere in this file. Remove this unused import.
| import os |
| with aioresponses() as mocked: | ||
|
|
||
| def callback(url, **kwargs): | ||
| a = requests.post( |
There was a problem hiding this comment.
Variable a is not used.
| **self.config | ||
| ) | ||
|
|
||
| def create_runner(self, host: str, port: int) -> ApplicationRunner: |
There was a problem hiding this comment.
This method requires 3 positional arguments, whereas overridden Environment.create_runner requires 1.
| @@ -0,0 +1,58 @@ | |||
| from tkinter import E | |||
There was a problem hiding this comment.
Import of 'E' is not used.
| from tkinter import E |
|
|
||
| import aiohttp.web | ||
|
|
||
| from .application_runner import ApplicationRunner |
There was a problem hiding this comment.
Import of 'ApplicationRunner' is not used.
| from .application_runner import ApplicationRunner |
| assert activities[0].type == "message" | ||
| assert activities[0].text == "Hello, World!" | ||
|
|
||
| assert (await response_client.pop()) == [] |
There was a problem hiding this comment.
This 'assert' statement contains an expression which may have side effects.
| assert (await response_client.pop()) == [] | |
| popped_activities = await response_client.pop() | |
| assert popped_activities == [] |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 27 out of 33 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
dev/integration/src/core/aiohttp/aiohttp_environment.py:1
- Import of 'E' is not used.
from tkinter import E
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import pytest | ||
| import asyncio | ||
|
|
||
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment, AiohttpEnvironment |
There was a problem hiding this comment.
Duplicate import of AiohttpEnvironment. Remove the duplicate occurrence from the import statement.
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment, AiohttpEnvironment | |
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment |
|
|
||
| async def _stop_server(self): | ||
| if not self._server_thread: | ||
| raise RuntimeError("ResponseClient is not running.") |
There was a problem hiding this comment.
Error message refers to 'ResponseClient' but this is the AiohttpRunner class. The message should say 'AiohttpRunner is not running.' or 'Server is not running.'
| conversation_id = ( | ||
| activity.conversation.id if activity.conversation else None | ||
| ) |
There was a problem hiding this comment.
Variable conversation_id is not used.
| conversation_id = ( | |
| activity.conversation.id if activity.conversation else None | |
| ) | |
| # Removed unused variable 'conversation_id' |
| from typing import ( | ||
| Optional, | ||
| TypeVar, | ||
| Union, | ||
| Callable, | ||
| Any, | ||
| AsyncGenerator, | ||
| ) |
There was a problem hiding this comment.
Import of 'Union' is not used.
| import aiohttp.web | ||
| from dotenv import load_dotenv | ||
|
|
||
| from .application_runner import ApplicationRunner |
There was a problem hiding this comment.
Import of 'ApplicationRunner' is not used.
| from .application_runner import ApplicationRunner |
| @@ -0,0 +1,22 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
| import pytest | ||
| import asyncio | ||
|
|
||
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment, AiohttpEnvironment |
There was a problem hiding this comment.
Import of 'integration' is not used.
Import of 'IntegrationFixtures' is not used.
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment, AiohttpEnvironment | |
| from src.core import AiohttpEnvironment, AiohttpEnvironment |
| import pytest | ||
| import asyncio | ||
| import requests | ||
| from copy import copy |
There was a problem hiding this comment.
Import of 'copy' is not used.
| @@ -0,0 +1,12 @@ | |||
| import pytest | |||
| import asyncio | |||
There was a problem hiding this comment.
Import of 'asyncio' is not used.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 26 out of 31 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
dev/integration/src/core/aiohttp/aiohttp_environment.py:1
- Import of 'E' is not used.
from tkinter import E
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| json=activity.model_dump(by_alias=True, exclude_unset=True, exclude_none=True, mode="json"), | ||
| ) as response: | ||
| content = await response.text() | ||
| breakpoint() |
There was a problem hiding this comment.
A breakpoint() statement is left in the production code. This should be removed before merging as it will halt execution in production environments.
| breakpoint() |
| import pytest | ||
| import asyncio | ||
|
|
||
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment, AiohttpEnvironment, AgentClient |
There was a problem hiding this comment.
AiohttpEnvironment is imported twice. Remove the duplicate import.
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment, AiohttpEnvironment, AgentClient | |
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment, AgentClient |
| timeout = timeout or self._default_timeout | ||
| activity = self._to_activity(activity_or_text) | ||
| activity.delivery_mode = DeliveryModes.expect_replies | ||
| activity.service_url = activity.service_url or "http://localhost" # temporary fix |
There was a problem hiding this comment.
Hardcoded fallback URL 'http://localhost' with a comment indicating this is a 'temporary fix'. Consider making this configurable or documenting why this default is necessary. If this is truly temporary, create a TODO or track it for proper resolution.
| async def send_activity( | ||
| self, activity_or_text: Activity | str, sleep: float = 0, timeout: Optional[float] = None | ||
| ) -> str: | ||
| timeout = timeout or self._default_timeout |
There was a problem hiding this comment.
Variable timeout is not used.
| print(f"Server running at http://{host}:{port}/api/messages") | ||
| while True: | ||
|
|
||
| res = await client.send_expect_replies("Hello, Agent!") |
There was a problem hiding this comment.
Variable res is not used.
| res = await client.send_expect_replies("Hello, Agent!") | |
| await client.send_expect_replies("Hello, Agent!") |
| **self.config | ||
| ) | ||
|
|
||
| def create_runner(self, host: str, port: int) -> ApplicationRunner: |
There was a problem hiding this comment.
This method requires 3 positional arguments, whereas overridden Environment.create_runner requires 1.
| def create_runner(self, host: str, port: int) -> ApplicationRunner: | |
| def create_runner(self, runner_config: dict) -> ApplicationRunner: | |
| host = runner_config.get("host", "127.0.0.1") | |
| port = runner_config.get("port", 8080) |
| @@ -0,0 +1,48 @@ | |||
| import os | |||
| import pytest | |||
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
| import pytest | ||
| import asyncio | ||
|
|
||
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment, AiohttpEnvironment, AgentClient |
There was a problem hiding this comment.
Import of 'integration' is not used.
Import of 'IntegrationFixtures' is not used.
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment, AiohttpEnvironment, AgentClient | |
| from src.core import AiohttpEnvironment, AiohttpEnvironment, AgentClient |
| import pytest | ||
| import asyncio | ||
| import requests | ||
| from copy import copy |
There was a problem hiding this comment.
Import of 'copy' is not used.
| from copy import copy |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 29 out of 36 changed files in this pull request and generated 14 comments.
Comments suppressed due to low confidence (1)
dev/integration/src/tests/manual_test.py:2
- Import of 'pytest' is not used.
import pytest
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| async def __aenter__(self): | ||
| if self._server_thread: | ||
| raise RuntimeError("ResponseClient is already running.") |
There was a problem hiding this comment.
Error message mentions 'ResponseClient' but this is the AiohttpRunner class. The message should say 'AiohttpRunner is already running.' or 'Server is already running.'
| raise RuntimeError("ResponseClient is already running.") | |
| raise RuntimeError("Server is already running.") |
| @@ -0,0 +1,41 @@ | |||
| import os | |||
| import pytest | |||
There was a problem hiding this comment.
Unused import. The pytest module is imported but never used in this file.
| import pytest |
| @integration() | ||
| class TestFoundation(IntegrationFixtures): | ||
|
|
||
| def load_activity(self, activity_name) -> Activity: |
There was a problem hiding this comment.
Missing import for return type annotation. The method uses Activity as a return type but it's not imported in this file. Add Activity to the imports from microsoft_agents.activity.
| assert result is not None | ||
| last = result[-1] | ||
| assert last.type == ActivityTypes.message | ||
| assert last.text.lower() == "you said: {activity.text}".lower() |
There was a problem hiding this comment.
String formatting issue. The assertion uses a literal string {activity.text} instead of an f-string. Change to f\"you said: {activity.text}\" to properly interpolate the activity.text variable.
| assert last.text.lower() == "you said: {activity.text}".lower() | |
| assert last.text.lower() == f"you said: {activity.text}".lower() |
|
|
||
| def load_activity(channel: str, name: str) -> Activity: | ||
|
|
||
| with open("./dev/integration/src/tests/integration/foundational/activities/{}/{}.json".format(channel, name), "r") as f: |
There was a problem hiding this comment.
[nitpick] Use f-string for better readability. Replace .format(channel, name) with an f-string: f\"./dev/integration/src/tests/integration/foundational/activities/{channel}/{name}.json\".
| with open("./dev/integration/src/tests/integration/foundational/activities/{}/{}.json".format(channel, name), "r") as f: | |
| with open(f"./dev/integration/src/tests/integration/foundational/activities/{channel}/{name}.json", "r") as f: |
| import asyncio | ||
|
|
There was a problem hiding this comment.
Import of 'asyncio' is not used.
| import asyncio |
| ActivityTypes, | ||
| ) | ||
|
|
||
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment |
There was a problem hiding this comment.
Import of 'AiohttpEnvironment' is not used.
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment | |
| from src.core import integration, IntegrationFixtures |
| ) | ||
|
|
||
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment | ||
| from src.samples import QuickstartSample |
There was a problem hiding this comment.
Import of 'QuickstartSample' is not used.
| from src.samples import QuickstartSample |
dev/integration/src/core/utils.py
Outdated
| from microsoft_agents.activity import ( | ||
| Activity, | ||
| ActivityTypes, | ||
| DeliveryModes, | ||
| ConversationAccount, | ||
| ChannelAccount, | ||
| ) |
There was a problem hiding this comment.
Import of 'ActivityTypes' is not used.
| assert activities[0].type == "message" | ||
| assert activities[0].text == "Hello, World!" | ||
|
|
||
| assert (await response_client.pop()) == [] |
There was a problem hiding this comment.
This 'assert' statement contains an expression which may have side effects.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 30 out of 38 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
dev/integration/src/core/aiohttp/aiohttp_environment.py:1
- Import of 'E' is not used.
from tkinter import E
dev/integration/src/tests/manual_test.py:2
- Import of 'pytest' is not used.
import pytest
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from microsoft_agents.activity import Activity | ||
|
|
||
| def load_activity(channel: str, name: str) -> Activity: | ||
|
|
||
| with open("./dev/integration/src/tests/integration/foundational/activities/{}/{}.json".format(channel, name), "r") as f: |
There was a problem hiding this comment.
Hardcoded relative path may fail depending on execution context. Consider using pathlib.Path(__file__).parent to construct a path relative to the current file, or use os.path.join() with a base directory constant to make the path more robust.
| from microsoft_agents.activity import Activity | |
| def load_activity(channel: str, name: str) -> Activity: | |
| with open("./dev/integration/src/tests/integration/foundational/activities/{}/{}.json".format(channel, name), "r") as f: | |
| from pathlib import Path | |
| from microsoft_agents.activity import Activity | |
| def load_activity(channel: str, name: str) -> Activity: | |
| activity_path = Path(__file__).parent / "activities" / channel / f"{name}.json" | |
| with open(activity_path, "r") as f: |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 44 out of 71 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (2)
dev/integration/src/core/aiohttp/aiohttp_environment.py:1
- Import of 'E' is not used.
from tkinter import E
dev/integration/src/core/aiohttp/aiohttp_environment.py:2
- Import of 'run_app' is not used.
from aiohttp.web import Request, Response, Application, run_app
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| async def _stop_server(self): | ||
| if not self._server_thread: | ||
| raise RuntimeError("ResponseClient is not running.") |
There was a problem hiding this comment.
Error message references 'ResponseClient' but this is the AiohttpRunner class. The message should say 'AiohttpRunner is not running.' or 'Server is not running.'
| raise RuntimeError("ResponseClient is not running.") | |
| raise RuntimeError("AiohttpRunner is not running.") |
| import pytest | ||
| import asyncio | ||
|
|
||
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment, AiohttpEnvironment, AgentClient |
There was a problem hiding this comment.
Duplicate import of AiohttpEnvironment. Remove one of these duplicates.
| app: Optional[AppT] = None, | ||
| **kwargs | ||
| ) -> Callable[[T], T]: | ||
| """Factory function to create an Integration instance based on provided parameters. | ||
|
|
||
| Essentially resolves to one of the static methods of Integration: | ||
| `from_service_url`, `from_sample`, or `from_app`, | ||
| based on the provided parameters. | ||
|
|
||
| If a service URL is provided, it creates the Integration using that. | ||
| If both sample and environment are provided, it creates the Integration using them. | ||
| If an aiohttp application is provided, it creates the Integration using that. | ||
|
|
||
| :param cls: The Integration class type. | ||
| :param service_url: Optional service URL to connect to. | ||
| :param sample: Optional Sample instance. | ||
| :param environment: Optional Environment instance. | ||
| :param host_agent: Flag to indicate if the agent should be hosted. | ||
| :param app: Optional aiohttp application instance. |
There was a problem hiding this comment.
The app parameter is defined but never used in the function body. Consider removing it or implementing the intended functionality.
| app: Optional[AppT] = None, | |
| **kwargs | |
| ) -> Callable[[T], T]: | |
| """Factory function to create an Integration instance based on provided parameters. | |
| Essentially resolves to one of the static methods of Integration: | |
| `from_service_url`, `from_sample`, or `from_app`, | |
| based on the provided parameters. | |
| If a service URL is provided, it creates the Integration using that. | |
| If both sample and environment are provided, it creates the Integration using them. | |
| If an aiohttp application is provided, it creates the Integration using that. | |
| :param cls: The Integration class type. | |
| :param service_url: Optional service URL to connect to. | |
| :param sample: Optional Sample instance. | |
| :param environment: Optional Environment instance. | |
| :param host_agent: Flag to indicate if the agent should be hosted. | |
| :param app: Optional aiohttp application instance. | |
| **kwargs | |
| ) -> Callable[[T], T]: | |
| """Factory function to create an Integration instance based on provided parameters. | |
| Essentially resolves to one of the static methods of Integration: | |
| `from_service_url` or `from_sample`, | |
| based on the provided parameters. | |
| If a service URL is provided, it creates the Integration using that. | |
| If both sample and environment are provided, it creates the Integration using them. | |
| :param cls: The Integration class type. | |
| :param service_url: Optional service URL to connect to. | |
| :param sample: Optional Sample instance. | |
| :param environment: Optional Environment instance. | |
| :param host_agent: Flag to indicate if the agent should be hosted. |
| print(f"Server running at http://{host}:{port}/api/messages") | ||
| while True: | ||
| await asyncio.sleep(1) | ||
| res = await client.send_expect_replies("Hello, Agent!") |
There was a problem hiding this comment.
Variable res is not used.
| data = await request.json() | ||
| activity = Activity.model_validate(data) | ||
|
|
||
| conversation_id = ( |
There was a problem hiding this comment.
Variable conversation_id is not used.
| @@ -0,0 +1,141 @@ | |||
| import json | |||
There was a problem hiding this comment.
Syntax Error (in Python 3).
| @@ -0,0 +1,136 @@ | |||
| import os | |||
There was a problem hiding this comment.
Import of 'os' is not used.
| @@ -0,0 +1,41 @@ | |||
| import os | |||
| import pytest | |||
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest | ||
| import asyncio | ||
|
|
||
| from src.core import integration, IntegrationFixtures, AiohttpEnvironment, AiohttpEnvironment, AgentClient |
There was a problem hiding this comment.
Import of 'integration' is not used.
Import of 'IntegrationFixtures' is not used.
| import sys | ||
| from io import StringIO | ||
| from typing import Optional | ||
| from threading import Lock, Thread, Event |
There was a problem hiding this comment.
Import of 'Thread' is not used.
Import of 'Event' is not used.
...microsoft-agents-testing/microsoft_agents/testing/integration/core/client/response_client.py
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 39 out of 45 changed files in this pull request and generated 16 comments.
Comments suppressed due to low confidence (5)
dev/microsoft-agents-testing/microsoft_agents/testing/integration/core/client/agent_client.py:1
- Import of 'default' is not used.
dev/microsoft-agents-testing/microsoft_agents/testing/integration/core/aiohttp/aiohttp_environment.py:1 - Import of 'E' is not used.
from tkinter import E
dev/benchmark/src/main.py:1
- Import of 'sys' is not used.
import json, sys
dev/benchmark/src/main.py:2
- Import of 'StringIO' is not used.
from io import StringIO
dev/benchmark/src/main.py:5
- Import of 'contextmanager' is not used.
from contextlib import contextmanager
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from typing import Optional | ||
| from typing import Optional |
There was a problem hiding this comment.
Duplicate import statement. Remove the second from typing import Optional on line 2.
| @@ -0,0 +1,58 @@ | |||
| from tkinter import E | |||
There was a problem hiding this comment.
Unused import. The E class from tkinter is imported but never used in this module. Remove this import statement.
| from tkinter import E |
| @@ -0,0 +1,152 @@ | |||
| from email.policy import default | |||
There was a problem hiding this comment.
Unused import. The default object from email.policy is imported but never used in this module. Remove this import statement.
| import json, sys | ||
| from io import StringIO |
There was a problem hiding this comment.
Unused imports. The sys and StringIO imports are not used in the code. Remove them.
| import json, sys | |
| from io import StringIO | |
| import json |
| from io import StringIO | ||
| import logging | ||
| from datetime import datetime, timezone | ||
| from contextlib import contextmanager |
There was a problem hiding this comment.
Unused import. The contextmanager decorator is imported but never used. Remove this import statement.
| from contextlib import contextmanager |
| **self.config | ||
| ) | ||
|
|
||
| def create_runner(self, host: str, port: int) -> ApplicationRunner: |
There was a problem hiding this comment.
This method requires 3 positional arguments, whereas overridden Environment.create_runner requires 1.
| def create_runner(self, host: str, port: int) -> ApplicationRunner: | |
| def create_runner(self, **kwargs) -> ApplicationRunner: | |
| host = kwargs.get("host", "127.0.0.1") | |
| port = kwargs.get("port", 8080) |
| @@ -0,0 +1,152 @@ | |||
| from email.policy import default | |||
| import os | |||
There was a problem hiding this comment.
Import of 'os' is not used.
| @@ -0,0 +1,58 @@ | |||
| from tkinter import E | |||
| from aiohttp.web import Request, Response, Application, run_app | |||
There was a problem hiding this comment.
Import of 'run_app' is not used.
| from aiohttp.web import Request, Response, Application, run_app | |
| from aiohttp.web import Request, Response, Application |
|
|
||
| import sys | ||
| from io import StringIO | ||
| from typing import Optional |
There was a problem hiding this comment.
Import of 'Optional' is not used.
| import sys | ||
| from io import StringIO | ||
| from typing import Optional | ||
| from threading import Lock, Thread, Event |
There was a problem hiding this comment.
Import of 'Thread' is not used.
Import of 'Event' is not used.
1 participant
This pull request introduces a new integration testing framework for agent-based applications, adding core abstractions, client utilities, and environment setup. It establishes a modular structure for integration tests, including agent and response clients, application runners, and environment definitions. The changes also add requirements for new dependencies and set up import/export patterns for the new modules.
Key changes include:
Core abstractions and environment setup:
Environmentabstract base class to define the structure and initialization of integration test environments, supporting agent applications, storage, adapters, and connection management. (dev/integration/src/core/environment.py)ApplicationRunnerabstract base class for managing the lifecycle (start/stop) of agent applications in integration tests, with support for asynchronous context management. (dev/integration/src/core/application_runner.py)Client utilities:
AgentClientfor sending authenticated activities to agent endpoints, including support for access token acquisition and handling both simple and expect-replies message flows. (dev/integration/src/core/client/agent_client.py)ResponseClientto simulate a server receiving activities, supporting activity validation and response logic, with a stub for streamed activity handling. (dev/integration/src/core/client/response_client.py)BotResponsefor simulating bot endpoints, including activity storage, streaming support, and a simple aiohttp-based web server. (dev/integration/src/core/client/bot_response.py)Integration framework and utilities:
integrationdecorator/factory, providing flexible integration test setup using service URLs, sample/environment classes, or aiohttp apps, and supporting response server hosting. (dev/integration/src/core/integration.py)core/__init__.pyandcore/client/__init__.pyfor consistent import patterns. (dev/integration/src/core/__init__.py,dev/integration/src/core/client/__init__.py) [1] [2]Dependency and environment configuration:
aioresponses,microsoft-agents-activity,microsoft-agents-hosting-core) to the integration environment and requirements. (dev/integration/env.TEMPLATE,dev/integration/requirements.txt) [1] [2]These changes lay the groundwork for robust, modular, and reusable integration tests for agent-based systems.