.2059806565729300:d6664ca4f32d20c14d55ddb52807acef_69e3499ab13256d09ece8088.69e41986b13256d09ece8339.69e419867afba9383018b939:Trae CN.T(2026/4/19 07:53:42)#1274
Conversation
重构集成测试脚本，增加健康检查轮询机制替代固定等待时间 添加详细的测试帮助文档和环境变量配置说明 改进日志记录和错误检测功能
添加全局和单测超时控制，改进错误分类和诊断信息 添加 pytest-timeout 依赖，支持进程树清理 优化日志输出和错误处理流程
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import json | ||
| import signal | ||
| import atexit | ||
| import sys |
|
|
||
|
|
||
| GLOBAL_SERVER_PROCESS: Optional['ServerProcess'] = None | ||
| GLOBAL_TEST_TIMEOUT: int = 120 |
| for child in children: | ||
| try: | ||
| child.kill() | ||
| except psutil.NoSuchProcess: |
|
|
||
| try: | ||
| parent.kill() | ||
| except psutil.NoSuchProcess: |
| for p in still_alive: | ||
| try: | ||
| p.kill() | ||
| except psutil.NoSuchProcess: |
| p.kill() | ||
| except psutil.NoSuchProcess: | ||
| pass | ||
| except psutil.NoSuchProcess: |
| else: | ||
| try: | ||
| os.killpg(os.getpgid(pid), signal.SIGKILL) | ||
| except ProcessLookupError: |
| except OSError: | ||
| try: | ||
| os.kill(pid, signal.SIGKILL) | ||
| except ProcessLookupError: |
|
|
||
| start_time = time.time() | ||
| poll_count = 0 | ||
| last_exit_check = 0 |
| if path and os.path.exists(path): | ||
| try: | ||
| os.remove(path) | ||
| except Exception: |
Greptile SummaryThis PR rewrites the integration test script with structured error classification, health-poll-based server startup, process-tree cleanup, and per-test/global timeout support, also wiring
Confidence Score: 3/5Not safe to merge — three P1 issues cause test failures to be misreported, cleanup to crash on binary-not-found, and existing aw-server tests to stop running. Three distinct P1 defects: wrong exception type causes ERRORs instead of FAILs, a FileNotFoundError in cleanup masks real failures, and a silent removal of aw-server/tests/ reduces coverage. These collectively undermine the primary goal of reliable CI integration testing. scripts/tests/integration_tests.py (TestFailure propagation, _cleanup_logs path clearing) and Makefile (aw-server/tests/ omission) Important Files Changed
Sequence DiagramsequenceDiagram
participant Make as make test-integration
participant Shell as timeout wrapper
participant Pytest as pytest
participant Fixture as server_process fixture
participant SP as ServerProcess
participant Bin as aw-server binary
participant Tests as Test classes
Make->>Shell: timeout 180s pytest ...
Shell->>Pytest: run integration_tests.py
Pytest->>Fixture: setup (scope=session)
Fixture->>SP: start()
SP->>Bin: subprocess.Popen(cmd)
SP->>SP: wait_for_ready() polls /api/0/info
Bin-->>SP: HTTP 200 ready
SP-->>Fixture: yield server
Fixture-->>Tests: server_process available
Tests->>SP: test_server_starts is_alive()
Tests->>SP: test_api_info_endpoint get_api_info()
SP->>Bin: GET /api/0/info
Bin-->>SP: 200 version hostname
Tests->>SP: test_api_buckets_endpoint get_api_buckets()
SP->>Bin: GET /api/0/buckets
Bin-->>SP: 200 buckets
Tests->>SP: test_no_error_indicators_in_logs check_for_errors()
Pytest->>Fixture: teardown
Fixture->>SP: check_for_errors() post-yield
Fixture->>SP: cleanup() finally also calls check_for_errors()
SP->>Bin: kill process tree
SP->>SP: _cleanup_logs() delete temp files
Reviews (1): Last reviewed commit: "feat(测试): 增强集成测试的超时处理和错误诊断" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| timeout --signal=SIGKILL $(AW_TEST_TIMEOUT) \ | ||
| pytest scripts/tests/integration_tests.py -v \ | ||
| --timeout=$(AW_PYTEST_TIMEOUT) \ | ||
| -o timeout_method=thread; \ | ||
| fi |
There was a problem hiding this comment.
./aw-server/tests/ silently dropped from test run
The original test-integration target ran both ./scripts/tests/integration_tests.py and ./aw-server/tests/. The new target only runs scripts/tests/integration_tests.py, meaning any tests under aw-server/tests/ are no longer executed via make test-integration. If this is intentional (e.g., those tests have moved or become redundant), it should be documented; otherwise, this is a silent regression in test coverage.
| def get_api_info(self) -> Dict[str, Any]: | ||
| if not HAS_URLLIB: | ||
| pytest.skip("urllib not available") | ||
|
|
||
| status, data = self._make_request("info") | ||
|
|
||
| if status != 200 or data is None: | ||
| self._print_diagnostic_summary( | ||
| ErrorType.API_ERROR, | ||
| f"GET /api/0/info failed with status {status}" | ||
| ) | ||
| raise TestFailure( | ||
| ErrorType.API_ERROR, | ||
| f"GET /api/0/info failed with status {status}", | ||
| {"status": status, "endpoint": "/api/0/info"} | ||
| ) | ||
|
|
There was a problem hiding this comment.
TestFailure propagates as pytest ERROR, not FAIL
get_api_info() and get_api_buckets() raise TestFailure (a plain Exception subclass) on non-200 responses. The test methods call these without try/except, so the exception propagates uncaught and pytest reports the test as ERROR (E) rather than FAILED (F). This makes CI output misleading — errors from a failing API look like infrastructure issues rather than test failures.
Either convert get_api_info() / get_api_buckets() to call pytest.fail() directly instead of raising TestFailure, or wrap the call in the test method:
try:
info = server_process.get_api_info()
except TestFailure as e:
pytest.fail(str(e))| def _cleanup_logs(self): | ||
| for path in [self.stdout_path, self.stderr_path]: | ||
| if path and os.path.exists(path): | ||
| try: | ||
| os.remove(path) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
cleanup() crashes with FileNotFoundError after start() fails
When start() raises TestFailure (e.g., binary not found), it calls self._cleanup_logs() which deletes the temp files, but leaves self.stdout_path and self.stderr_path pointing to the now-deleted paths. The fixture's finally block then calls cleanup(), which calls check_for_errors():
if self.stdout_path: # non-None, still points to deleted file
with open(self.stdout_path, "r", ...) as f: # raises FileNotFoundErrorThis unhandled FileNotFoundError in finally replaces the original TestFailure as the reported exception, masking the actual failure reason. Fix by clearing the paths in _cleanup_logs():
def _cleanup_logs(self):
for attr in ["stdout_path", "stderr_path"]:
path = getattr(self, attr)
if path and os.path.exists(path):
try:
os.remove(path)
except Exception:
pass
setattr(self, attr, None)|
|
||
|
|
||
| GLOBAL_SERVER_PROCESS: Optional['ServerProcess'] = None | ||
| GLOBAL_TEST_TIMEOUT: int = 120 |
There was a problem hiding this comment.
GLOBAL_TEST_TIMEOUT is dead code; default inconsistent with Makefile
GLOBAL_TEST_TIMEOUT = 120 is set but never read anywhere in the module — the actual timeout enforcement is done by the shell's timeout/gtimeout wrapper in the Makefile, which defaults to AW_TEST_TIMEOUT ?= 180. The module docstring also claims the default is 120s, creating a three-way inconsistency. The unused constant should be removed or wired up.
| ok, errors = server.check_for_errors() | ||
| if not ok: | ||
| server._print_diagnostic_summary( | ||
| ErrorType.LOG_ERROR, | ||
| f"Error indicators found after tests: {errors}" | ||
| ) | ||
| pytest.fail(f"Error indicators found in server logs: {errors}") | ||
|
|
||
| finally: | ||
| server.cleanup() |
There was a problem hiding this comment.
check_for_errors() call after tests finish
The server_process fixture calls server.check_for_errors() after yield, then unconditionally calls server.cleanup() in finally. cleanup() also calls self.check_for_errors() (line 471), so if errors are found the diagnostic summary is printed twice, adding noise to CI output.
3 participants
增强集成测试的超时处理和错误诊断
添加全局和单测超时控制，改进错误分类和诊断信息
添加 pytest-timeout 依赖，支持进程树清理
优化日志输出和错误处理流程