Add URL and payload validation to conversation client tests#265
Add URL and payload validation to conversation client tests#265
Conversation
Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
heyitsaamir
left a comment
There was a problem hiding this comment.
@copilot some feedback.
Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR enhances conversation client tests by adding validation of HTTP request details (methods, URLs, query parameters, and JSON payloads) that were previously only checking for non-null responses. A new request_capture fixture is introduced to capture and inspect HTTP requests during testing.
Changes:
- Added
request_capturefixture in conftest.py to capture HTTP request details with mock transport - Enhanced 10 test methods to validate HTTP method, URL construction, query parameters, and request payloads
- Added new test for Bearer token authorization header validation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/api/tests/conftest.py | Added request_capture fixture with request capturing capabilities and mock response handling |
| packages/api/tests/unit/test_conversation_client.py | Updated 10 test methods to use request_capture fixture and validate HTTP request details; added token authorization test |
| from typing import Any | ||
|
|
||
| import httpx |
There was a problem hiding this comment.
Importing modules inside test functions is an anti-pattern. The typing.Any and httpx imports should be moved to the top of the file with the other imports, especially since httpx is already imported in conftest.py and used by the request_capture fixture.
| # Create request capture with the configured client | ||
| from typing import Any | ||
|
|
||
| import httpx | ||
|
|
||
| class RequestCapture: | ||
| def __init__(self): | ||
| self.requests: list[httpx.Request] = [] | ||
|
|
||
| def handler(self, request: httpx.Request) -> httpx.Response: | ||
| self.requests.append(request) | ||
| response_data: Any = { | ||
| "conversations": [{"id": "test", "conversationType": "personal", "isGroup": False}], | ||
| "continuationToken": "token", | ||
| } | ||
| return httpx.Response(status_code=200, json=response_data, headers={"content-type": "application/json"}) | ||
|
|
||
| @property | ||
| def last_request(self) -> httpx.Request | None: | ||
| return self.requests[-1] if self.requests else None | ||
|
|
||
| capture = RequestCapture() |
There was a problem hiding this comment.
This test has significant code duplication with the request_capture fixture defined in conftest.py. The RequestCapture class implementation (lines 92-106) duplicates the same class from the fixture. Instead of duplicating this code, the test should create the fixture with a custom Client that has the token configured, or extend the existing fixture to accept optional parameters.
| # Create request capture with the configured client | |
| from typing import Any | |
| import httpx | |
| class RequestCapture: | |
| def __init__(self): | |
| self.requests: list[httpx.Request] = [] | |
| def handler(self, request: httpx.Request) -> httpx.Response: | |
| self.requests.append(request) | |
| response_data: Any = { | |
| "conversations": [{"id": "test", "conversationType": "personal", "isGroup": False}], | |
| "continuationToken": "token", | |
| } | |
| return httpx.Response(status_code=200, json=response_data, headers={"content-type": "application/json"}) | |
| @property | |
| def last_request(self) -> httpx.Request | None: | |
| return self.requests[-1] if self.requests else None | |
| capture = RequestCapture() | |
| # Create request capture with the configured client using shared helper from conftest | |
| import httpx | |
| import conftest | |
| capture = conftest.RequestCapture() |
|
|
||
| capture = RequestCapture() | ||
| transport = httpx.MockTransport(capture.handler) | ||
| from microsoft_teams.common.http import Client |
There was a problem hiding this comment.
The import statement is placed after several lines of code in the test function. This violates PEP 8 style guidelines where imports should be at the top of the file. Move this import to the top of the file.
| assert str(last_request.url) == "https://test.service.url/v3/conversations" | ||
|
|
||
| # Validate request payload | ||
| payload = json.loads(last_request.content.decode("utf-8")) |
There was a problem hiding this comment.
The .decode("utf-8") call is unnecessary here. The json.loads() function can accept bytes directly in Python 3.6+, so this can be simplified to json.loads(last_request.content). This applies to all similar instances in this file (lines 156, 207, 234, 261).
| payload = json.loads(last_request.content.decode("utf-8")) | |
| payload = json.loads(last_request.content) |
| if "GetAadTokens" in str(request.url): | ||
| response_data = { | ||
| "https://graph.microsoft.com": { | ||
| "connectionName": "test_connection", | ||
| "token": "mock_graph_token_123", | ||
| "expiration": "2024-12-01T12:00:00Z", | ||
| }, | ||
| } | ||
| elif "/conversations/" in str(request.url) and str(request.url).endswith("/members"): | ||
| response_data = [ | ||
| { | ||
| "id": "mock_member_id", | ||
| "name": "Mock Member", | ||
| "objectId": "mock_object_id", | ||
| } | ||
| ] | ||
| elif "/conversations/" in str(request.url) and "/members/" in str(request.url) and request.method == "GET": | ||
| response_data = { | ||
| "id": "mock_member_id", | ||
| "name": "Mock Member", | ||
| "objectId": "mock_object_id", | ||
| } | ||
| elif "/conversations" in str(request.url) and request.method == "GET": | ||
| response_data = { | ||
| "conversations": [ | ||
| { | ||
| "id": "mock_conversation_id", | ||
| "conversationType": "personal", | ||
| "isGroup": True, | ||
| } | ||
| ], | ||
| "continuationToken": "mock_continuation_token", | ||
| } | ||
| elif "/conversations" in str(request.url) and request.method == "POST": | ||
| response_data = { | ||
| "id": "mock_conversation_id", | ||
| "type": "message", | ||
| "activityId": "mock_activity_id", | ||
| "serviceUrl": "https://mock.service.url", | ||
| } | ||
| elif "/activities" in str(request.url): | ||
| if request.method == "POST": | ||
| response_data = { | ||
| "id": "mock_activity_id", | ||
| "type": "message", | ||
| "text": "Mock activity response", | ||
| } | ||
| elif request.method == "PUT": | ||
| response_data = { | ||
| "id": "mock_activity_id", | ||
| "type": "message", | ||
| "text": "Updated mock activity", | ||
| } | ||
| elif request.method == "GET": | ||
| response_data = [ | ||
| { | ||
| "id": "mock_member_id", | ||
| "name": "Mock Member", | ||
| "objectId": "mock_object_id", | ||
| } | ||
| ] | ||
|
|
There was a problem hiding this comment.
The request_capture fixture duplicates response handling logic that already exists in the mock_transport fixture. Lines 244-304 in request_capture replicate the same endpoint matching and response generation as lines 46-198 in mock_transport. This duplication creates a maintenance burden where changes to mock responses must be made in two places. Consider refactoring to extract the response generation logic into a shared function that both fixtures can use, or make request_capture extend mock_transport instead of reimplementing it.
Conversation client tests only verified non-null responses without validating HTTP requests. Tests now capture and assert on request methods, URLs, query parameters, and JSON payloads.
Changes
Added
request_capturefixturelast_requestproperty for validationClientinstance compatible with existing APIsEnhanced test assertions
continuationToken)Added token authorization test
test_get_conversations_with_tokenvalidates Bearer tokens are properly sent in the Authorization headerExample
Before:
After:
Covers 10 methods across ConversationClient, ActivityOperations, and MemberOperations, plus token authorization.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.