terrymanu commented on PR #39007:
URL: https://github.com/apache/shardingsphere/pull/39007#issuecomment-4885953589
### Summary
**Review Result: Mergeable**
Reason: Code-scope review only. The PR is a test-support refactor within
`test/e2e/mcp`, and the latest patch preserves the existing raw HTTP request
semantics while reducing duplication and removing the hard-coded official MCP
tool-name list. CI not reviewed by request.
### Evidence
- The PR scope is limited to 6 files under `test/e2e/mcp`, matching GitHub
`/pulls/39007/files` and local triple-dot diff against `apache/master`.
- `MCPHttpTransportTestSupport` now owns the shared raw HTTP helper behavior:
- JSON POST requests still use the same JSON request builder headers at
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/transport/client/MCPHttpTransportTestSupport.java:50`.
- Session headers preserve the same `MCP-Session-Id` and
`MCP-Protocol-Version` pair at
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/transport/client/MCPHttpTransportTestSupport.java:77`.
- DELETE, raw POST, event-stream GET, and JSON-RPC request helpers keep
caller-supplied header application and response handling at
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/transport/client/MCPHttpTransportTestSupport.java:94`,
`:111`, `:128`, and `:147`.
- Both abstract HTTP E2E bases now delegate request construction to the
shared helper without changing their protected test-facing methods:
- Programmatic runtime path:
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/AbstractHttpProgrammaticRuntimeE2ETest.java:84`.
- Protocol-only runtime path:
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/AbstractHttpProtocolOnlyE2ETest.java:81`.
- `OfficialMCPToolNames` now derives the official tool names from registered
MCP descriptors at
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/OfficialMCPToolNames.java:30`.
The `test/e2e/mcp` test classpath already includes `shardingsphere-mcp-core`
and the MCP feature modules in `test/e2e/mcp/pom.xml:45`, so this does not add
a dependency or widen the production contract.
- Added tests cover the new support-contract surface:
- HTTP helper request methods and headers are covered in
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/transport/client/MCPHttpTransportTestSupportTest.java:55`.
- Official tool-name source alignment is covered in
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/OfficialMCPToolNamesTest.java:29`.
- Existing in-process HTTP protocol E2E coverage still exercises
initialize, follow-up request, and delete flows through the abstract
protocol-only base at
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/HttpTransportProtocolContractE2ETest.java:37`.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR #39007, head
`da761a1d688ec20a72957d9600b1b97a135c0c92`, base `apache/master` at
`afe998587e779d11e18bd87c0dfaf0ab2c4747a1`, merge-base
`afe998587e779d11e18bd87c0dfaf0ab2c4747a1`. Reviewed all 6 GitHub-listed files,
and the local triple-dot file list matched GitHub `/pulls/39007/files`.
- Not Reviewed Scope: GitHub Actions/check-runs, full Docker MCP E2E, LLM
E2E, and unrelated repository modules outside the changed `test/e2e/mcp`
support/test paths.
- Verification:
- `./mvnw -pl test/e2e/mcp
-Dtest=MCPHttpTransportTestSupportTest,OfficialMCPToolNamesTest,HttpTransportProtocolContractE2ETest
-DfailIfNoTests=false test -Pcheck -T1C` exited 0.
- `./mvnw checkstyle:check -Pcheck -T1C` exited 0.
- GitHub PR metadata, file list, commits, issue comments, review comments,
and reviews were accessible through public GitHub API; no PR comments or
reviews were present at review time.
- Release Note / User Docs: Not required. This is an internal test-support
refactor with no user-facing behavior, configuration, protocol, or distribution
contract change.
--
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]
