Skip to content

[tests]: add test cases for tools and edge cases - #219

Open
ashwinkumar1211 wants to merge 1 commit into
apache:masterfrom
ashwinkumar1211:feat/addTests
Open

[tests]: add test cases for tools and edge cases#219
ashwinkumar1211 wants to merge 1 commit into
apache:masterfrom
ashwinkumar1211:feat/addTests

Conversation

@ashwinkumar1211

Copy link
Copy Markdown

This PR adds new test coverage for previously under-tested MCP tool behavior, with a focus client-server tool interaction.

Added integration-style client-server tests for:

  • invalid tool name handling
  • missing required parameters
  • database table listing

Intentional: some server responses represent failures as an explicit error object while others use a success: False envelope. The test accepts either success or error to ensure we validate behavior without being brittle to those response-shape variations

@ashwinkumar1211
ashwinkumar1211 marked this pull request as draft August 16, 2026 12:04
@FreeOnePlus
FreeOnePlus marked this pull request as ready for review August 17, 2026 00:50
@FreeOnePlus

Copy link
Copy Markdown
Member

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.

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.

2 participants