terrymanu commented on PR #38809: URL: https://github.com/apache/shardingsphere/pull/38809#issuecomment-4627604618
### Decision - **Merge Verdict: Mergeable** - **Reviewed Scope:** PR #38809 latest head `0c251cfa3ff03b3895aa9dd2d79e3fd2f8b5e52c`; base/merge-base `1e934a2f99e9ff5d9d4d2ebedda05ef8a13e852e`. GitHub `/pulls/38809/files` matched local triple-dot diff exactly: 11 files, all under `test/e2e/mcp`. - **Not Reviewed Scope:** GitHub Actions / CI status, full repository `clean install`, external LLM-provider behavior, and files outside the GitHub PR file list except supporting lower-layer tests used as coverage evidence. - **Need Expert Review:** No additional expert review required for this test-layering-only change. ### Basis - The PR only changes MCP E2E test coverage and support test resources; no production code, public API/SPI, dependency, parser, routing, or runtime config behavior is changed. - The remaining E2E coverage still exercises the runtime boundaries that should stay E2E: HTTP initialization/session/protocol behavior in `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/HttpTransportProtocolContractE2ETest.java:45`, transaction/session behavior in `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/ExecuteQueryTransactionE2ETest.java:40`, recovery paths in `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/HttpTransportRecoveryE2ETest.java:50`, security/origin behavior in `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/HttpTransportSecurityE2ETest.java:51`, and Docker-backed metadata discovery in `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/MetadataDiscoveryE2ETest.java:44`. - The moved deterministic checks are covered at lower cost by focused tests: payload parsing/casting in `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/transport/MCPInteractionPayloadsTest.java:30`, model contract assertions in `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/assertion/MCPModelContractAssertionsTest.java:31`, and normalized baseline matching in `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/assertion/MCPBaselineContractAssertionsTest.java:31`. - Adjacent and removed duplicate branches are still covered in lower layers: MCP error recovery categories in `mcp/core/src/test/java/org/apache/shardingsphere/mcp/core/protocol/MCPErrorConverterTest.java:81`, SQL tool mismatch and metadata introspection recovery in `mcp/core/src/test/java/org/apache/shardingsphere/mcp/core/protocol/MCPErrorConverterTest.java:286`, transaction/savepoint command handling in `mcp/core/src/test/java/org/apache/shardingsphere/mcp/core/session/MCPJdbcTransactionStatementExecutorTest.java:72`, protocol negotiation/header validation in `mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/server/http/StreamableHttpMCPServletTest.java:101`, and origin/protocol constraints in `mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/server/http/validator/constraint/OriginHeaderConstraintTest.java:30`. - Linked issue #35294 is an umbrella MCP ideas/discussion issue, and this PR uses `For #35294` rather than claiming to close it. The patch scope is therefore appropriately limited to reducing duplicated MCP E2E coverage. ### Verification - Confirmed GitHub file scope against local triple-dot diff: `git diff --name-status 1e934a2f99e9ff5d9d4d2ebedda05ef8a13e852e..refs/remotes/apache/pr/38809`. - `./mvnw -pl test/e2e/mcp -am -DskipITs -Dspotless.skip=true -Dtest=MCPInteractionPayloadsTest,MCPModelContractAssertionsTest,MCPBaselineContractAssertionsTest -Dsurefire.failIfNoSpecifiedTests=false test` passed; 28 focused support tests. - `./mvnw -pl mcp/core -am -DskipITs -Dspotless.skip=true -Dtest=MCPErrorConverterTest,MCPJdbcTransactionStatementExecutorTest -Dsurefire.failIfNoSpecifiedTests=false test` passed; 54 tests. - `./mvnw -pl mcp/bootstrap -am -DskipITs -Dspotless.skip=true -Dtest=StreamableHttpMCPServletTest,ProtocolVersionHeaderConstraintTest,OriginHeaderConstraintTest,ShardingSphereServerTransportSecurityValidatorTest,ServerTransportSecurityValidatorFactoryTest -Dsurefire.failIfNoSpecifiedTests=false test` passed; 46 tests. - `./mvnw -pl test/e2e/mcp -am -DskipITs -Dspotless.skip=true -De2e.run.type=DOCKER -Dtest=ExecuteQueryTransactionE2ETest,HttpTransportApprovalSafetyE2ETest,HttpTransportContractE2ETest,HttpTransportProtocolContractE2ETest,HttpTransportRecoveryE2ETest,HttpTransportSecurityE2ETest,MetadataDiscoveryE2ETest -Dsurefire.failIfNoSpecifiedTests=false test` passed; 39 E2E tests, 0 skipped. -- 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]
