lintingbin commented on PR #4199:
URL: https://github.com/apache/amoro/pull/4199#issuecomment-4360003857
@czy006 @xxubai — would appreciate a review when you have a chance.
## Why this PR is intentionally narrow
This is a **partial** migration of `amoro-common` to JUnit 5, not a full
one. I split it that way after looking at the two earlier attempts on #3974:
- **#4004** rewrote the shared helpers (`TestAms`/`TestHMS` → plain classes;
`AmoroCatalogTestBase` → `@TempDir`/`@BeforeEach`; `AmoroRunListener` →
`TestExecutionListener`). CI then failed in `amoro-format-iceberg` with `No
ParameterResolver registered for parameter [AmoroCatalogTestHelper]`, because
the `@RunWith(Parameterized.class)` subclasses in iceberg/paimon still inject
`AmoroCatalogTestHelper` via constructor — JUnit 5 has no equivalent of that
mechanism without a custom `ParameterResolver` or refactoring the subclasses.
The thread stalled and the PR went stale.
- **#4059** tried a different angle on the same files but used `@BeforeEach
setup(AmoroCatalogTestHelper helper)`, which JUnit 5 won't inject either (only
`TestInfo`/`TestReporter`/`RepetitionInfo` are auto-injected). It also went
stale without review.
The root issue both PRs hit is that the shared bases in `amoro-common`
(`TestAms`, `TestHMS`, `AmoroCatalogTestBase`, `TestAmoroCatalogBase`,
`TestServerTableDescriptor`, `AmoroRunListener`) are extended or used as
`@Rule`/`@ClassRule`/`RunListener` by ~200 JUnit 4 tests in
`amoro-format-iceberg`, `amoro-format-paimon`, `amoro-format-mixed-flink`,
`amoro-mixed-hive`, `amoro-ams`, `amoro-optimizer-common`, `amoro-openapi-sdk`,
etc. You can't migrate the bases in isolation — every concrete
`@RunWith(Parameterized.class)` subclass has to be rewritten as
`@ParameterizedTest` + `@MethodSource` at the same time, and `AmoroRunListener`
has to be re-wired in `amoro-mixed-flink-common`'s surefire config.
## What this PR does
Migrates only the **6 self-contained test classes** in `amoro-common` that
have no external consumers:
- `TestLocalExecutionEngine`, `TestSimpleFuture`, `JacksonUtilTest`,
`MemorySizeTest`, `TestPoolConfig`, `MockZookeeperServer`
Mechanical changes only: `org.junit.*` → `org.junit.jupiter.api.*`;
`@Before/@After` → `@BeforeEach/@AfterEach`; `@Test(expected=...)` →
`Assertions.assertThrows(...)`; `Assert.assertX("msg", e, a)` →
`Assertions.assertX(e, a, "msg")`. `MemorySizeTest` also drops Hamcrest
`assertThat(x, is(y))` (which was a JUnit 4 transitive) for plain
`assertEquals`.
Shared bases / `@Rule` helpers / the `RunListener` are **not touched** —
that's the whole point of this scoping. Consequence: `junit:junit` cannot be
dropped from the parent POM yet. CI is green on all 6 checks.
## Follow-up plan to fully remove JUnit 4
Across the repo there are ~246 JUnit-4 test files left, including 89
`@RunWith(Parameterized.class)` classes. Plan is **leaves first, root last** —
opposite of what #4004/#4059 attempted:
1. ✅ this PR — `amoro-common` self-contained tests
2. `amoro-optimizer-common` (5 files, smallest leaf, validates the pattern
with reviewers)
3. `amoro-format-iceberg` (55 files; this is where the `Parameterized`
rewrite pattern gets exercised, plus the iceberg side of
`TestAmoroCatalogBase`/`TestServerTableDescriptor` consumers)
4. `amoro-format-mixed-hive` (23)
5. `amoro-format-mixed-spark` 3.3 / 3.4 / 3.5 (~13)
6. `amoro-format-mixed-flink-common` (52, includes converting the
`AmoroRunListener` surefire wiring to a JUnit 5 `TestExecutionListener`
simultaneously)
7. `amoro-format-paimon` + `amoro-mixed-trino` + `amoro-openapi-sdk` +
`amoro-mixed-spark-3-common` (~8)
8. `amoro-ams` (81, largest)
9. **Closing PR**: convert the 6 `amoro-common` helpers (`TestAms`,
`TestHMS`, `AmoroCatalogTestBase`, `TestAmoroCatalogBase`,
`TestServerTableDescriptor`, `AmoroRunListener`) to JUnit 5 Extensions / plain
classes, then remove `junit:junit` and `junit-vintage-engine` from the parent
POM.
Each step keeps CI green because the helpers stay on JUnit 4 until the very
last consumer is gone. Happy to keep driving the chain if maintainers are OK
with this approach — let me know if you'd prefer a different ordering, or if
combining some steps would be easier to review.
--
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]
