test: add tests for platform metadata and manager#5360
test: add tests for platform metadata and manager#5360whatevertogo wants to merge 2 commits intoAstrBotDevs:masterfrom
Conversation
- Add platform metadata unit tests - Enhance platform base tests - Update platform manager tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
bot
added
the
size:XXL
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 focuses on bolstering the reliability and robustness of the core platform infrastructure by substantially increasing test coverage. It introduces new unit tests for platform metadata, enhances existing tests for the base platform class, and adds comprehensive validation for the platform manager's registration and discovery functionalities. These additions aim to ensure foundational components behave as expected, reducing potential bugs and improving overall system stability and maintainability. 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? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些整体性的反馈：
- 目前 PlatformManager 的 helper 测试依赖于启动一个 Python 子进程并对
case参数进行字符串化，这样的方式看起来既脆弱又偏慢；可以考虑重构当前的循环依赖导入链，这样这些逻辑就可以在同一进程内直接测试，而不需要通过_run_python/_assert_platform_manager_case这样的间接调用。 - 有一些断言依赖于精确匹配中文错误信息的子串（例如：
"已经注册过"、"未实现统一 Webhook 模式"）；如果这些错误信息将来被本地化或重写，测试就会失败，因此你可能更希望基于更稳定的信号进行断言（例如异常类型，或者更不依赖具体语言表述的片段）。
给 AI Agents 的提示词
Please address the comments from this code review:
## Overall Comments
- The PlatformManager helper tests relying on spawning a Python subprocess and stringifying the `case` argument look a bit brittle and slow; consider refactoring the circular import chain so these can be exercised directly in-process without `_run_python`/`_assert_platform_manager_case` indirection.
- Several assertions depend on exact Chinese error-message substrings (e.g., `"已经注册过"`, `"未实现统一 Webhook 模式"`); if these messages are ever localized or rephrased the tests will break, so you might want to assert on a more stable signal (e.g., exception type or a less language-specific fragment).
## Individual Comments
### Comment 1
<location> `tests/unit/test_platform_manager.py:332` </location>
<code_context>
+ # Manually set module path for testing
+ platform_registry[1].module_path = "plugins.other_plugin.adapter"
+
+ # Unregister by module prefix
+ unregistered = unregister_platform_adapters_by_module("plugins.test_plugin")
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting that `platform_registry` is also updated when unregistering by module prefix.
In `test_unregister_by_module_prefix`, you verify `platform_cls_map` after calling `unregister_platform_adapters_by_module`. To fully cover the behavior, also assert that `platform_registry` no longer contains metadata for the removed adapter (e.g., filter by `name`/`id` for `adapter_to_remove` and expect nothing). This confirms registry and class map stay in sync on removal.
Suggested implementation:
```python
# Manually set module path for testing
platform_registry[1].module_path = "plugins.other_plugin.adapter"
# Unregister by module prefix
unregistered = unregister_platform_adapters_by_module("plugins.test_plugin")
# Ensure the registry no longer contains metadata for the removed adapter
remaining_registry_entries = [
meta
for meta in platform_registry.values()
if getattr(meta, "name", None) == "adapter_to_remove"
]
assert remaining_registry_entries == []
```
If the metadata object stored in `platform_registry` uses a different attribute than `name` for the adapter identifier (e.g., `adapter_name` or `id`), update the `getattr(meta, "name", None)` call to use the correct attribute so that the filter correctly targets `adapter_to_remove`.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进以后的评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The PlatformManager helper tests relying on spawning a Python subprocess and stringifying the
caseargument look a bit brittle and slow; consider refactoring the circular import chain so these can be exercised directly in-process without_run_python/_assert_platform_manager_caseindirection. - Several assertions depend on exact Chinese error-message substrings (e.g.,
"已经注册过","未实现统一 Webhook 模式"); if these messages are ever localized or rephrased the tests will break, so you might want to assert on a more stable signal (e.g., exception type or a less language-specific fragment).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The PlatformManager helper tests relying on spawning a Python subprocess and stringifying the `case` argument look a bit brittle and slow; consider refactoring the circular import chain so these can be exercised directly in-process without `_run_python`/`_assert_platform_manager_case` indirection.
- Several assertions depend on exact Chinese error-message substrings (e.g., `"已经注册过"`, `"未实现统一 Webhook 模式"`); if these messages are ever localized or rephrased the tests will break, so you might want to assert on a more stable signal (e.g., exception type or a less language-specific fragment).
## Individual Comments
### Comment 1
<location> `tests/unit/test_platform_manager.py:332` </location>
<code_context>
+ # Manually set module path for testing
+ platform_registry[1].module_path = "plugins.other_plugin.adapter"
+
+ # Unregister by module prefix
+ unregistered = unregister_platform_adapters_by_module("plugins.test_plugin")
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting that `platform_registry` is also updated when unregistering by module prefix.
In `test_unregister_by_module_prefix`, you verify `platform_cls_map` after calling `unregister_platform_adapters_by_module`. To fully cover the behavior, also assert that `platform_registry` no longer contains metadata for the removed adapter (e.g., filter by `name`/`id` for `adapter_to_remove` and expect nothing). This confirms registry and class map stay in sync on removal.
Suggested implementation:
```python
# Manually set module path for testing
platform_registry[1].module_path = "plugins.other_plugin.adapter"
# Unregister by module prefix
unregistered = unregister_platform_adapters_by_module("plugins.test_plugin")
# Ensure the registry no longer contains metadata for the removed adapter
remaining_registry_entries = [
meta
for meta in platform_registry.values()
if getattr(meta, "name", None) == "adapter_to_remove"
]
assert remaining_registry_entries == []
```
If the metadata object stored in `platform_registry` uses a different attribute than `name` for the adapter identifier (e.g., `adapter_name` or `id`), update the `getattr(meta, "name", None)` call to use the correct attribute so that the filter correctly targets `adapter_to_remove`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # Manually set module path for testing | ||
| platform_registry[1].module_path = "plugins.other_plugin.adapter" | ||
|
|
||
| # Unregister by module prefix |
There was a problem hiding this comment.
suggestion (testing): Consider asserting that platform_registry is also updated when unregistering by module prefix.
在 test_unregister_by_module_prefix 中，你在调用 unregister_platform_adapters_by_module 之后验证了 platform_cls_map。为了更全面地覆盖该行为，还可以断言 platform_registry 中不再包含被移除适配器的元数据（例如根据 adapter_to_remove 的 name / id 进行过滤，并期望结果为空）。这样可以确认在移除时注册表和类映射保持同步。
建议实现如下：
# Manually set module path for testing
platform_registry[1].module_path = "plugins.other_plugin.adapter"
# Unregister by module prefix
unregistered = unregister_platform_adapters_by_module("plugins.test_plugin")
# Ensure the registry no longer contains metadata for the removed adapter
remaining_registry_entries = [
meta
for meta in platform_registry.values()
if getattr(meta, "name", None) == "adapter_to_remove"
]
assert remaining_registry_entries == []如果存储在 platform_registry 中的元数据对象使用的适配器标识字段不是 name（例如 adapter_name 或 id），请将 getattr(meta, "name", None) 调整为正确的属性名称，以便筛选逻辑能准确匹配 adapter_to_remove。
Original comment in English
suggestion (testing): Consider asserting that platform_registry is also updated when unregistering by module prefix.
In test_unregister_by_module_prefix, you verify platform_cls_map after calling unregister_platform_adapters_by_module. To fully cover the behavior, also assert that platform_registry no longer contains metadata for the removed adapter (e.g., filter by name/id for adapter_to_remove and expect nothing). This confirms registry and class map stay in sync on removal.
Suggested implementation:
# Manually set module path for testing
platform_registry[1].module_path = "plugins.other_plugin.adapter"
# Unregister by module prefix
unregistered = unregister_platform_adapters_by_module("plugins.test_plugin")
# Ensure the registry no longer contains metadata for the removed adapter
remaining_registry_entries = [
meta
for meta in platform_registry.values()
if getattr(meta, "name", None) == "adapter_to_remove"
]
assert remaining_registry_entries == []If the metadata object stored in platform_registry uses a different attribute than name for the adapter identifier (e.g., adapter_name or id), update the getattr(meta, "name", None) call to use the correct attribute so that the filter correctly targets adapter_to_remove.
dosubot
bot
added
the
area:platform
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the test coverage for the platform infrastructure by adding comprehensive unit tests for PlatformMetadata, Platform base class, and PlatformManager. The tests are well-structured and cover a good range of functionality. My feedback focuses on a few areas to further enhance test quality and code clarity, such as strengthening an assertion in one test, removing a misleading comment, and consolidating a couple of redundant tests.
| with patch( | ||
| "astrbot.core.platform.platform.Metric.upload", new_callable=AsyncMock | ||
| ): | ||
| # Should not raise any exception | ||
| await platform.send_by_session(mock_session, mock_message_chain) |
There was a problem hiding this comment.
The test currently only checks that send_by_session doesn't raise an exception. To make the test more robust, you should also assert that the Metric.upload method was called with the expected arguments. This ensures the default implementation's behavior is correctly verified.
| with patch( | |
| "astrbot.core.platform.platform.Metric.upload", new_callable=AsyncMock | |
| ): | |
| # Should not raise any exception | |
| await platform.send_by_session(mock_session, mock_message_chain) | |
| with patch( | |
| "astrbot.core.platform.platform.Metric.upload", new_callable=AsyncMock | |
| ) as mock_upload: | |
| await platform.send_by_session(mock_session, mock_message_chain) | |
| mock_upload.assert_awaited_once_with( | |
| msg_event_tick=1, adapter_name="test_platform" | |
| ) |
| # NOTE: The following tests are skipped due to circular import issues | ||
| # when importing PlatformManager from astrbot.core.platform.manager. | ||
| # This is a known issue that should be addressed in the future. | ||
| # The circular import chain is: | ||
| # manager.py -> star_handler -> star_tools -> api.platform -> star.register -> star_handler -> astr_agent_context -> context -> manager | ||
| # | ||
| # Skipping these tests as per task requirements to only record issues, not fix them. |
There was a problem hiding this comment.
This comment block is misleading. The tests for PlatformManager are not actually skipped; they are implemented using a subprocess workaround (_assert_platform_manager_case) to avoid the circular import issue. Please remove this comment block as it does not reflect the current state of the code.
| def test_platform_metadata_creation_basic(self): | ||
| """Test creating PlatformMetadata with required fields.""" | ||
| meta = PlatformMetadata( | ||
| name="test_platform", | ||
| description="A test platform", | ||
| id="test_platform_id", | ||
| ) | ||
|
|
||
| assert meta.name == "test_platform" | ||
| assert meta.description == "A test platform" | ||
| assert meta.id == "test_platform_id" | ||
|
|
||
| def test_platform_metadata_default_values(self): | ||
| """Test PlatformMetadata default values.""" | ||
| meta = PlatformMetadata( | ||
| name="test_platform", | ||
| description="A test platform", | ||
| id="test_platform_id", | ||
| ) | ||
|
|
||
| # Default values | ||
| assert meta.default_config_tmpl is None | ||
| assert meta.adapter_display_name is None | ||
| assert meta.logo_path is None | ||
| assert meta.support_streaming_message is True | ||
| assert meta.support_proactive_message is True | ||
| assert meta.module_path is None | ||
| assert meta.i18n_resources is None | ||
| assert meta.config_metadata is None |
There was a problem hiding this comment.
The tests test_platform_metadata_creation_basic and test_platform_metadata_default_values both instantiate PlatformMetadata with the same arguments to test different aspects. You can merge them into a single test to improve conciseness and avoid redundant object creation.
def test_platform_metadata_creation_and_defaults(self):
"""Test creating PlatformMetadata with required fields and checking defaults."""
meta = PlatformMetadata(
name="test_platform",
description="A test platform",
id="test_platform_id",
)
assert meta.name == "test_platform"
assert meta.description == "A test platform"
assert meta.id == "test_platform_id"
# Default values
assert meta.default_config_tmpl is None
assert meta.adapter_display_name is None
assert meta.logo_path is None
assert meta.support_streaming_message is True
assert meta.support_proactive_message is True
assert meta.module_path is None
assert meta.i18n_resources is None
assert meta.config_metadata is NoneThere was a problem hiding this comment.
Pull request overview
Adds unit tests around the platform infrastructure to improve confidence in metadata, base Platform behavior, and registration/manager helpers.
Changes:
- Added
PlatformMetadatadataclass tests. - Added unit tests for
Platformbase class behaviors (status/errors/stats/webhook helpers). - Added tests for platform adapter registration/unregistration and some
PlatformManagerhelper methods (via subprocess isolation).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/unit/test_platform_metadata.py |
Verifies PlatformMetadata construction and default/optional fields. |
tests/unit/test_platform_base.py |
Covers Platform base behaviors: status lifecycle, error recording/clearing, stats, webhook helpers, and default method behavior. |
tests/unit/test_platform_manager.py |
Tests platform adapter registration/unregistration globals and exercises PlatformManager helper/stats behaviors using subprocess isolation. |
| def _run_python(code: str) -> subprocess.CompletedProcess[str]: | ||
| repo_root = Path(__file__).resolve().parents[2] | ||
| return subprocess.run( | ||
| [sys.executable, "-c", textwrap.dedent(code)], | ||
| cwd=repo_root, | ||
| capture_output=True, | ||
| text=True, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
The PlatformManager tests run via subprocess, but this introduces significant overhead (spawning a new interpreter per case) and makes failures harder to debug. Consider importing PlatformManager directly in-process and isolating side effects with monkeypatch/fixtures (or, if subprocess isolation is required, consolidate multiple cases into a single subprocess run or mark these as integration tests to keep unit test runtime low).
| # NOTE: The following tests are skipped due to circular import issues | ||
| # when importing PlatformManager from astrbot.core.platform.manager. | ||
| # This is a known issue that should be addressed in the future. | ||
| # The circular import chain is: | ||
| # manager.py -> star_handler -> star_tools -> api.platform -> star.register -> star_handler -> astr_agent_context -> context -> manager | ||
| # | ||
| # Skipping these tests as per task requirements to only record issues, not fix them. |
There was a problem hiding this comment.
This comment block says the following tests are "skipped due to circular import issues", but the tests below are not actually skipped—they run via _assert_platform_manager_case(...). Please update the comment to reflect the current approach (subprocess-based isolation), or add an explicit pytest.mark.skip/xfail if they truly should be skipped.
| # NOTE: The following tests are skipped due to circular import issues | |
| # when importing PlatformManager from astrbot.core.platform.manager. | |
| # This is a known issue that should be addressed in the future. | |
| # The circular import chain is: | |
| # manager.py -> star_handler -> star_tools -> api.platform -> star.register -> star_handler -> astr_agent_context -> context -> manager | |
| # | |
| # Skipping these tests as per task requirements to only record issues, not fix them. | |
| # NOTE: The following tests previously ran into circular import issues | |
| # when importing PlatformManager directly from astrbot.core.platform.manager. | |
| # To avoid this, they exercise PlatformManager behavior in a separate | |
| # subprocess via `_assert_platform_manager_case(...)`, which imports | |
| # PlatformManager in isolation and prevents circular imports in this process. | |
| # The historical circular import chain was: | |
| # manager.py -> star_handler -> star_tools -> api.platform -> star.register -> star_handler -> astr_agent_context -> context -> manager |
Add tests for platform metadata and enhance platform base/manager test coverage to improve reliability of platform infrastructure.
Modifications / 改动点
tests/unit/test_platform_metadata.pyfor metadata structure teststests/unit/test_platform_base.pytests/unit/test_platform_manager.pyTest Coverage:
Platform metadata validation
Platform base class functionality
Platform manager registration and discovery
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
为平台元数据、Platform 基类以及平台管理器/注册表添加单元测试以提升覆盖率。
Tests:
PlatformMetadata引入全面测试，覆盖默认值、可选字段以及配置/i18n 元数据。PlatformManager辅助方法，例如 ID 校验、统计聚合以及实例获取。Original summary in English
Summary by Sourcery
Add unit tests to improve coverage for platform metadata, the Platform base class, and the platform manager/registry.
Tests: