terrymanu commented on PR #38813: URL: https://github.com/apache/shardingsphere/pull/38813#issuecomment-4631977029
### Decision - **Merge Verdict: Mergeable** - **Reviewed Scope:** PR #38813 latest head `1df7f5bb69094a1b619095135bb8f86df934eefa`, local merge-base `bbbb20cdef21db82e8008ab14ca09120183c7dfa`; GitHub `/pulls/38813/files` matched local `apache/master...apache/pr/38813` exactly: 27 files, all under `test/e2e/mcp/src/test/java`. - **Not Reviewed Scope:** GitHub Actions/check-runs, full live MCP E2E lanes that require external LLM/Docker/database runtime, and full completion of broad discussion issue #35294 beyond this test-only coverage increment. - **Need Expert Review:** No extra expert review required for this test-only change; it does not modify production protocol, security, SQL parser, routing, or high-frequency Proxy/JDBC paths. ### Basis - The PR scope is correctly test-only and aligned with its stated goal: adding focused unit coverage for MCP E2E support logic that can be validated without running end-to-end scenarios. - The new tests are suitable to sink into unit tests because they exercise deterministic public or package-visible helper behavior directly: LLM conversation/action helpers, final-answer validation, tool-call normalization, JSON/payload utilities, HTTP/client request construction, fixture handlers, custom encrypt/mask algorithms, and packaged-distribution fixture support. - The change does not over-replace E2E coverage. Real runtime concerns such as live MCP server behavior, external LLM/model readiness, Docker/packaged distribution startup, database integration, and protocol interoperability remain outside these unit tests and should continue to be validated by the existing E2E/runtime lanes. - The fixture-assisted tests are scoped appropriately: they prove local fixture/SPi packaging and helper behavior, but do not claim to be full production distribution or live runtime proof. - No substantive unrelated changes were found. There are no dependency, POM, production-code, SQL parser, shared runtime, or high-frequency execution-path changes. - The linked issue #35294 is a broad MCP design/discussion issue. This PR uses `For #35294` and does not appear to claim full closure of that PRD-sized scope, so the narrower test-coverage increment is acceptable. ### Verification - `git diff --name-status refs/remotes/apache/master...refs/remotes/apache/pr/38813` matched the GitHub PR file list: exit code 0. - `./mvnw -pl test/e2e/mcp -am -DskipITs -Dspotless.skip=true -Dtest=<27 changed test classes> -Dsurefire.failIfNoSpecifiedTests=false test`: exit code 0, `BUILD SUCCESS`. - `./mvnw -pl test/e2e/mcp -DskipTests spotless:check -Pcheck -T1C`: exit code 0, `BUILD SUCCESS`. - `./mvnw -pl test/e2e/mcp -DskipTests checkstyle:check -Pcheck -T1C`: exit code 0, `BUILD SUCCESS`, 0 Checkstyle violations. - No GitHub Actions or CI status was used for this decision. -- 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]
