terrymanu commented on PR #39212: URL: https://github.com/apache/shardingsphere/pull/39212#issuecomment-5046362432
### Summary **Review Result: Mergeable** Reason: This result applies to code scope only; CI not reviewed by request. The PR fixes the root cause at the argument-provider boundary by replacing synthetic all-null parameters with JUnit’s zero-invocation semantics. No functional, compatibility, or scope blocker was found. ### Evidence - Historical commit [`6e5466f3`](https://github.com/apache/shardingsphere/commit/6e5466f3b910c959422114e2c9c9e163b8706839) introduced the “empty parameter collection produces one null placeholder” workaround during the JUnit 4 dependency removal. The TODOs and downstream null guards were added to accommodate that synthetic invocation. - The project currently uses JUnit 5.14.1, whose official [`allowZeroInvocations`](https://docs.junit.org/5.14.1/api/org.junit.jupiter.params/org/junit/jupiter/params/ParameterizedTest.html#allowZeroInvocations()) contract supports parameter sources that are expected to produce zero invocations. - `SQLE2EITArgumentsProvider.java:37-43` now returns the stream of real parameters directly. For non-empty collections, it still uses the same factories and `Arguments::of` mapping, preserving parameter contents, ordering, and branch selection. - All 25 usages of `SQLE2EITArgumentsProvider` declare `allowZeroInvocations = true`. No placeholder construction, related TODO, or `testCaseContext == null` guard remains. - The only remaining construction sites for `AssertionTestParameter` and `CaseTestParameter` are `E2ETestParameterGenerator.java:126,196`, and both provide fully populated parameters. Removing null formatting paths used exclusively by synthetic placeholders is therefore safe. - The DML cleanup lifecycle remains valid. For example, `GeneralDMLE2EIT.java:49-59` completes initialization before entering the `try/finally`, while `BaseDMLE2EIT.java:102-115` creates the state required for cleanup during initialization. Zero-argument scenarios never enter this lifecycle. ### Review Details - Review Focus: Code Correctness Review — CI not reviewed by request. - Reviewed Scope: Latest PR head `84c2a2624b7942153f803eab31cec5d9710f3b9b`; base tip `5200b7d0a155dbea6043f889d5d2179a3fb2dd92`; local merge-base `6621687546488d654546b2c8a9f8bfd66cd12dfc`. All 16 changed files under `test/e2e/sql` were reviewed. The local triple-dot file list matched GitHub `/pulls/39212/files`. - Not Reviewed Scope: GitHub Actions, check-runs, and CI status; unchanged modules; non-empty E2E database matrices requiring actual database execution. - Verification: - `./mvnw -pl test/e2e/sql -Pcheck -DskipTests spotless:check`: exit 0, BUILD SUCCESS. - `./mvnw -pl test/e2e/sql -Pcheck -DskipTests checkstyle:check`: exit 0, BUILD SUCCESS. - `./mvnw -pl test/e2e/sql -DskipITs -Dspotless.skip=true test`: exit 0, 25 tests, 0 failures or errors. - Deterministic zero-argument `GeneralDMLE2EIT` scenario with `distsql_rdl`: exit 0, 0 invocations, BUILD SUCCESS. - Zero-argument `AdditionalDMLE2EIT` scenario: exit 0; four explicitly disabled methods remained skipped and the enabled methods produced zero invocations, BUILD SUCCESS. - Release Note / User Docs: Not required. The change affects internal SQL E2E test infrastructure only and does not alter user-facing behavior, configuration, APIs, SPIs, or release artifacts. -- 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]
