isolate testing data between runs#1024
isolate testing data between runs#1024friedrichwilken wants to merge 2 commits intokyma-project:mainfrom
Conversation
Note(s) for PR Auther:
Note(s) for PR Reviewer(s):
|
SummaryThe following content is AI-generated and provides a summary of the pull request: Isolate Testing Data Between Runs🧪 Test: Improved test isolation to prevent data leaks between test runsThis pull request addresses test isolation issues by ensuring that shared state and data are properly reset between individual test executions. Changes
These changes ensure that each test runs in a clean environment, preventing potential test failures caused by shared state or data contamination from previous test runs. GitHub Issues
PR Bot InformationVersion:
💌 Have ideas or want to contribute? Create an issue and share your thoughts with us! Made with ❤️ by Hyperspace. |
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
The pull request changes test fixture scopes to improve isolation, but introduces critical issues:
-
Resource leaks: The
start_fake_redisfixture creates a new server thread per test without guaranteed cleanup, potentially leaving orphaned threads. -
Incomplete cleanup: The
clear_shared_memoryfixture clears data before tests but not after, allowing data to leak between tests. -
Performance concerns: Moving from session to function scope significantly increases test execution time by recreating expensive Redis servers and graph instances for every test.
These issues should be addressed before merging to ensure the isolation goals are achieved without introducing new problems.
PR Bot Information
Version: 1.17.69 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Correlation ID:
6e158300-1177-11f1-803b-200d8cced00f - LLM:
anthropic--claude-4.5-sonnet - Event Trigger:
pull_request.opened
| @pytest.fixture(autouse=True) | ||
| def clear_shared_memory(): | ||
| """Clear shared memory before each test to prevent data leaks.""" | ||
| shared_memory.clear() | ||
| yield |
There was a problem hiding this comment.
Bug: clear_shared_memory fixture runs before conversation_service fixture but doesn't clear data inserted during test execution
The clear_shared_memory fixture with autouse=True clears shared_memory before each test, but line 126 writes shared_memory[given_conversation_id] = given_latest_state during test execution. This data persists until the next test's setup phase, potentially causing data leaks between tests if they execute in parallel or if cleanup fails.
The fixture should use yield with cleanup after the test completes, or the test should explicitly clear its entry.
| @pytest.fixture(autouse=True) | |
| def clear_shared_memory(): | |
| """Clear shared memory before each test to prevent data leaks.""" | |
| shared_memory.clear() | |
| yield | |
| @pytest.fixture(autouse=True) | |
| def clear_shared_memory(): | |
| """Clear shared memory before each test to prevent data leaks.""" | |
| shared_memory.clear() | |
| yield | |
| shared_memory.clear() |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| @pytest.fixture(scope="function") | ||
| def conversation_service(app_models, companion_graph): |
There was a problem hiding this comment.
Logic Error: Fixture scope changed from session to function but conversation_service depends on expensive fixtures that are still session-scoped
The conversation_service fixture (line 46) now has scope="function" but depends on app_models and companion_graph. The app_models fixture remains scope="session" (conftest.py line 80), while companion_graph was changed to scope="function". This creates an inconsistent dependency chain where conversation_service is recreated for each test function but some of its dependencies aren't, which may not achieve the isolation goal and creates unnecessary overhead recreating the service wrapper.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
|
|
||
|
|
||
| @pytest.fixture(scope="session") | ||
| @pytest.fixture(scope="function") |
There was a problem hiding this comment.
Performance: Changing fixture scope from session to function causes expensive Redis server and graph setup on every test function
The start_fake_redis and companion_graph fixtures now create new instances for each test function instead of once per session. This dramatically increases test execution time since each test will:
- Find a free port
- Start a new Redis server in a thread
- Initialize AsyncRedisSaver
- Create a new CompanionGraph with all models
For a test suite with dozens of tests using these fixtures, this could increase runtime from seconds to minutes. Consider using scope="module" or scope="class" as a middle ground if full session isolation isn't needed.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
|
|
||
|
|
||
| @pytest.fixture(scope="session") | ||
| @pytest.fixture(scope="function") |
There was a problem hiding this comment.
Performance: Creating CompanionGraph per function is expensive due to model initialization overhead
The companion_graph fixture depends on app_models (session-scoped) but now recreates the CompanionGraph for every test function. The CompanionGraph likely performs significant initialization work with the models. Since app_models is already session-scoped and shared, consider whether this fixture truly needs function scope or if module/class scope would suffice for isolation.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
1 participant
Description
Changes proposed in this pull request:
Related issue(s)