SEZ9 commented on PR #10252:
URL: https://github.com/apache/seatunnel/pull/10252#issuecomment-5230721863
Thanks @DanielLeens for the detailed follow-up — your analysis matches what
I found in my review of `9dfc82c5932f`, and +1 to @chl-wxp's suggestion to
rebase on the latest `dev` before the next push.
To summarize the concrete remaining asks before this can merge:
1. **(Blocking)** Add `<scope>test</scope>` to the `junit-jupiter-api`
dependency in `connector-jdbc/pom.xml`. As it stands it leaks into the
connector's compile/runtime dependency surface, which we can't ship.
2. **Rebase on latest dev** and re-run CI — the Build check is currently
red, and we need a green run on the rebased head to confirm the new container
tests are stable in CI.
3. **Clean up `test_catalog_table`** after the tests run (e.g. drop it in
`@AfterEach`/`@AfterAll`). Since the container is shared per test class,
leftover state can cause ordering-dependent failures as more tests are added to
the suite.
4. **Resolve the lifecycle inconsistency**: either use `@Testcontainers` +
`@Container` properly, or drop the `@Testcontainers` annotation and keep the
manual `@BeforeAll`/`@AfterAll` lifecycle. Right now the annotation is inert
and misleading.
5. **Avoid mutating global state in the static initializer** —
`System.setProperty("seatunnel.engine.port", "0")` in
`AbstractPostgresContainerTest.<clinit>` affects the whole JVM for other tests
in the same fork. If it's genuinely needed, set it in `@BeforeAll` and restore
the original value in `@AfterAll`; if not, please remove it.
The migration itself is sound and the added coverage (real catalog/dialect
behavior against an actual PostgreSQL instead of string assertions) is a clear
improvement — happy to do a final pass once the above are addressed.
<!-- streview-comment:109 -->
--
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]
