Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new integration test suite to validate the RPC/TCP interfaces that ckb-tui relies on, and extends the Python RPC client with a get_overview helper for the Terminal RPC module.
Changes:
- Add
test_cases/ckb-tui/test_ckb_tui.pycovering RPC dependencies, TCP subscriptions, and basicckb-tuibinary start/exit behavior. - Add
RPCClient.get_overview()to call the Terminal module’sget_overviewRPC.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
test_cases/ckb-tui/test_ckb_tui.py |
New test suite for ckb-tui RPC deps, TCP subscriptions, and binary smoke checks. |
framework/rpc.py |
Adds a get_overview RPC wrapper used by the new tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @classmethod | ||
| def setup_class(cls): | ||
| if not os.path.isfile(CKB_TUI_BIN): | ||
| pytest.skip("ckb-tui binary not found, run ckb_tui_prepare.sh first") |
There was a problem hiding this comment.
TestCkbTuiBinary skips the entire class when the ckb-tui binary is missing. There doesn't appear to be any build/download step in the repository that places source/ckb-tui/ckb-tui during make prepare, so in CI this suite may silently skip and not validate the binary integration. Consider adding a prepare step that builds/downloads ckb-tui, or converting these into explicit xfail/marker-gated tests so CI coverage is intentional.
| pytest.skip("ckb-tui binary not found, run ckb_tui_prepare.sh first") | |
| pytest.xfail("ckb-tui binary not found, run ckb_tui_prepare.sh first") |
| time.sleep(3) | ||
| poll = proc.poll() | ||
| os.killpg(os.getpgid(proc.pid), signal.SIGTERM) | ||
| proc.wait(timeout=5) |
There was a problem hiding this comment.
The process group is terminated unconditionally via os.killpg(...) even if proc.poll() indicates the process already exited. If ckb-tui exits early, killpg can raise ProcessLookupError and fail the test before the intended assertion. Only send signals when poll is None, and add a fallback (e.g., SIGKILL) if the process doesn't exit within the timeout.
| time.sleep(3) | ||
| poll = proc.poll() | ||
| os.killpg(os.getpgid(proc.pid), signal.SIGTERM) | ||
| proc.wait(timeout=5) |
There was a problem hiding this comment.
Same termination pattern as above: os.killpg(...) is called regardless of whether the process already exited, which can raise and make the test flaky. Gate the signal-sending on proc.poll() is None, and handle TimeoutExpired to avoid leaving orphaned processes.
| CKB_TUI_BIN = f"{get_project_root()}/source/ckb-tui/ckb-tui" | ||
| TCP_PORT = 18314 | ||
|
|
There was a problem hiding this comment.
This new test suite is added under test_cases/ckb-tui, but the repo's make test runner iterates only over the test_cases := ... list in Makefile, which currently does not include this directory. As a result, these tests likely won't run in CI unless the Makefile (or CI test selection) is updated to include test_cases/ckb-tui.
add tui test