terrymanu commented on PR #39213: URL: https://github.com/apache/shardingsphere/pull/39213#issuecomment-5046480847
### Summary **Review Result: Mergeable** Reason: The PR correctly removes synthetic null E2E parameters, supports empty parameter sources through `allowZeroInvocations`, and updates Docker port resolution. No code-scope blocker was found. This is a code-scope result; CI was not reviewed by request. ### Evidence - `SQLE2EITArgumentsProvider` now forwards only real parameters from `E2ETestParameterFactory`; empty parameter collections are handled explicitly by the updated parameterized tests. - Null-dependent skips and placeholder constructors were removed consistently across SQL E2E tests. - `DockerStorageContainer#getMappedPort()` resolves the published Docker binding after container startup and validates missing mappings. - A clean scoped compilation of `test/e2e/env` and `test/e2e/sql` completed successfully. ### Review Details - Review Focus: Code Correctness Review; CI not reviewed by request. - Reviewed Scope: 17 files listed by GitHub, head `b79ff7f5bd1df2bf2fc0cc66404175e624186cb6`; local changed-file list matched GitHub’s PR file list. Local merge-base: `6621687546488d654546b2c8a9f8bfd66cd12dfc`. - Not Reviewed Scope: CI checks, Docker runtime execution, and unrelated repository changes. - Verification: `./mvnw -pl test/e2e/env,test/e2e/sql -DskipTests -Dcheckstyle.skip -Dspotless.check.skip clean test-compile -T1C` — exit 0. - Release Note / User Docs: Not required; test-infrastructure-only changes with no user-facing behavior or configuration 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]
