terrymanu commented on PR #39216: URL: https://github.com/apache/shardingsphere/pull/39216#issuecomment-5048717573
### Summary **Review Result: Mergeable** **Reason:** This is a code-scope-only result; CI was not reviewed by request. The refactor preserves existing empty-argument behavior while replacing a synthetic `null` invocation with JUnit’s supported zero-invocation contract. No code-level blocker was found. ### Evidence - The [latest implementation](https://github.com/apache/shardingsphere/blob/8b596b7726e0d9d8858eb88809d668f2067eed0d/test/e2e/operation/transaction/src/test/java/org/apache/shardingsphere/test/e2e/operation/transaction/engine/base/TransactionBaseE2EIT.java#L99-L109) enables `allowZeroInvocations` and leaves normal non-empty transaction execution unchanged. - The [arguments provider](https://github.com/apache/shardingsphere/blob/8b596b7726e0d9d8858eb88809d668f2067eed0d/test/e2e/operation/transaction/src/test/java/org/apache/shardingsphere/test/e2e/operation/transaction/engine/base/TransactionBaseE2EIT.java#L335-L382) now returns the filtered parameter collection directly. The [PR diff](https://github.com/apache/shardingsphere/pull/39216/files) shows that the previous empty-set path instead inserted `null` and immediately returned from the resulting invocation, so the observable no-work behavior is preserved without a fake test case. - The project [pins JUnit 5.14.1](https://github.com/apache/shardingsphere/blob/8b596b7726e0d9d8858eb88809d668f2067eed0d/pom.xml#L142). The official [JUnit 5.14.1 API](https://docs.junit.org/5.14.1/api/org.junit.jupiter.params/org/junit/jupiter/params/ParameterizedTest.html#allowZeroInvocations%28%29) defines `allowZeroInvocations = true` for cases where an empty argument set is expected. - A deterministic container-free sentinel selected a nonexistent transaction case, exercised the empty provider path, and completed with `Tests run: 0, Failures: 0, Errors: 0` and `BUILD SUCCESS`. - The change is limited to one test-harness file and does not alter production transaction behavior, public APIs, dependencies, packaging, security, or runtime performance. ### Review Details - **Review Focus:** Code Correctness Review — CI not reviewed by request. - **Reviewed Scope:** `test/e2e/operation/transaction`, specifically `TransactionBaseE2EIT.java`; latest head `8b596b7726e0d9d8858eb88809d668f2067eed0d`; base and local merge-base `5d0733a527c01f995a6060439a7c684672984fa7`. The local triple-dot file list matched GitHub `/pulls/39216/files`. - **Not Reviewed Scope:** GitHub Actions, check runs, workflow logs, and real Docker/native transaction execution. No production code was changed. - **Verification:** - `./mvnw -pl test/e2e/operation/transaction -DskipTests -Dspotless.skip=true test-compile` — exit 0, `BUILD SUCCESS`. - `./mvnw -pl test/e2e/operation/transaction checkstyle:check -Pcheck -T1C` — exit 0, `BUILD SUCCESS`. - `./mvnw -pl test/e2e/operation/transaction -Dspotless.skip=true -Dit.test=MySQLProxyTransactionE2EIT -De2e.run.type=DOCKER -De2e.transaction.cases=NoSuchTransactionCase -De2e.docker.database.mysql.images=dummy:latest failsafe:integration-test` — exit 0; zero invocations accepted as expected. - Authenticated public PR metadata, files, commits, comments, and reviews were inspected. No required GitHub endpoint remained inaccessible. - **Release Note / User Docs:** Not required because this is an internal test-harness refactor with no user-facing behavior or configuration 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]
