Skip to content

Conversation

@RC-CHN
Copy link
Member

@RC-CHN RC-CHN commented Jan 22, 2026

统一了原来的测试脚本,使用pytest进行规范化测试

Summary by Sourcery

将 Bay 的测试标准化为基于 pytest 的结构化单元测试、集成测试和端到端(e2e)测试套件,并改进 ship 会话处理。

New Features:

  • 为 Bay 添加基于 pytest 的端到端 API 测试,实现每个测试使用隔离容器,并覆盖会话重用/持久化场景。
  • 为内存和磁盘工具函数以及 ShipSpec 磁盘配置引入单元测试。
  • 提供测试脚本,用于在 Docker(宿主机/容器)和 Podman(宿主机/容器)环境中运行端到端测试。
  • 添加 Bay 测试所需的 pytest 配置、fixtures 和 markers,并文档化统一的测试目录结构和使用方式。

Bug Fixes:

  • 修复 ship 会话计数更新逻辑,在创建或重用 ship 时使用已自增的 ship 状态进行更新。

Enhancements:

  • 记录 ship exec 成功响应日志,以改进调试能力和可观测性。
  • 将 Kubernetes 测试清单与脚本重组至 tests/k8s 下,并更新 k8s 测试脚本以运行基于 pytest 的端到端测试。
Original summary in English

Summary by Sourcery

Standardize Bay testing around pytest with structured unit, integration, e2e suites and improve ship session handling.

New Features:

  • Add pytest-based E2E API tests for Bay with per-test isolated containers and session reuse/persistence coverage.
  • Introduce unit tests for memory and disk utility functions and ShipSpec disk configuration.
  • Provide test scripts for running E2E tests across Docker (host/container) and Podman (host/container) environments.
  • Add pytest configuration, fixtures, and markers for Bay tests and document the unified tests layout and usage.

Bug Fixes:

  • Fix ship session count updates to use the incremented ship state when creating or reusing ships.

Enhancements:

  • Log ship exec success responses for better debugging and observability.
  • Reorganize Kubernetes test manifests and scripts under tests/k8s and point k8s test script to run pytest-based E2E tests.

…ture

Reorganize Bay test suite with proper pytest conventions:

- Add `fresh_ship()` context manager for automatic container cleanup
- Convert test_bay_api.py to use isolated containers per test
- Create pytest-based e2e tests in tests/e2e/test_bay_api.py
- Add unit tests for memory and disk utilities in tests/unit/
- Add test scripts for Docker, Podman (host/container modes)
- Add Kubernetes test deployment configs and test script
- Organize tests into unit, e2e, and integration categories
- Add conftest.py with common fixtures and pytest markers
Add logging for ship exec responses and extend e2e tests to
cover filesystem, ipython execution, and multi-session reuse.
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - 我发现了两个问题,并给出了一些整体性的反馈:

  • BAY_URL/ACCESS_TOKEN/get_auth_headers 配置现在同时定义在 conftest.py 和 tests/e2e/test_bay_api.py 中;建议将这些配置集中放在 conftest 中,以避免重复和配置偏差。
  • forward_request_to_ship 中新增的 logger.info 调用会记录完整的 exec 响应,这里面可能包含很大的负载或敏感数据;建议降低日志详细程度(例如只记录类型/摘要),或者把它放到 debug 日志级别之下。
  • tests/scripts/ 目录下的测试脚本对代理设置和镜像名称进行了硬编码(例如 HTTP_PROXY、HTTPS_PROXY、shipyard-ship 标签);建议通过环境变量进行配置,这样可以更方便地在不同环境中复用这些脚本。
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The BAY_URL/ACCESS_TOKEN/get_auth_headers configuration is now defined both in conftest.py and in tests/e2e/test_bay_api.py; consider centralizing this in conftest to avoid duplication and configuration drift.
- The new logger.info call in forward_request_to_ship logs full exec responses, which may include large payloads or sensitive data; consider reducing the verbosity (e.g., log only type/summary) or gating it behind a debug log level.
- The test scripts under tests/scripts/ hard-code proxy settings and image names (e.g., HTTP_PROXY, HTTPS_PROXY, shipyard-ship tags); consider making these configurable via environment variables to make the scripts easier to reuse in different environments.

## Individual Comments

### Comment 1
<location> `pkgs/bay/app/services/ship/http_client.py:87` </location>
<code_context>
             ) as response:
                 if response.status == 200:
                     data = await response.json()
+                    logger.info(f"Ship exec response for {request.type}: {data}")
                     return ExecResponse(success=True, data=data)
                 else:
</code_context>

<issue_to_address>
**🚨 suggestion (security):** Avoid logging full exec response payloads at info level to reduce noise and potential data exposure.

Given the potential size and sensitivity of exec outputs, please avoid logging the full `data` object here. Instead, consider moving this to `debug`, logging only a small, redacted/summary subset (e.g., status, a few safe fields, length), or gating full-body logging behind a config/feature flag so production logs stay minimal and safe.
</issue_to_address>

### Comment 2
<location> `pkgs/bay/tests/unit/test_utils.py:10-19` </location>
<code_context>
+class TestMemoryUtils:
</code_context>

<issue_to_address>
**suggestion (testing):** Add unit tests for invalid/unsupported memory formats to validate error handling paths.

Current tests only cover valid units and minimum-enforcement behavior. Please also add cases for invalid inputs (e.g. "abc", "10XZ", negative values, None) to assert the expected failure mode (e.g. specific exception type or fallback value). This helps ensure misconfigurations are caught rather than silently accepted.
</issue_to_address>

Sourcery 对开源项目是免费的——如果你觉得我们的评审有帮助,欢迎分享 ✨
帮我变得更有用!请在每条评论上点击 👍 或 👎,我会根据你的反馈改进后续评审。
Original comment in English

Hey - I've found 2 issues, and left some high level feedback:

  • The BAY_URL/ACCESS_TOKEN/get_auth_headers configuration is now defined both in conftest.py and in tests/e2e/test_bay_api.py; consider centralizing this in conftest to avoid duplication and configuration drift.
  • The new logger.info call in forward_request_to_ship logs full exec responses, which may include large payloads or sensitive data; consider reducing the verbosity (e.g., log only type/summary) or gating it behind a debug log level.
  • The test scripts under tests/scripts/ hard-code proxy settings and image names (e.g., HTTP_PROXY, HTTPS_PROXY, shipyard-ship tags); consider making these configurable via environment variables to make the scripts easier to reuse in different environments.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The BAY_URL/ACCESS_TOKEN/get_auth_headers configuration is now defined both in conftest.py and in tests/e2e/test_bay_api.py; consider centralizing this in conftest to avoid duplication and configuration drift.
- The new logger.info call in forward_request_to_ship logs full exec responses, which may include large payloads or sensitive data; consider reducing the verbosity (e.g., log only type/summary) or gating it behind a debug log level.
- The test scripts under tests/scripts/ hard-code proxy settings and image names (e.g., HTTP_PROXY, HTTPS_PROXY, shipyard-ship tags); consider making these configurable via environment variables to make the scripts easier to reuse in different environments.

## Individual Comments

### Comment 1
<location> `pkgs/bay/app/services/ship/http_client.py:87` </location>
<code_context>
             ) as response:
                 if response.status == 200:
                     data = await response.json()
+                    logger.info(f"Ship exec response for {request.type}: {data}")
                     return ExecResponse(success=True, data=data)
                 else:
</code_context>

<issue_to_address>
**🚨 suggestion (security):** Avoid logging full exec response payloads at info level to reduce noise and potential data exposure.

Given the potential size and sensitivity of exec outputs, please avoid logging the full `data` object here. Instead, consider moving this to `debug`, logging only a small, redacted/summary subset (e.g., status, a few safe fields, length), or gating full-body logging behind a config/feature flag so production logs stay minimal and safe.
</issue_to_address>

### Comment 2
<location> `pkgs/bay/tests/unit/test_utils.py:10-19` </location>
<code_context>
+class TestMemoryUtils:
</code_context>

<issue_to_address>
**suggestion (testing):** Add unit tests for invalid/unsupported memory formats to validate error handling paths.

Current tests only cover valid units and minimum-enforcement behavior. Please also add cases for invalid inputs (e.g. "abc", "10XZ", negative values, None) to assert the expected failure mode (e.g. specific exception type or fallback value). This helps ensure misconfigurations are caught rather than silently accepted.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +10 to +19
class TestMemoryUtils:
"""内存工具函数单元测试"""

def test_parse_memory_string(self):
"""测试 parse_memory_string 函数"""
from app.drivers.core.utils import parse_memory_string

# K8s 风格单位
assert parse_memory_string("512Mi") == 536870912
assert parse_memory_string("1Gi") == 1073741824
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): 建议为无效/不受支持的内存格式添加单元测试,以验证错误处理路径。

当前测试只覆盖了有效单位和最小值强制行为。请同时增加无效输入的测试用例(例如 "abc"、"10XZ"、负数、None),以断言预期的失败模式(如特定的异常类型或回退值)。这有助于确保配置错误能被检测到,而不是被静默接受。

Original comment in English

suggestion (testing): Add unit tests for invalid/unsupported memory formats to validate error handling paths.

Current tests only cover valid units and minimum-enforcement behavior. Please also add cases for invalid inputs (e.g. "abc", "10XZ", negative values, None) to assert the expected failure mode (e.g. specific exception type or fallback value). This helps ensure misconfigurations are caught rather than silently accepted.

Add CI pipeline configuration to run unit and E2E tests for bay and ship
packages.

Include additional improvements for bay:
- Sanitize ship execution logs to prevent data exposure by creating a
  response summary and moving full logs to debug level.
- Add unit tests for invalid memory string parsing scenarios.
…tion

- Add background process registry to track processes per session
- Implement process listing endpoint GET /processes
- Fix environment variable passing in sudo commands
- Set kernel working directory via os.chdir in IPython init
- Add Python version file (.python-version)
- Fix integration test NO_PROXY handling and startup error flow
- Set kernel working directory via start_kernel(cwd=...) instead of
  dynamic os.chdir() execution for better reliability
- Extract matplotlib font configuration to static constant
- Rename function to _init_kernel_matplotlib for clarity
- Add error handling for image cleanup in integration tests
@RC-CHN RC-CHN merged commit a705f1e into AstrBotDevs:main Jan 23, 2026
3 checks passed
@RC-CHN RC-CHN deleted the improve-test branch January 23, 2026 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant