[KYUUBI #7473] Migrate Java tests from JUnit 4 to JUnit 5#7474
[KYUUBI #7473] Migrate Java tests from JUnit 4 to JUnit 5#7474wangzhigang1999 wants to merge 4 commits into
Conversation
wangzhigang1999
force-pushed
the
kyuubi-7473-junit5-migration
branch
from
70644df to
ac9b18c
Compare
- Add junit-bom 5.11.4 to root dependencyManagement; keep junit 4.13.2 for vendored Hive beeline tests under org.apache.hive.beeline. - Switch kyuubi-rest-client, kyuubi-util, kyuubi-hive-jdbc and kyuubi-data-agent-engine to junit-jupiter only. - Rewrite imports and annotations (@test, @before -> @beforeeach, ...), swap message argument from msg-first to msg-last, convert @RunWith(Parameterized.class) to @ParameterizedTest with @MethodSource / @valuesource, replace @test(expected=...) with assertThrows, and convert Assume.assumeTrue to Assumptions.assumeTrue. - Use static imports throughout per google-java-format (no class- qualified Assertions.X calls, no wildcard imports). - kyuubi-hive-beeline stays on JUnit 4 to avoid diverging from the vendored Apache Hive beeline tests.
wangzhigang1999
force-pushed
the
kyuubi-7473-junit5-migration
branch
from
ac9b18c to
5fa8033
Compare
Replace assertTrue(expr.equals(literal)) with assertEquals(literal, expr) in tests touched by the JUnit 5 migration, plus one assertTrue(x == 100) to assertEquals(100, x). Pure cleanup, no behavior change. Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-7
|
Done — |
- Replace assumeTrue(!X.isEmpty()) with assumeFalse(X.isEmpty()) (review feedback from @aajisaka). - Replace assertTrue(x instanceof Y) with assertInstanceOf(Y.class, x), and use its return value to eliminate the redundant cast where the test then accesses Y-specific methods. - Replace try/catch + fail() patterns with assertThrows. - Replace Java native `assert` in tests (silently skipped without -ea) with proper JUnit assertions. - Replace assertEquals(true, X) / assertTrue(!X) / assertTrue(X.equals(Y)) with the canonical assertTrue / assertFalse / assertEquals forms. - Switch Assertions.X(...) call sites to the static import form for consistency with the rest of the migrated suite. - Replace a couple of wildcard imports (event.*, Mockito.*, java.io.*) with explicit imports.
|
@aajisaka thanks, pushed the assumeFalse(X.isEmpty(), ...) change at both call sites. While in there I also swept the migrated tests for a few related JUnit 5 idiom polish-ups:
All touched tests pass locally, including the live LLM suite. |
|
@wangzhigang1999, congrats and welcome to join us as a committer, you should have the write permission on this repo if you set your apache account properly, you can try to merge this PR via the script |
Thanks @pan3793! 🎉 Will set things up properly and try the merge script. |
|
@wangzhigang1999, we usually leave a comment after merging, like "merged to xxx/xxx", in case users may confuse the PR state as |
|
merged to master |
3 participants
Why are the changes needed?
Closes #7473.
JUnit 4 has been in maintenance mode since 4.13.2 (Feb 2021); modern Java test deps (Mockito 5, Testcontainers, etc.) target JUnit 5. Our usage is shallow (no
@Rule/@ClassRule/Theories), so the migration is mostly mechanical.Migrates all five modules with Java tests:
kyuubi-rest-client,kyuubi-util,kyuubi-hive-jdbc,kyuubi-hive-beeline, andexternals/kyuubi-data-agent-engine. Drops the directjunit:junitdependency andjunit.versionproperty from the root pom; transitive exclusions are kept.junit-jupiteris pinned to the latest stable, 5.14.4.How was this patch tested?
build/mvn -Pfast test -pl <module> -amon each of the five modules — all green.kyuubi-data-agent-enginehas 7 live tests skipped viaAssumptions.assumeTrue(require an external LLM API).dev/reformatclean.Was this patch authored or co-authored using generative AI tooling?
Assisted-by: Claude:claude-opus-4-7