terrymanu commented on PR #39240:
URL: https://github.com/apache/shardingsphere/pull/39240#issuecomment-5077524741
### Summary
**Review Result: Mergeable**
The latest version of PR #39240 is mergeable from a code-correctness
perspective. It removes redundant MCP E2E scenarios while preserving HTTP
protocol contract coverage, real database/Proxy workflows, and both HTTP and
stdio LLM coverage. This conclusion covers code correctness only; CI not
reviewed by request.
### Evidence
- HTTP initialization, instructions, and unsupported resource/tool behavior
are directly verified by
[StreamableHttpMCPServerIT.java:76](mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/server/http/StreamableHttpMCPServerIT.java#L76).
- The stdio LLM path remains covered by
[LLMStdioTransportE2ETest.java:94](test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/suite/transport/LLMStdioTransportE2ETest.java#L94),
while HTTP usability scenarios remain covered by `LLMUsabilitySuiteE2ETest`.
- The workflow matrix is aligned with the stdio, usability core/extended,
and builder scenarios; the class name matches the workflow entry at
[e2e-mcp.yml:273](.github/workflows/e2e-mcp.yml#L273).
- The removed SQL error paths remain covered by `MCPStatementAnalyzerTest`,
`ExplainSQLCandidateValidatorTest`, `ExecuteQueryToolHandlerTest`, and
transaction-related unit tests.
- The removed cross-module workflow resource test is covered by feature
handler tests that verify actual `resources_to_read` generation.
- No production code, public API, dependency, or distribution configuration
was modified.
### Review Details
- Review Focus: Code Correctness Review; CI not reviewed by request.
- Reviewed Scope: 18 GitHub-changed files; head
`536cdc515ceb9fc006c0b988197a0eb08e51fa99`; base/merge-base
`b6ff1fb56dc34412801f08dd06f27fa6f2ea36b4`. The local triple-dot file list
matches GitHub `/pulls/39240/files`.
- Not Reviewed Scope: GitHub Actions, check runs, and full remote E2E
requiring Docker/LLM services.
- Verification:
- Spotless: passed
- Checkstyle: passed
- MCP test harness: 351 tests passed
- HTTP MCP IT: 10 tests passed
- Release Note / User Docs: Not required; this PR only refactors test
layering and the CI test matrix without changing user-visible runtime behavior,
configuration, or APIs.
--
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]
