-
Notifications
You must be signed in to change notification settings - Fork 2
Improve test #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve test #4
Conversation
…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.
There was a problem hiding this 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>帮我变得更有用!请在每条评论上点击 👍 或 👎,我会根据你的反馈改进后续评审。
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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 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 |
There was a problem hiding this comment.
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
统一了原来的测试脚本,使用pytest进行规范化测试
Summary by Sourcery
将 Bay 的测试标准化为基于 pytest 的结构化单元测试、集成测试和端到端(e2e)测试套件,并改进 ship 会话处理。
New Features:
Bug Fixes:
Enhancements:
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:
Bug Fixes:
Enhancements: