-
Notifications
You must be signed in to change notification settings - Fork 50
Integration package refinements #266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 PR introduces several refinements to the integration testing package, including support for invoke responses, renaming of the Selector class to ModelSelector, new utility functions, and a comprehensive benchmarking CLI tool. However, the changes contain several critical bugs that must be addressed before merging.
Key Changes
- Added support for invoke activity responses in data-driven tests with new
InvokeResponsehandling - Renamed
SelectortoModelSelectorfor better clarity across the codebase - Introduced
resolve_envutility for loading environment configuration - Added comprehensive benchmark CLI tool with thread and coroutine execution modes
Reviewed changes
Copilot reviewed 14 out of 35 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| test_ddt.py | Updated mock assertions to include prefix="" parameter |
| test_data_driven_test.py | Added test cases for expect_replies delivery mode and invoke response assertions |
| test_agent_client.py | Added test for new send_invoke_activity method |
| test_selector.py | Renamed Selector to ModelSelector throughout test cases |
| test_model_assertion.py | Renamed Selector to ModelSelector in assertion tests |
| resolve_env.py | New utility for resolving .env files from paths or directories |
| utils/init.py | Exported new resolve_env utility |
| data_driven_test.py | Added invoke response support with refactored input/assertion handling |
| response_client.py | Improved streaming activity handling with new _handle_streamed_activity implementation |
| agent_client.py | Added send_invoke_activity method and refactored sleep handling |
| auto_client.py | Removed commented-out placeholder code |
| test_auth.py | Added placeholder test file |
| auth_sample.py | New authentication sample implementation |
| benchmark/* | Complete benchmarking tool infrastructure with executors, configuration, and documentation |
| init.py | Added cli module export |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| model_list: list | ||
|
|
||
| if step.get("assertion_type") == "activity": | ||
| model_list = responses | ||
|
|
||
| if step.get("assertion_type") == "invoke_response": | ||
| model_list = invoke_responses |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable model_list is declared but may not be initialized before use. If assertion_type is neither "activity" nor "invoke_response", or if it's not set at all, the variable will be uninitialized when passed to assertion.check() on line 135, causing a UnboundLocalError.
Consider:
- Initializing
model_listwith a default value (e.g.,model_list: list = []) - Using
elifon line 132 instead of a secondif - Adding an
elseclause to handle unexpected assertion types
| model_list: list | |
| if step.get("assertion_type") == "activity": | |
| model_list = responses | |
| if step.get("assertion_type") == "invoke_response": | |
| model_list = invoke_responses | |
| model_list: list = [] | |
| assertion_type = step.get("assertion_type") | |
| if assertion_type == "activity": | |
| model_list = responses | |
| elif assertion_type == "invoke_response": | |
| model_list = invoke_responses | |
| else: | |
| raise ValueError(f"Unsupported assertion_type: {assertion_type!r} in step: {step}") |
| if "invokeResponse" in selector: | ||
| selector["model"] = selector["invokeResponse"] | ||
| if "invoke_response" in selector: |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the assertion_type logic issue, lines 85-90 have multiple if statements that could execute sequentially, potentially overwriting the selector["model"] value when multiple keys exist.
Use elif statements to ensure only one selector model is set:
if "activity" in selector:
selector["model"] = selector["activity"]
elif "invokeResponse" in selector:
selector["model"] = selector["invokeResponse"]
elif "invoke_response" in selector:
selector["model"] = selector["invoke_response"]| if "invokeResponse" in selector: | |
| selector["model"] = selector["invokeResponse"] | |
| if "invoke_response" in selector: | |
| elif "invokeResponse" in selector: | |
| selector["model"] = selector["invokeResponse"] | |
| elif "invoke_response" in selector: |
| if ( | ||
| stream_info.stream_sequence <= 0 | ||
| and activity.type == ActivityTypes.typing | ||
| ): | ||
| raise ValueError( | ||
| "Streamed activity's stream sequence should be a positive number." | ||
| ) |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic condition on lines 110-113 is confusing. It checks if stream_sequence <= 0 AND activity.type == typing, but the error message suggests it should be checking if stream_sequence <= 0 AND activity.type != typing.
The condition appears backwards - it raises an error when the sequence is <= 0 AND it's a typing activity, but the error message says streaming activities should have positive sequence numbers. This suggests the condition should be checking for streaming activities (non-typing messages), not typing activities.
Consider clarifying the logic or the error message to match the intended behavior.
| async def _handle_streamed_activity( | ||
| self, activity: Activity, *args, **kwargs | ||
| self, activity: Activity, stream_info: Entity, cid: str | ||
| ) -> bool: |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function _handle_streamed_activity is defined with parameters activity, stream_info, and cid, but is called with only activity. This will cause a TypeError at runtime.
The function should either:
- Be called with all required parameters extracted from the activity
- Have its signature updated to match the call site (taking only
activityand extractingstream_infoandcidinternally)
|
|
||
|
|
||
| def create_auth_route(auth_handler_id: str, agent: AgentApplication): | ||
| def dynamic_function(context: TurnContext, state: TurnState): |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function dynamic_function is defined as a regular function but uses await on line 12, making it an async function. This will cause a SyntaxError.
Change line 11 to:
async def dynamic_function(context: TurnContext, state: TurnState):| def dynamic_function(context: TurnContext, state: TurnState): | |
| async def dynamic_function(context: TurnContext, state: TurnState): |
| ) -> InvokeResponse: | ||
|
|
||
| if not activity.type == ActivityTypes.invoke: | ||
| raise ValueError("Activity type must be 'invoke' for send_invoke method.") |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message on line 181 refers to "send_invoke method" but the actual method name is "send_invoke_activity". This inconsistency could confuse users debugging errors.
Update to:
raise ValueError("Activity type must be 'invoke' for send_invoke_activity method.")| raise ValueError("Activity type must be 'invoke' for send_invoke method.") | |
| raise ValueError("Activity type must be 'invoke' for send_invoke_activity method.") |
| except ValidationError: | ||
| raise ValueError( | ||
| "Error when sending invoke activity: InvokeResponse not returned or invalid format." | ||
| ) |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception handling on lines 185-191 catches ValidationError but doesn't preserve the original error information. This makes debugging difficult when the InvokeResponse validation fails.
Consider preserving the original exception:
except ValidationError as e:
raise ValueError(
f"Error when sending invoke activity: InvokeResponse not returned or invalid format. Details: {e}"
) from e| except ValidationError: | |
| raise ValueError( | |
| "Error when sending invoke activity: InvokeResponse not returned or invalid format." | |
| ) | |
| except ValidationError as e: | |
| raise ValueError( | |
| f"Error when sending invoke activity: InvokeResponse not returned or invalid format. Details: {e}" | |
| ) from e |
| @classmethod | ||
| async def get_config(cls) -> dict: | ||
| """Retrieve the configuration for the sample.""" | ||
| load_dotenv("./src/tests/.env") | ||
| return dict(os.environ) |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing async keyword in function definition. The function contains await on line 12 but is not declared as async, which will cause a SyntaxError.
Change the function signature to:
@classmethod
async def get_config(cls) -> dict:| for auth_handler in app.config.authentication_handlers: | ||
| app.message( | ||
| auth_handler.name.lower(), | ||
| create_auth_route(auth_handler.name), |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call to function create_auth_route with too few arguments; should be no fewer than 2.
| create_auth_route(auth_handler.name), | |
| create_auth_route(auth_handler.name, app), |
| @@ -0,0 +1,5 @@ | |||
| import cli | |||
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'cli' is not used.
| import cli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 27 out of 50 changed files in this pull request and generated 24 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| environment.setup() | ||
|
|
||
| sample = AuthSample() | ||
| sample.run(resolve_env) No newline at end of file |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sample.run(resolve_env) call appears incorrect. The run method likely expects a configuration dictionary, not the resolve_env function itself. This should probably be sample.run(resolve_env(config_path)) or similar.
| ) -> InvokeResponse: | ||
|
|
||
| if not activity.type == ActivityTypes.invoke: | ||
| raise ValueError("Activity type must be 'invoke' for send_invoke method.") |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message refers to "send_invoke method" but the actual method name is "send_invoke_activity". The error message should be updated to match the method name for clarity.
| class QuickstartSample(Sample): | ||
| """A quickstart sample implementation.""" |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class is named QuickstartSample but should be AuthSample based on the file name and context. Additionally, the docstring "A quickstart sample implementation." doesn't accurately describe this authentication-specific sample.
| @@ -0,0 +1,5 @@ | |||
| import cli | |||
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement import cli will fail because cli is not a valid module. This appears to be an incomplete or incorrect import that should either import from .cli or be removed entirely.
| assertion = self._load_assertion(step) | ||
| responses.extend(await response_client.pop()) | ||
|
|
||
| model_list: list |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type annotation model_list: list is too generic. It should be model_list: list[Activity] | list[InvokeResponse] or use a Union type to properly reflect what's being stored. This improves type safety and code clarity.
| await self._add(activity) | ||
| if activity.type != ActivityTypes.typing: | ||
| await asyncio.sleep(0.05) # Simulate processing delay | ||
|
|
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace on this line should be removed.
| for auth_handler in app.config.authentication_handlers: | ||
| app.message( | ||
| auth_handler.name.lower(), | ||
| create_auth_route(auth_handler.name), |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call to function create_auth_route with too few arguments; should be no fewer than 2.
| for auth_handler in app.config.authentication_handlers: | ||
| app.message( | ||
| auth_handler.name.lower(), | ||
| create_auth_route(auth_handler.name), |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call to function create_auth_route with too few arguments; should be no fewer than 2.
| Activity, | ||
| ActivityTypes, | ||
| ) | ||
| from microsoft_agents.activity import Activity, ActivityTypes, Entity |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Entity' is not used.
| @@ -0,0 +1,5 @@ | |||
| import cli | |||
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'cli' is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 37 out of 60 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| model_list: list | ||
|
|
||
| if step.get("assertion_type") == "activity": | ||
| model_list = responses | ||
|
|
||
| if step.get("assertion_type") == "invoke_response": | ||
| model_list = invoke_responses |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable model_list may be uninitialized if assertion_type is neither "activity" nor "invoke_response". This will cause an UnboundLocalError when model_list is used on line 146. Consider adding an else clause or initializing model_list before the if statements, or using elif for the second condition.
| from .auth_sample import AuthSample | ||
|
|
||
| from microsoft_agents.testing.utils import resolve_env | ||
| from microsoft_agents.testing.core import AiohttpEnvironment |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid import path. The import from microsoft_agents.testing.core import AiohttpEnvironment appears to be incorrect. Based on the codebase structure, the correct import should be from microsoft_agents.testing.integration.core import AiohttpEnvironment.
| def mock_mcs_handler(activity: Activity) -> Callable[[Request], Awaitable[Response]]: | ||
| """Creates a mock handler for MCS endpoint returning the given activity.""" | ||
|
|
||
| async def handler(request: Request) -> Response: | ||
| activity_data = activity.model_dump_json(exclude_unset=True) | ||
| return Response(body=activity_data) | ||
|
|
||
| return handler | ||
|
|
||
|
|
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate function definition. The function mock_mcs_handler is defined twice with different signatures (lines 19-26 and 28-45), which will cause the second definition to override the first. The second definition is the one actually used on line 60. Consider renaming one of them or removing the unused first definition.
| def mock_mcs_handler(activity: Activity) -> Callable[[Request], Awaitable[Response]]: | |
| """Creates a mock handler for MCS endpoint returning the given activity.""" | |
| async def handler(request: Request) -> Response: | |
| activity_data = activity.model_dump_json(exclude_unset=True) | |
| return Response(body=activity_data) | |
| return handler |
| await self._add(activity) | ||
| if activity.type != ActivityTypes.typing: | ||
| await asyncio.sleep(0.05) # Simulate processing delay | ||
|
|
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace detected. There are trailing spaces on this line which should be removed to follow Python style guidelines (PEP 8).
| await self._server_task | ||
| except asyncio.CancelledError: | ||
| pass | ||
|
|
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace detected. There are trailing spaces on this line which should be removed to follow Python style guidelines (PEP 8).
| return True, None | ||
|
|
||
| elif isinstance(baseline, list): | ||
|
|
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace detected. There are trailing spaces on this line which should be removed to follow Python style guidelines (PEP 8).
| """Compile the data driven test to ensure all steps are valid.""" | ||
| for step in self._test: | ||
| for i, step in enumerate(self._test): | ||
|
|
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace detected. There are trailing spaces on this line which should be removed to follow Python style guidelines (PEP 8).
| else: | ||
| step["assertion"] = {} | ||
| step["assertion_type"] = "activity" | ||
|
|
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace detected. There are trailing spaces on this line which should be removed to follow Python style guidelines (PEP 8).
| assert call_args.locale == "en-US" | ||
| assert call_args.channel_id == "test-channel" | ||
|
|
||
| async def test_with_invoke_response_assertion(self): |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing @pytest.mark.asyncio decorator. This async test method lacks the required pytest asyncio decorator. All async test methods in this file use @pytest.mark.asyncio, but this one is missing it on line 718, which will cause the test to fail when executed.
| self._server_task.cancel() | ||
| try: | ||
| await self._server_task | ||
| except asyncio.CancelledError: |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'except' clause does nothing but pass and there is no explanatory comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 93 out of 207 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
dev/microsoft-agents-testing/microsoft_agents/testing/integration/client/response_client.py:13
- Import of 'Entity' is not used.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @classmethod | ||
| async def get_config(cls) -> dict: | ||
| """Retrieve the configuration for the sample.""" | ||
| load_dotenv("./src/tests/.env") |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file path "./src/tests/.env" appears to be incorrect. The directory structure suggests the .env file should be in the root or tests directory, not "./src/tests/.env".
|
|
||
| @app.error | ||
| async def on_error(context: TurnContext, error: Exception): | ||
| # This check writes out errors to console log .vs. app insights. |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a spelling/grammar issue: the abbreviation ".vs." should be "vs." or "versus". Additionally, this comment has inconsistent style with the next line using proper NOTE format.
| # import pytest | ||
|
|
||
| # from typing import Awaitable, Callable, Iterable | ||
|
|
||
| # from aiohttp.web import Request, Response, Application, StreamResponse | ||
|
|
||
| # from microsoft_agents.activity import Activity | ||
|
|
||
| # from microsoft_agents.copilotstudio.client import ( | ||
| # CopilotClient, | ||
| # ConnectionSettings, | ||
| # PowerPlatformEnvironment, | ||
| # ) | ||
|
|
||
| # from microsoft_agents.testing.integration.core import AiohttpRunner | ||
|
|
||
|
|
||
| # def mock_mcs_handler(activity: Activity) -> Callable[[Request], Awaitable[Response]]: | ||
| # """Creates a mock handler for MCS endpoint returning the given activity.""" | ||
|
|
||
| # async def handler(request: Request) -> Response: | ||
| # activity_data = activity.model_dump_json(exclude_unset=True) | ||
| # return Response(body=activity_data) | ||
|
|
||
| # return handler | ||
|
|
||
|
|
||
| # def mock_mcs_handler( | ||
| # activities: Iterable[Activity], | ||
| # ) -> Callable[[Request], Awaitable[StreamResponse]]: | ||
| # """Creates a mock handler for MCS endpoint returning SSE-formatted activity.""" | ||
|
|
||
| # async def handler(request: Request) -> StreamResponse: | ||
| # response = StreamResponse(status=200) | ||
| # response.headers["Content-Type"] = "text/event-stream" | ||
| # response.headers["x-ms-conversationid"] = "test-conv-id" | ||
| # # response.headers['Content-Length'] = str(len(activity_data)) | ||
| # await response.prepare(request) | ||
|
|
||
| # # Proper SSE format | ||
| # for activity in activities: | ||
| # activity_data = activity.model_dump_json(exclude_unset=True) | ||
| # await response.write(b"event: activity\n") | ||
| # await response.write(f"data: {activity_data}\n\n".encode("utf-8")) | ||
|
|
||
| # await response.write_eof() | ||
| # return response | ||
|
|
||
| # return handler | ||
|
|
||
|
|
||
| # def mock_mcs_endpoint( | ||
| # mocker, activities: Iterable[Activity], path: str, port: int | ||
| # ) -> AiohttpRunner: | ||
| # """Mock MCS responses for testing.""" | ||
|
|
||
| # PowerPlatformEnvironment.get_copilot_studio_connection_url = mocker.MagicMock( | ||
| # return_value=f"http://localhost:{port}{path}" | ||
| # ) | ||
|
|
||
| # app = Application() | ||
| # app.router.add_post(path, mock_mcs_handler(activities)) | ||
|
|
||
| # return AiohttpRunner(app, port=port) | ||
|
|
||
|
|
||
| # @pytest.mark.asyncio | ||
| # async def test_start_conversation_and_ask_question_large_message(mocker): | ||
|
|
||
| # activity = Activity( | ||
| # type="message", text="*" * 1_000_000, conversation={"id": "conv-id"} | ||
| # ) | ||
|
|
||
| # runner = mock_mcs_endpoint(mocker, [activity], "/mcs-endpoint", port=8081) | ||
|
|
||
| # async with runner: | ||
| # settings = ConnectionSettings("environment-id", "agent-id") | ||
| # client = CopilotClient(settings=settings, token="test-token") | ||
|
|
||
| # with pytest.raises(Exception, match="Chunk too big"): | ||
| # async for conv_activity in client.start_conversation(): | ||
| # assert conv_activity.type == "message" | ||
|
|
||
| # # with pytest.raises(Exception, match="Chunk too big"): | ||
| # # async for question_activity in client.ask_question("Hello!", "conv-id"): | ||
| # # assert question_activity.type == "message" | ||
|
|
||
|
|
||
| # def activity_generator(activity: Activity, n: int) -> Iterable[Activity]: | ||
| # for i in range(n): | ||
| # yield activity | ||
|
|
||
|
|
||
| # @pytest.mark.asyncio | ||
| # async def test_start_conversation_many(mocker): | ||
|
|
||
| # activity = Activity(type="message", conversation={"id": "conv-id"}) | ||
| # activities = activity_generator(activity, 100_000) | ||
|
|
||
| # runner = mock_mcs_endpoint(mocker, activities, "/mcs-endpoint", port=8081) | ||
|
|
||
| # async with runner: | ||
| # settings = ConnectionSettings("environment-id", "agent-id") | ||
| # client = CopilotClient(settings=settings, token="test-token") | ||
|
|
||
| # for i in range(100): | ||
| # # try: | ||
| # async for conv_activity in client.start_conversation(): | ||
| # assert conv_activity.type == "message" | ||
| # # except Exception as e: | ||
| # # assert str(e) == "Chunk too big" | ||
|
|
||
| # # with pytest.raises(Exception, match="Chunk too big"): | ||
| # # async for question_activity in client.ask_question("Hello!", "conv-id"): | ||
| # # assert question_activity.type == "message" |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire test file is commented out. Commented-out code should either be removed or uncommented if it's intended to be used.
| raise AttributeError(f"Cannot set item '{key}' on {type(self).__name__}") | ||
|
|
||
| def __delitem__(self, key): | ||
| raise AttributeError(f"Cannot delete item '{key}' on {type(self).__name__}") No newline at end of file |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method always raises AttributeError - should raise a LookupError (KeyError or IndexError) instead.
| raise AttributeError(f"Cannot set item '{key}' on {type(self).__name__}") | |
| def __delitem__(self, key): | |
| raise AttributeError(f"Cannot delete item '{key}' on {type(self).__name__}") | |
| raise KeyError(f"Cannot set item '{key}' on {type(self).__name__}") | |
| def __delitem__(self, key): | |
| raise KeyError(f"Cannot delete item '{key}' on {type(self).__name__}") |
|
|
||
| from .types.safe_object import ( | ||
| SafeObject, | ||
| resolve, |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'resolve' is not used.
| resolve, |
| from __future__ import annotations | ||
|
|
||
| from typing import Any | ||
| import inspect |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'inspect' is not used.
| import inspect |
|
|
||
| from typing import Any | ||
| import inspect | ||
| from pathlib import PurePath |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'PurePath' is not used.
| from pathlib import PurePath |
| from typing import Any, Callable | ||
|
|
||
| from microsoft_agents.activity import AgentsModel | ||
| from .types import SafeObject, DynamicObject |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'DynamicObject' is not used.
| from .types import SafeObject, DynamicObject | |
| from .types import SafeObject |
| @staticmethod | ||
| def check_verbose(actual: Any, baseline: Any) -> tuple[bool, str]: | ||
| actual = SafeObject(actual) | ||
| context = AssertionContext(actual, baseline) |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call to AssertionContext.init with too few arguments; should be no fewer than 4.
| context = AssertionContext(actual, baseline) | |
| context = AssertionContext(actual, baseline, None, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 94 out of 208 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
dev/microsoft-agents-testing/microsoft_agents/testing/integration/client/response_client.py:13
- Import of 'Entity' is not used.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raise AttributeError(f"Cannot set item '{key}' on {type(self).__name__}") | ||
|
|
||
| def __delitem__(self, key): | ||
| """Prevent deleting items on the readonly object.""" | ||
| raise AttributeError(f"Cannot delete item '{key}' on {type(self).__name__}") No newline at end of file |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method always raises AttributeError - should raise a LookupError (KeyError or IndexError) instead.
| raise AttributeError(f"Cannot set item '{key}' on {type(self).__name__}") | |
| def __delitem__(self, key): | |
| """Prevent deleting items on the readonly object.""" | |
| raise AttributeError(f"Cannot delete item '{key}' on {type(self).__name__}") | |
| raise KeyError(f"Cannot set item '{key}' on {type(self).__name__}") | |
| def __delitem__(self, key): | |
| """Prevent deleting items on the readonly object.""" | |
| raise KeyError(f"Cannot delete item '{key}' on {type(self).__name__}") |
| @@ -0,0 +1,477 @@ | |||
| import pytest | |||
| from unittest.mock import Mock | |||
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Mock' is not used.
| from unittest.mock import Mock | ||
|
|
||
| from microsoft_agents.testing.assertions.assertion_context import AssertionContext | ||
| from microsoft_agents.testing.assertions.types import SafeObject, DynamicObject, Unset |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Unset' is not used.
|
|
||
| from microsoft_agents.testing.assertions.assertion_context import AssertionContext | ||
| from microsoft_agents.testing.assertions.types import SafeObject, DynamicObject, Unset | ||
| from microsoft_agents.testing.assertions.types.safe_object import resolve, parent |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'parent' is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 94 out of 208 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
dev/microsoft-agents-testing/pytest.ini:1
- The warning filter value should be 'pytest.PytestUnraisableExceptionWarning' but this is not a standard pytest warning category. The correct category should be 'PytestUnraisableExceptionWarning' without the 'pytest.' prefix, or use the full module path.
dev/microsoft-agents-testing/microsoft_agents/testing/utils/misc.py:1 - The function uses the deprecated pdb.set_trace() method. Python 3.7+ includes a built-in breakpoint() function that should be used instead. Consider removing this wrapper function entirely and using breakpoint() directly in code.
dev/microsoft-agents-testing/microsoft_agents/testing/cli/cli_config.py:1 - The add_trailing_slash function is called in the setter but also in the initialization default value. Consider calling it only in the setter to avoid duplication and ensure consistency.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…into users/robrandao/integration-refinements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 95 out of 208 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
dev/tests/E2E/basic_agent/directline/SendActivity_SendsHi5_Returns5HiActivities.yaml:17
- Line 17 contains an invalid YAML syntax. The value should be
- type: skip(with proper indentation and "type:" key), not just|- skip. This will cause YAML parsing errors when the test is executed.
dev/microsoft-agents-testing/microsoft_agents/testing/integration/client/response_client.py:13 - Import of 'Entity' is not used.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def mock_mcs_handler(activity: Activity) -> Callable[[Request], Awaitable[Response]]: | ||
| """Creates a mock handler for MCS endpoint returning the given activity.""" | ||
|
|
||
| async def handler(request: Request) -> Response: | ||
| activity_data = activity.model_dump_json(exclude_unset=True) | ||
| return Response(body=activity_data) | ||
|
|
||
| return handler | ||
|
|
||
|
|
||
| def mock_mcs_handler( |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two functions with the same name mock_mcs_handler defined at lines 18 and 28. The second definition (lines 28-49) completely shadows the first one (lines 18-25), making the first function unreachable. This appears to be leftover code from refactoring. Remove the first function definition or rename one of them if both are needed.
| from typing import Any, Callable | ||
|
|
||
| from microsoft_agents.activity import AgentsModel | ||
| from .types import SafeObject, DynamicObject, resolve, parent |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'DynamicObject' is not used.
Import of 'parent' is not used.
| from .types import SafeObject, DynamicObject, resolve, parent | |
| from .types import SafeObject, resolve |
| @@ -0,0 +1,570 @@ | |||
| import pytest | |||
| from unittest.mock import Mock | |||
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Mock' is not used.
|
|
||
| from microsoft_agents.testing.assertions.assertions import Assertions | ||
| from microsoft_agents.testing.assertions.assertion_context import AssertionContext | ||
| from microsoft_agents.testing.assertions.types import SafeObject, DynamicObject, Unset |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'DynamicObject' is not used.
Import of 'Unset' is not used.
| @@ -0,0 +1,570 @@ | |||
| import pytest | |||
| from unittest.mock import Mock | |||
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Mock' is not used.
|
|
||
| from microsoft_agents.testing.assertions.assertions import Assertions | ||
| from microsoft_agents.testing.assertions.assertion_context import AssertionContext | ||
| from microsoft_agents.testing.assertions.types import SafeObject, DynamicObject, Unset |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'DynamicObject' is not used.
Import of 'Unset' is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 96 out of 209 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
dev/tests/E2E/basic_agent/directline/SendActivity_SendsHi5_Returns5HiActivities.yaml:17
- Typo in YAML - "- skip" should be "- type: skip" to match the expected YAML structure format.
dev/microsoft-agents-testing/microsoft_agents/testing/integration/client/response_client.py:13 - Import of 'Entity' is not used.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def __delitem__(self, key): | ||
| """Prevent deleting items on the readonly object.""" | ||
| raise AttributeError(f"Cannot delete item '{key}' on {type(self).__name__}") No newline at end of file |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method always raises AttributeError - should raise a LookupError (KeyError or IndexError) instead.
| raise AttributeError(f"Cannot delete item '{key}' on {type(self).__name__}") | |
| raise KeyError(f"Cannot delete item '{key}' on {type(self).__name__}") |
|
|
||
| from typing import Awaitable, Callable, Iterable | ||
|
|
||
| from aiohttp.web import Request, Response, Application, StreamResponse |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Response' is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 100 out of 214 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
dev/microsoft-agents-testing/microsoft_agents/testing/integration/client/response_client.py:13
- Import of 'Entity' is not used.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Prevent setting items on the readonly object.""" | ||
| raise AttributeError(f"Cannot set item '{key}' on {type(self).__name__}") | ||
|
|
||
| def __delitem__(self, key): |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method always raises AttributeError - should raise a LookupError (KeyError or IndexError) instead.
| @@ -0,0 +1,4 @@ | |||
| class DataDrivenTest | |||
|
|
|||
| def __init__(self, test_seq: list[DDTComponent]): | |||
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax Error (in Python 3).
|
|
||
| class Receive(DDTComponent): | ||
|
|
||
| def __init__(self, ) No newline at end of file |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax Error (in Python 3).
| from .types import ( | ||
| SafeObject, | ||
| resolve, | ||
| parent | ||
| ) |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'resolve' is not used.
| @@ -0,0 +1,73 @@ | |||
| from typing import Protocol, TypeVar, overload, Iterable, Callable | |||
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'Protocol' is not used.
No description provided.