terrymanu commented on PR #39043:
URL: https://github.com/apache/shardingsphere/pull/39043#issuecomment-4907174900
### Summary
**Review Result: Mergeable**
Reason: This review result is limited to the Code Correctness Review scope
only. CI not reviewed by request, so this is not a final repository merge-gate
decision. PR #39043 only changes MCP test code. It mainly replaces real
database-type fixtures that do not carry dialect semantics with
`FixtureDB`-style samples, and uses explicit mocked capability semantics in
executor/capability tests instead of dispatching through real database-type SPI
implementations. I found no code-level blocking issue.
### Evidence
- GitHub `/pulls/39043/files` matches the local
`apache/master...refs/remotes/apache/pr/39043` triple-dot diff file list. All
19 changed files are under `mcp/**/src/test/**`; there are no production code,
configuration, dependency, distribution, or test-jar changes.
- Executor tests that previously obtained capabilities through real database
types now declare the tested semantics directly. For example,
`MCPJdbcStatementExecutorTest` mocks `MCPDatabaseCapabilityOption` and
explicitly sets schema execution semantics instead of depending on a concrete
database type via `TypedSPILoader`:
`mcp/core/src/test/java/org/apache/shardingsphere/mcp/core/tool/handler/execute/MCPJdbcStatementExecutorTest.java:386`.
- The transaction executor test also declares transaction capability
directly: `logic_db` covers `LOCAL_WITH_SAVEPOINT`, while `warehouse` covers
the `LOCAL` branch without savepoint support, preserving the original test
intent:
`mcp/core/src/test/java/org/apache/shardingsphere/mcp/core/session/MCPJdbcTransactionStatementExecutorTest.java:136`
and
`mcp/core/src/test/java/org/apache/shardingsphere/mcp/core/session/MCPJdbcTransactionStatementExecutorTest.java:149`.
- The capability response test no longer binds itself to the `MySQL` SPI
implementation. It mocks the object type, statement, transaction, schema, and
explain-analyze capabilities needed by the response contract directly:
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/database/response/MCPDatabaseCapabilityResponseTest.java:41`.
- Metadata/query test data now uses fixture database type names without
introducing runtime database dispatch logic. The changed files do not add real
database type names such as `MySQL`, `PostgreSQL`, or `Hive`:
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/database/metadata/query/DatabaseTestDataFactory.java:55`.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR #39043 latest head
`d729b1d3f3530001aed5bdd9d1fa9a4595e1f036`, base `master` at
`801447ba3ce887f51bf751724263ff0ed469dfa9`, local merge-base
`801447ba3ce887f51bf751724263ff0ed469dfa9`. Reviewed all 19 GitHub-listed
changed files across `mcp/bootstrap`, `mcp/core`, `mcp/features/broadcast`,
`mcp/features/readwrite-splitting`, `mcp/features/sharding`, and `mcp/support`;
the local file list matched GitHub `/pulls/39043/files`.
- Not Reviewed Scope: GitHub Actions/check-runs/workflow logs were not
queried for this Code Correctness Review. MCP Docker/E2E scenarios were not run
because the PR is test-only and does not touch `test/e2e/mcp`, production
runtime code, or distribution packaging.
- Verification:
- Public GitHub REST PR metadata/files/comments/reviews were accessible;
no public review/comment threads were present.
- Confirmed local tracked changed files match PR head
`d729b1d3f3530001aed5bdd9d1fa9a4595e1f036`: exit 0.
- `./mvnw -pl
mcp/support,mcp/core,mcp/bootstrap,mcp/features/broadcast,mcp/features/readwrite-splitting,mcp/features/sharding
-am -DskipITs
-Dtest=MCPDatabaseMetadataTest,MCPJdbcDatabaseProfileLoaderTest,MCPJdbcMetadataLoaderFailureTest,MCPDatabaseCapabilityResponseTest,RuntimeDatabaseValidationServiceTest,MCPCompletionServiceTest,MetadataCompletionProviderTest,MetadataResourceHandlerTest,MCPJdbcTransactionStatementExecutorTest,MCPJdbcStatementExecutorTest,MCPSQLExecutionFacadeConcurrencyTest,SearchMetadataToolHandlerTest,SearchMetadataToolServiceTest,MCPCompletionSpecificationFactoryTest,BroadcastTableRuleHandlerTest,BroadcastWorkflowValidationServiceTest,BroadcastWorkflowPlanningServiceTest,ReadwriteSplittingWorkflowValidationServiceTest,ReadwriteSplittingWorkflowPlanningServiceTest,ShardingWorkflowValidationServiceTest
-Dsurefire.failIfNoSpecifiedTests=false test -Pcheck`: exit 0.
- `./mvnw checkstyle:check -Pcheck -T1C`: exit 0.
- Release Note / User Docs: not required; this is an internal test-only
refactor with no user-facing behavior, configuration, API/SPI, migration, or
compatibility 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]
