terrymanu commented on PR #39040: URL: https://github.com/apache/shardingsphere/pull/39040#issuecomment-4905007817
### Summary **Review Result: Mergeable** Reason: This is a code-scope only result; CI was not reviewed by request. The PR is limited to MCP test fixtures and test callers, removes duplicated real JDBC-prefix dispatch from MCP tests, and keeps database type selection expressed through fixture-owned test URLs without changing production behavior. ### Evidence - `mcp/core/src/test/java/org/apache/shardingsphere/mcp/core/fixture/CoreDatabaseTypeFactoryMocker.java:53` and `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/fixture/SupportDatabaseTypeFactoryMocker.java:53` introduce a fixture URL creator, while `:86` parses only the fixture URL contract instead of maintaining per-database JDBC prefix branches. - `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/database/capability/MCPDatabaseCapabilityProviderTest.java:137`, `mcp/core/src/test/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowProxyQueryServiceTest.java:247`, and `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/database/metadata/jdbc/AbstractMCPJdbcMetadataLoaderTest.java:279` now delegate database-type URL construction to the fixture boundary. - Static scans on the latest PR head found no remaining `switch (databaseType)`, `switch (databaseMetadata.getDatabaseType())`, `METADATA_JDBC_URLS`, `databaseType.toLowerCase`, or real JDBC-prefix matching in the reviewed MCP core/support test paths. - Existing changed tests still exercise the affected capability-provider, metadata-loader, metadata-query, workflow-query, transaction-executor, and SQL-execution facade paths through the updated fixture. ### Review Details - Review Focus: Code Correctness Review; CI not reviewed by request. - Reviewed Scope: all 11 GitHub-listed changed files under `mcp/core/src/test` and `mcp/support/src/test`; latest PR head `1c8233969bb67452e8b3334b7305ac61a737aecb`, base ref `master` at `0db9ae3f841545fa751c7cadb4bdeaaf363064e6`, local merge-base `91a4e27823054d1050db1856c57e0dbdf5912736`; local triple-dot file list matched GitHub `/pulls/39040/files`. - Not Reviewed Scope: GitHub Actions/check-runs/workflow logs, MCP E2E, production runtime behavior outside test-fixture usage, and unrelated repository areas. - Verification: `./mvnw -pl mcp/support,mcp/core -am -DskipITs -Dspotless.skip=true -Dtest=MCPDatabaseCapabilityProviderTest,MCPJdbcDatabaseProfileLoaderTest,MCPJdbcMetadataLoaderSequenceTest,MCPJdbcMetadataLoaderSchemaFilteringTest,MetadataQueryServiceTest,WorkflowProxyQueryServiceTest,MCPJdbcTransactionStatementExecutorTest,MCPSQLExecutionFacadeConcurrencyTest -Dsurefire.failIfNoSpecifiedTests=false test` passed with exit code 0 on the latest PR head. - Release Note / User Docs: Not required because the PR changes only test fixtures and test setup, with no production API, configuration, distribution, dependency, or user-facing behavior 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]
