SEZ9 commented on PR #10252:
URL: https://github.com/apache/seatunnel/pull/10252#issuecomment-5225134186
Thanks @DanielLeens for the detailed review — it lines up with what I found
on the latest head, and thanks @chl-wxp for the ping.
@DanielLeens agreed on the key point: the `junit-jupiter-api` dependency in
`connector-jdbc/pom.xml` is missing `<scope>test</scope>`, which leaks JUnit
into the connector's compile/runtime dependency surface. This is the blocking
issue and is likely related to the red Build check as well.
To the PR author, here's the concrete list of remaining asks before this can
merge:
1. **(Blocking)** Add `<scope>test</scope>` to the `junit-jupiter-api`
dependency in `connector-jdbc/pom.xml`. Test-only dependencies must not end up
on the connector's compile classpath.
2. Please rebase on the latest `dev` as @chl-wxp suggested and push so CI
can re-run — the Build check needs to be green.
3. Move the `System.setProperty("seatunnel.engine.port", "0")` out of the
static initializer in `AbstractPostgresContainerTest` (e.g. into `@BeforeAll`
with cleanup in `@AfterAll`, or drop it if it's not actually needed). Mutating
global state in `<clinit>` can leak into unrelated tests running in the same
JVM.
4. Clean up `test_catalog_table` after the catalog tests (an
`@AfterEach`/`@AfterAll` drop is fine). Since the container is shared per
class, leftover tables can cause order-dependent failures.
5. Either remove the `@Testcontainers` annotation or use
`@Container`-managed lifecycle consistently. Right now the annotation is inert
because the container is started/stopped manually, which is misleading to
future contributors.
The migration approach itself is solid — testing `PostgresCatalog` and the
dialect against a real `postgres:15-alpine` container is a clear improvement
over the environment-dependent legacy suites, and the new coverage (9 catalog +
13 dialect tests) is welcome. Once the items above are addressed and CI is
green, this should be good to go.
<!-- streview-comment:88 -->
--
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]
