terrymanu commented on PR #39614:
URL: https://github.com/apache/shardingsphere/pull/39614#issuecomment-5435265197
### Result
**Review Result: Mergeable**
The last commit `4b3b55e3a0f2` ("Refactor JDBCBackendStatementTest to remove
redundant parameterization") correctly removes a genuinely redundant
parameterization: the deleted `databaseTypeName` parameter was never used in
the test body, so the original three parameterized runs executed identical code
paths, and the refactoring loses zero behavioral coverage. This verdict covers
only the last commit, not the PR's other three commits.
### Evidence
- In the pre-commit version (introduced by `dd873cc948a`, #38644),
`assertCreateStorageResourceWithoutPreparedStatementCacheWithoutActiveKey` was
parameterized over MySQL/PostgreSQL/openGauss via
`@MethodSource("nonFirebirdDatabaseTypes")`, but the method body only
constructed an unstubbed `mock(DatabaseType.class)`. The `databaseTypeName`
string parameter was dead, making all three runs byte-for-byte equivalent.
- Production path verified: starting at
`proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/statement/JDBCBackendStatement.java:62`,
with `CONNECTION_STRICTLY` mode and an empty
`getCurrentPreparedStatementCacheKey()`, the code calls
`connection.prepareStatement(sql)` directly. The `DatabaseType` argument is
consumed only by `setFetchSize` (`JDBCBackendStatement.java:98`) in the
`MEMORY_STRICTLY` branch. This test uses `CONNECTION_STRICTLY`, so replacing
the unstubbed mock with the `databaseType` field (the "FIXTURE" SPI instance)
changes no behavior under test.
- Assertions and stubbing are fully preserved: the test still verifies
`verify(connection, times(2)).prepareStatement(sql)` (no PreparedStatement
reuse without a cache context or an active key).
- The removed imports (`ParameterizedTest`, `Arguments`, `MethodSource`,
`Stream`) have no remaining references in the file.
- Local verification: `./mvnw -pl proxy/backend/core -DskipITs
-Dspotless.skip=true
-Dtest=org.apache.shardingsphere.proxy.backend.connector.jdbc.statement.JDBCBackendStatementTest
-Dsurefire.failIfNoSpecifiedTests=false test`, exit 0, `Tests run: 6,
Failures: 0, Errors: 0, Skipped: 0` (previously 8 runs; the 2 removed runs are
exactly the eliminated duplicates).
### Coverage
- Review scope: PR #39614 head `4b3b55e3a0f280def0303152e5896b92d3cf9ce6`
(identical to local HEAD), narrowed per the explicit user request to the last
commit only; that commit changes 1 file
(`proxy/backend/core/src/test/java/.../JDBCBackendStatementTest.java`, +4/−16,
test-only). The PR's other three commits (27 files total, including
Skill/policy and CDC fixes) are outside this review scope.
- Behavior clusters: a single cluster — test de-parameterization removing a
dead parameter; risk triage completed with no high-risk axis triggered (no
production code, public contract, configuration, or compatibility change).
- Discovery lenses: all three lenses completed (root cause and behavior,
blast radius and contracts, tests and runtime); candidate findings (dialect
coverage loss, DatabaseType instance source change, leftover imports) were each
disproven by evidence, with no blockers.
- Convergence pass: one full-scope convergence pass over the commit found no
new independent candidates.
- Unresolved gaps: none. This is a code-scope-only conclusion (Code
Correctness Review); CI was not queried or reviewed.
--
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]
