terrymanu commented on PR #39047: URL: https://github.com/apache/shardingsphere/pull/39047#issuecomment-4911419566
### Summary **Review Result: Mergeable** Reason: Code-scope review only: the latest PR head strengthens MCP workflow/protocol E2E coverage and shared test support without production behavior changes, and I did not find a code-level blocker. CI not reviewed by request. ### Evidence - The PR scope is test/E2E-only: 9 GitHub-listed files under `mcp/bootstrap/src/test`, `test/e2e/mcp/src/test`, and one proxy workflow test resource. - `mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/MCPArchitectureBoundaryTest.java:40` adds feature-module source directories, and `:66` verifies feature modules do not import bootstrap packages, preserving the intended MCP layering boundary. - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/production/AbstractProductionProxyWorkflowE2ETest.java:111` centralizes model-facing payload assertions for validation/apply/preview responses, so existing and new proxy workflow tests check the same payload contract. - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/production/HttpProductionProxyFeatureWorkflowContractE2ETest.java:116` through `:213` adds real proxy-backed plan/apply/validate/read-back coverage for readwrite-splitting, shadow, and sharding workflows, with storage-unit topology provided by `test/e2e/mcp/src/test/resources/proxy/workflow/database-logic-db.yaml`. - `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/HttpTransportProtocolContractE2ETest.java:87` and `:187` add protocol coverage for parameterized `Accept` headers and cursor-bearing list requests through the real HTTP transport path. - The rename from `ProxyEncryptWorkflowRuntimeTestSupport` to `ProxyWorkflowRuntimeTestSupport` is reflected at all in-scope references; no stale references were found in the changed test tree. ### Review Details - Review Focus: Code Correctness Review; CI not reviewed by request. - Reviewed Scope: latest PR head `70e7914efb36e3ac27f3d024490d9dd32cf36f3c`, base ref `master` at `5c5adcdf579529416e91bc6cecb71ff02b509bd9`, local merge-base `0b8dcbb89202b87d6b981a28c1438c8178eb431c`. Local triple-dot file list matched GitHub `/pulls/39047/files`. - Reviewed files/modules: `mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/MCPArchitectureBoundaryTest.java`; all 7 changed MCP E2E Java test/support files under `test/e2e/mcp/src/test/java`; `test/e2e/mcp/src/test/resources/proxy/workflow/database-logic-db.yaml`. - Not Reviewed Scope: GitHub Actions/check-runs/logs were not queried; Docker-backed proxy workflow E2E scenarios were reviewed statically but not executed locally. - Verification: `./mvnw -pl mcp/bootstrap,test/e2e/mcp -DskipTests -DskipITs -Dspotless.skip=true test-compile` passed with exit code 0. `./mvnw -pl mcp/bootstrap,test/e2e/mcp -DskipITs -Dspotless.skip=true -Dtest=MCPArchitectureBoundaryTest,HttpTransportProtocolContractE2ETest -Dsurefire.failIfNoSpecifiedTests=false test` passed with exit code 0; `MCPArchitectureBoundaryTest` ran 2 tests and `HttpTransportProtocolContractE2ETest` ran 25 tests. No required GitHub endpoint for this code-correctness review was inaccessible. - Release Note / User Docs: not required; this PR changes MCP test coverage, test fixture data, and test support names only, with no user-facing behavior, configuration, migration, or release-note impact. -- 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]
