feat(integration-tests): add OpenAI integration tests and configuration#252
feat(integration-tests): add OpenAI integration tests and configuration#252
Conversation
- Introduced integration tests for the Hello World demo, verifying the Charlie agent's response to both channel and direct messages. - Added configuration and fixtures for dynamic network setup and agent lifecycle management in `conftest.py`. - Updated `pytest.yml` to include necessary API keys for testing. - Created initial documentation for integration tests in `__init__.py`.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This pull request introduces comprehensive integration tests for the Hello World demo, verifying end-to-end functionality of the Charlie agent's message handling capabilities with real LLM interactions. The tests validate channel messaging, direct messaging, conversation flows, and self-message loop prevention.
Changes:
- Added integration test suite for Hello World demo with 5 test cases covering channel messages, direct messages, conversation flow, self-response prevention, and network connectivity
- Implemented shared test infrastructure with fixtures for network setup, agent lifecycle management, and client connections using dynamic port allocation
- Updated CI/CD pipeline to include OpenAI API key configuration for integration tests
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 28 comments.
| File | Description |
|---|---|
| tests/integration/test_hello_world_demo.py | Comprehensive test suite validating Charlie agent's responses to various message types with LLM integration |
| tests/integration/conftest.py | Shared fixtures providing network setup, dynamic port allocation, and agent/client lifecycle management |
| tests/integration/init.py | Package documentation describing integration test scope |
| .github/workflows/pytest.yml | Added OpenAI API key environment variables for integration test execution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Wait for second response | ||
| try: | ||
| await asyncio.wait_for(response_event.wait(), timeout=60) | ||
| except asyncio.TimeoutError: | ||
| pass |
There was a problem hiding this comment.
The response_event is not cleared before waiting for the second response, which could cause the test to immediately pass if Charlie sent multiple messages in response to the first question. Consider adding response_event.clear() after line 409 or restructuring to use a different synchronization mechanism for each expected response.
| first_response_payload = charlie_responses_1[0].payload | ||
| first_response = first_response_payload.get("text") or first_response_payload.get("content", {}).get("text", "") |
There was a problem hiding this comment.
If charlie_responses_1[0].payload is None, accessing .get() will raise an AttributeError. While the payload is checked for existence in earlier tests, this specific test doesn't validate that the payload exists before attempting to access it. Consider adding a null check: first_response_payload = charlie_responses_1[0].payload or {} to handle cases where payload might be None.
| second_response_payload = charlie_responses_2[-1].payload | ||
| second_response = second_response_payload.get("text") or second_response_payload.get("content", {}).get("text", "") |
There was a problem hiding this comment.
If charlie_responses_2[-1].payload is None, accessing .get() will raise an AttributeError. Consider adding a null check: second_response_payload = charlie_responses_2[-1].payload or {} to handle cases where payload might be None.
| except: | ||
| pass | ||
| except: |
There was a problem hiding this comment.
Bare except clauses should specify exception types. Using bare except: can catch system-critical exceptions like KeyboardInterrupt and SystemExit, which should typically be allowed to propagate. Consider catching specific exceptions like Exception or more targeted exceptions like aiohttp.ClientError and asyncio.TimeoutError.
| except: | |
| pass | |
| except: | |
| except (aiohttp.ClientError, asyncio.TimeoutError): | |
| pass | |
| except Exception: |
| max_retries = 10 | ||
| for attempt in range(max_retries): | ||
| try: | ||
| import aiohttp |
There was a problem hiding this comment.
The aiohttp module is imported inside the retry loop on each attempt. This import should be moved to the top of the file to follow Python conventions and avoid repeated import overhead during retries.
|
|
||
| import pytest | ||
| import asyncio | ||
| import os |
There was a problem hiding this comment.
Import of 'os' is not used.
| import os |
| import pytest | ||
| import asyncio | ||
| import os | ||
| from pathlib import Path |
There was a problem hiding this comment.
Import of 'Path' is not used.
| from pathlib import Path |
| from openagents.agents.runner import AgentRunner | ||
| from openagents.models.event import Event | ||
|
|
||
| from tests.integration.conftest import check_llm_api_key, skip_without_api_key |
There was a problem hiding this comment.
Import of 'check_llm_api_key' is not used.
| from tests.integration.conftest import check_llm_api_key, skip_without_api_key | |
| from tests.integration.conftest import skip_without_api_key |
| # Wait for second response | ||
| try: | ||
| await asyncio.wait_for(response_event.wait(), timeout=60) | ||
| except asyncio.TimeoutError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| env_file = Path(__file__).parent.parent.parent / ".env" | ||
| if env_file.exists(): | ||
| load_dotenv(env_file) | ||
| print(f"Loaded environment from {env_file}") |
There was a problem hiding this comment.
Print statement may execute during import.
conftest.py.pytest.ymlto include necessary API keys for testing.__init__.py.