FreeOnePlus commented on PR #219:
URL: https://github.com/apache/doris-mcp-server/pull/219#issuecomment-5310718153
Thanks for adding client/server coverage. I found two blocking issues that
need to be addressed before this PR provides meaningful regression protection.
### 1. The four new tests do not run in CI
The class-scoped connectivity fixture skips the entire class when no
external server is available at `localhost:3000`. The base branch reported
`1863 passed, 85 skipped`; this PR reports `1863 passed, 89 skipped`, so all
four added tests are being skipped.
Please make these tests self-contained: start an ephemeral MCP server from a
pytest fixture, preferably reusing the real transport patterns in
`test/integration/test_real_doris_transports.py`, and connect the client to
that fixture. A required CI fixture should fail if setup fails instead of
silently skipping. Since this is client/server coverage, please exercise
Streamable HTTP and STDIO, or clearly scope the tests to the transport they
actually verify.
### 2. The tests call removed 0.x flat tool names and accept `Tool not
found` as success
The 1.0 default endpoint exposes top-level domains. `exec_query`,
`get_db_table_list`, and `get_catalog_list` are not callable top-level tools.
Against a current 1.0 hierarchical server, all three return `{"success": false,
"error": "Tool not found"}`, yet the new assertions still pass.
Please call the current child tools through the client's
progressive-disclosure API:
- `client_instance.tools.call_child("doris_query", "execute_query", {})` for
the missing-`sql` case. Require an error result whose validation details
identify `sql`.
- `client_instance.tools.call_child("doris_catalog", "list_tables",
{"database": "information_schema"})`. Require `mode == "result"` and validate
the returned collection payload.
- `client_instance.tools.call_child("doris_catalog", "list_catalogs", {})`.
Require `mode == "result"` and validate the returned collection payload.
For the invalid-tool case, require `result.get("success") is False` and
assert the `Tool not found` error. The current `assert "success" in result or
"error" in result` would also accept an incorrect `{"success": true}` response.
After these changes, the CI skip count should not increase, and each test
should fail if the intended child is unavailable, returns the wrong envelope,
or produces the wrong payload shape.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
