test: enhance tests for conversation and persona management#5361
Closed
whatevertogo wants to merge 6 commits intoAstrBotDevs:masterfrom
Closed
test: enhance tests for conversation and persona management#5361whatevertogo wants to merge 6 commits intoAstrBotDevs:masterfrom
whatevertogo wants to merge 6 commits intoAstrBotDevs:masterfrom
Conversation
- Expand conversation manager tests - Update persona manager tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
bot
added
the
size:XXL
Contributor
Summary of ChangesHello @whatevertogo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the test coverage for the conversation and persona management modules. The primary goal is to improve the reliability and stability of these core functionalities by adding extensive unit tests. These new tests validate various operations, including concurrent access patterns and dynamic updates, ensuring that conversation history and persona configurations behave as expected under diverse conditions. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
dosubot
bot
added
area:core
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些高层次的反馈：
- 在
test_conversation_mgr.py中，有很多测试会手动构造几乎相同的ConversationV2mock（每次都设置相同的属性）；建议抽取一个共享的 fixture 或辅助工厂方法来构建这些 mocks，这样可以让测试保持 DRY，并且在模型变更时更容易统一更新。 - 一些并发测试（例如
test_concurrent_add_message_pair、test_concurrent_read_conversations、test_concurrent_create_conversations）会启动数量相对较多、行为非常相似的任务；你可以考虑减少任务数量，或者用参数化的方式覆盖更小的一组用例，在仍然覆盖并发行为的前提下，更好地控制测试运行时间和潜在的不稳定性。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_conversation_mgr.py` there are many tests that manually construct almost identical `ConversationV2` mocks (setting the same attributes each time); consider extracting a shared fixture or helper factory to build these mocks to keep the tests DRY and easier to update if the model changes.
- Several of the concurrent tests (e.g. `test_concurrent_add_message_pair`, `test_concurrent_read_conversations`, `test_concurrent_create_conversations`) spin up relatively large numbers of tasks with very similar behavior; you might reduce the task counts or parameterize a smaller set of cases to keep test runtime and potential flakiness under better control while still exercising concurrency.
## Individual Comments
### Comment 1
<location> `tests/unit/test_conversation_mgr.py:22` </location>
<code_context>
+@pytest.fixture
+def mock_db():
</code_context>
<issue_to_address>
**suggestion (testing):** 为 get_conversations 和 get_filtered_conversations 的行为添加明确的测试
该 fixture 暴露了 `get_conversations` 和 `get_filtered_conversations`，但目前没有专门的单元测试来验证：
- `ConversationV2` 到 `ConversationManager.get_conversations()` 返回的公共结构的精确映射（顺序、字段名、时间戳）。
- `ConversationManager.get_filtered_conversations()` 中的分页、过滤条件，以及 `(items, total)` 的处理。
请添加一些小而确定性的测试，用 1–2 个 `ConversationV2` 实例来 stub 这些 DB 方法，并断言精确的返回结构，以及过滤条件和 `total` 是否被正确传递。
建议实现如下：
```python
db.get_filtered_conversations = AsyncMock(return_value=([], 0))
return db
@pytest.mark.asyncio
async def test_get_conversations_maps_conversation_v2_structure(mock_db):
"""Ensure ConversationManager.get_conversations maps ConversationV2 to the expected public structure."""
# Arrange
# NOTE: adjust fields here to match ConversationV2 and ConversationManager implementation.
conv = ConversationV2(
id="conv1",
user_id="user1",
title="First conversation",
created_at=1234567890,
updated_at=1234567999,
)
mock_db.get_conversations.return_value = [conv]
manager = ConversationManager(db=mock_db)
# Act
result = await manager.get_conversations(user_id="user1")
# Assert
# Assert ordering and structure of the public response; update keys/values as needed.
assert isinstance(result, list)
assert len(result) == 1
conv_public = result[0]
# These keys should match the public API of ConversationManager.get_conversations
assert conv_public["id"] == "conv1"
assert conv_public["user_id"] == "user1"
assert conv_public["title"] == "First conversation"
assert conv_public["created_at"] == 1234567890
assert conv_public["updated_at"] == 1234567999
mock_db.get_conversations.assert_awaited_once_with(user_id="user1")
@pytest.mark.asyncio
async def test_get_filtered_conversations_pagination_filters_and_total(mock_db):
"""Ensure get_filtered_conversations forwards filters/pagination and maps items & total correctly."""
# Arrange
conv1 = ConversationV2(
id="conv1",
user_id="user1",
title="First conversation",
created_at=1,
updated_at=2,
)
conv2 = ConversationV2(
id="conv2",
user_id="user1",
title="Second conversation",
created_at=3,
updated_at=4,
)
db_items = [conv1, conv2]
db_total = 10
filters = {"archived": False, "search": "First"}
limit = 2
offset = 0
mock_db.get_filtered_conversations.return_value = (db_items, db_total)
manager = ConversationManager(db=mock_db)
# Act
items, total = await manager.get_filtered_conversations(
user_id="user1",
filters=filters,
limit=limit,
offset=offset,
)
# Assert: underlying DB called with the same filters/pagination
mock_db.get_filtered_conversations.assert_awaited_once_with(
user_id="user1",
filters=filters,
limit=limit,
offset=offset,
)
# Assert: total is passed through
assert total == db_total
# Assert: items length, ordering, and structure are preserved/mapped correctly
assert isinstance(items, list)
assert [item["id"] for item in items] == ["conv1", "conv2"]
assert items[0]["title"] == "First conversation"
assert items[1]["title"] == "Second conversation"
assert items[0]["created_at"] == 1
assert items[1]["created_at"] == 3
```
这些测试基于以下假设：
1. `ConversationV2` 的 `__init__` 与使用到的关键字参数兼容（`id`、`user_id`、`title`、`created_at`、`updated_at`）。如果实际模型不同，请相应调整构造函数参数。
2. `ConversationManager.get_conversations` 返回 `List[Dict]`，其中每个 dict 都包含 `id`、`user_id`、`title`、`created_at` 和 `updated_at` 这些键。如果公共结构使用了不同的字段名，或包含额外字段，请更新断言以匹配真实的映射逻辑。
3. `ConversationManager.get_filtered_conversations` 的签名类似于：
`async def get_filtered_conversations(self, user_id: str, filters: dict, limit: int, offset: int) -> Tuple[List[dict], int]`
并调用 `db.get_filtered_conversations(user_id=user_id, filters=filters, limit=limit, offset=offset)`。如果签名或关键字参数名不同，请相应更新调用和 `assert_awaited_once_with`。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据反馈改进之后的 Review。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
test_conversation_mgr.pythere are many tests that manually construct almost identicalConversationV2mocks (setting the same attributes each time); consider extracting a shared fixture or helper factory to build these mocks to keep the tests DRY and easier to update if the model changes. - Several of the concurrent tests (e.g.
test_concurrent_add_message_pair,test_concurrent_read_conversations,test_concurrent_create_conversations) spin up relatively large numbers of tasks with very similar behavior; you might reduce the task counts or parameterize a smaller set of cases to keep test runtime and potential flakiness under better control while still exercising concurrency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_conversation_mgr.py` there are many tests that manually construct almost identical `ConversationV2` mocks (setting the same attributes each time); consider extracting a shared fixture or helper factory to build these mocks to keep the tests DRY and easier to update if the model changes.
- Several of the concurrent tests (e.g. `test_concurrent_add_message_pair`, `test_concurrent_read_conversations`, `test_concurrent_create_conversations`) spin up relatively large numbers of tasks with very similar behavior; you might reduce the task counts or parameterize a smaller set of cases to keep test runtime and potential flakiness under better control while still exercising concurrency.
## Individual Comments
### Comment 1
<location> `tests/unit/test_conversation_mgr.py:22` </location>
<code_context>
+@pytest.fixture
+def mock_db():
</code_context>
<issue_to_address>
**suggestion (testing):** Add explicit tests for get_conversations and get_filtered_conversations behavior
The fixture exposes `get_conversations` and `get_filtered_conversations`, but there are no focused unit tests that validate:
- The exact mapping from `ConversationV2` to the public structure in `ConversationManager.get_conversations()` (ordering, field names, timestamps).
- Pagination, filters, and `(items, total)` handling in `ConversationManager.get_filtered_conversations()`.
Please add small, deterministic tests that stub these DB methods with 1–2 `ConversationV2` instances and assert the precise returned structure and that filters and `total` are passed through correctly.
Suggested implementation:
```python
db.get_filtered_conversations = AsyncMock(return_value=([], 0))
return db
@pytest.mark.asyncio
async def test_get_conversations_maps_conversation_v2_structure(mock_db):
"""Ensure ConversationManager.get_conversations maps ConversationV2 to the expected public structure."""
# Arrange
# NOTE: adjust fields here to match ConversationV2 and ConversationManager implementation.
conv = ConversationV2(
id="conv1",
user_id="user1",
title="First conversation",
created_at=1234567890,
updated_at=1234567999,
)
mock_db.get_conversations.return_value = [conv]
manager = ConversationManager(db=mock_db)
# Act
result = await manager.get_conversations(user_id="user1")
# Assert
# Assert ordering and structure of the public response; update keys/values as needed.
assert isinstance(result, list)
assert len(result) == 1
conv_public = result[0]
# These keys should match the public API of ConversationManager.get_conversations
assert conv_public["id"] == "conv1"
assert conv_public["user_id"] == "user1"
assert conv_public["title"] == "First conversation"
assert conv_public["created_at"] == 1234567890
assert conv_public["updated_at"] == 1234567999
mock_db.get_conversations.assert_awaited_once_with(user_id="user1")
@pytest.mark.asyncio
async def test_get_filtered_conversations_pagination_filters_and_total(mock_db):
"""Ensure get_filtered_conversations forwards filters/pagination and maps items & total correctly."""
# Arrange
conv1 = ConversationV2(
id="conv1",
user_id="user1",
title="First conversation",
created_at=1,
updated_at=2,
)
conv2 = ConversationV2(
id="conv2",
user_id="user1",
title="Second conversation",
created_at=3,
updated_at=4,
)
db_items = [conv1, conv2]
db_total = 10
filters = {"archived": False, "search": "First"}
limit = 2
offset = 0
mock_db.get_filtered_conversations.return_value = (db_items, db_total)
manager = ConversationManager(db=mock_db)
# Act
items, total = await manager.get_filtered_conversations(
user_id="user1",
filters=filters,
limit=limit,
offset=offset,
)
# Assert: underlying DB called with the same filters/pagination
mock_db.get_filtered_conversations.assert_awaited_once_with(
user_id="user1",
filters=filters,
limit=limit,
offset=offset,
)
# Assert: total is passed through
assert total == db_total
# Assert: items length, ordering, and structure are preserved/mapped correctly
assert isinstance(items, list)
assert [item["id"] for item in items] == ["conv1", "conv2"]
assert items[0]["title"] == "First conversation"
assert items[1]["title"] == "Second conversation"
assert items[0]["created_at"] == 1
assert items[1]["created_at"] == 3
```
These tests assume:
1. `ConversationV2` has an `__init__` compatible with the keyword arguments used (`id`, `user_id`, `title`, `created_at`, `updated_at`). If the actual model differs, adjust the constructor arguments accordingly.
2. `ConversationManager.get_conversations` returns a `List[Dict]` where each dict has keys `id`, `user_id`, `title`, `created_at`, and `updated_at`. If the public structure uses different field names or includes additional fields, update the assertions to match the real mapping logic.
3. `ConversationManager.get_filtered_conversations` has a signature like:
`async def get_filtered_conversations(self, user_id: str, filters: dict, limit: int, offset: int) -> Tuple[List[dict], int]`
and calls `db.get_filtered_conversations(user_id=user_id, filters=filters, limit=limit, offset=offset)`. If the signature or keyword names differ, update the call and `assert_awaited_once_with` accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
The pull request introduces comprehensive unit tests for ConversationManager and PersonaManager, significantly enhancing test coverage for conversation and persona management functionalities. The tests cover initialization, CRUD operations, session management, folder management, and concurrent access scenarios, which is a great improvement for the reliability and robustness of these modules. The addition of hot reload tests for persona management is also a valuable inclusion.
Contributor
There was a problem hiding this comment.
Pull request overview
Expands unit test coverage for AstrBot’s conversation and persona management layers to improve confidence in conversation lifecycle behavior and persona/folder operations.
Changes:
- Adds comprehensive unit tests for
ConversationManagercovering creation, switching, deletion, updates, history mutations, and concurrent access patterns. - Adds extensive unit tests for
PersonaManagercovering persona CRUD, folder CRUD/tree building, sort-order batch updates, and v3 persona data generation/hot-reload scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/unit/test_conversation_mgr.py |
Adds unit tests for ConversationManager behaviors including session selection storage and concurrent operations. |
tests/unit/test_persona_mgr.py |
Adds unit tests for PersonaManager behaviors including persona/folder management and v3 persona conversion logic. |
Contributor
Author
|
done |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Expand test coverage for conversation and persona management modules to improve reliability of conversation history and persona functionality.
Modifications / 改动点
tests/unit/test_conversation_mgr.pywith additional test casestests/unit/test_persona_mgr.pyTest Coverage:
Conversation creation, retrieval, and deletion
Conversation session management
Persona creation, update, and deletion
Persona assignment and validation
This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Summary by Sourcery
扩展对会话和人物配置管理的单元测试覆盖范围，以验证核心行为、配置处理，以及在并发访问下的健壮性。
Tests:
Original summary in English
Summary by Sourcery
Expand unit test coverage for conversation and persona management to validate core behaviors, configuration handling, and robustness under concurrent access.
Tests: