terrymanu commented on PR #39046: URL: https://github.com/apache/shardingsphere/pull/39046#issuecomment-4910847024
### Summary **Review Result: Mergeable** Reason: Code-scope review only; CI not reviewed by request. The PR is a small test-only cleanup that removes unnecessary real database-type fixture values from MCP workflow tests without changing production behavior or weakening the dedicated dialect/case-sensitivity coverage. ### Evidence - The authoritative PR scope contains only four MCP workflow test files, and the GitHub `/pulls/39046/files` list matches the local triple-dot diff against `325b0c56874280030e30ca9efc97ada1b87961d5`. - `EncryptWorkflowPlanningServiceTest` now uses neutral `FixtureDB` metadata and exact `phone` fixture data for the lifecycle mismatch scenario, while the production path still obtains database type from metadata before `WorkflowSQLUtils.isSameIdentifier(...)` is used in planning. - `EncryptWorkflowValidationServiceTest` and `MaskWorkflowValidationServiceTest` now validate happy paths with `FixtureDB` and matching `phone` rule/request data, so the tests no longer depend on PostgreSQL unquoted identifier folding to pass. - `MaskWorkflowPlanningServiceTest` keeps the required `MCPFeatureQueryFacade#getDatabaseType` stubs where mask planning actually reads the facade, but changes them from `MySQL` to `FixtureDB`, preserving the tested lifecycle and mutation outcomes. - Dialect-specific behavior remains covered outside this PR scope, including `WorkflowSQLUtilsTest` cases for MySQL/PostgreSQL identifier matching and `EncryptRuleDistSQLPlanningServiceTest` cases for MySQL/PostgreSQL DistSQL formatting and matching. ### Review Details - Review Focus: Code Correctness Review. CI not reviewed by request. - Reviewed Scope: PR #39046, head `3d7c611f96cef9e338ef610b0828c913665e5114`, base `325b0c56874280030e30ca9efc97ada1b87961d5`, merge-base `325b0c56874280030e30ca9efc97ada1b87961d5`; reviewed the four changed files under `mcp/features/encrypt` and `mcp/features/mask`. Local file list matched GitHub `/pulls/39046/files`. - Not Reviewed Scope: GitHub Actions/check-runs were not reviewed; no production code, docs, release artifacts, or E2E changes were in this PR scope. - Verification: Ran `./mvnw -pl mcp/features/encrypt,mcp/features/mask -Dtest=EncryptWorkflowPlanningServiceTest,EncryptWorkflowValidationServiceTest,MaskWorkflowPlanningServiceTest,MaskWorkflowValidationServiceTest -Dsurefire.failIfNoSpecifiedTests=false -DskipITs test -Pcheck` on head `3d7c611f96cef9e338ef610b0828c913665e5114`; exit code 0, 60 targeted tests passed across the four changed test classes. - Release Note / User Docs: Not required; this is a test-only fixture cleanup with no user-facing behavior, configuration, API/SPI, or migration 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]
