Skip to content

test: migrate server tests to JUnit 5 - #19911

Open
FrankChen021 wants to merge 4 commits into
apache:masterfrom
FrankChen021:codex/tmp-junit5-server
Open

test: migrate server tests to JUnit 5#19911
FrankChen021 wants to merge 4 commits into
apache:masterfrom
FrankChen021:codex/tmp-junit5-server

Conversation

@FrankChen021

Copy link
Copy Markdown
Member

Summary

  • Migrates the server test batch from JUnit 4 to JUnit 5, including lifecycle, rule, parameterization, and Mockito integrations.
  • Replaces Hamcrest assertions and matchers in the server scope with Jupiter assertions, AssertJ, and server-local JUnit 5 helpers for exception and log capture.
  • Removes unused JUnit 4, Vintage, migration-support, Hamcrest, Jersey test-framework, and transitive Truth compatibility paths from server/pom.xml.
  • Moves the server QueryStack group-by factory off the legacy processing GroupByQueryRunnerTest.
  • Reconciles the batch with the latest apache/druid/master; the complete diff is limited to server/** (411 files).

Validation

  • mvn -ntp -pl server test-compile -DskipTests -Dweb.console.skip=true — passed.
    • Checkstyle: 0 violations.
    • PMD: passed.
    • Enforcer: passed.
    • Forbidden APIs: 0 errors in main classes and 0 errors in test classes.
  • Targeted server tests — 559 tests, 0 failures, 0 errors, 13 skipped.
  • QueryStack consumers QuerySchedulerTest (22) and DirectDruidClientTest (12) — passed.
  • mvn -ntp -pl server spotbugs:check -DskipTests -Dweb.console.skip=true — passed; 0 BugInstances, 0 errors.
  • Effective server test dependency tree — no junit:junit, Vintage, JUnit migration-support, Hamcrest, Jersey test-framework, or Truth artifacts.
  • Full server Java audit, including static imports — no JUnit 4 or Hamcrest imports/usages.
  • ./dev/validate-junit5-usage — passed; server reports hasj4: 0 and hasVintage: 0.
  • git diff --check — passed.

Shared fixture stacking prerequisite

#19875 currently contains older copies of:

  • server/src/test/java/org/apache/druid/server/QueryStackTests.java
  • server/src/test/java/org/apache/druid/metadata/TestDerbyConnector.java

This PR is the sole owner of the finalized server versions. Before merging either PR, rebase #19875 to drop those two server paths (or apply the equivalent stack/rebase ordering); do not retain divergent copies in both PRs.

The server CachingClusteredClientTest also consumes processing/src/test/java/org/apache/druid/segment/TestHelper.java. On current master that shared fixture still calls JUnit 4 org.junit.Assert; with the cleaned server classpath, the test currently fails with NoClassDefFoundError: org/junit/Assert (262 tests attempted, 150 errors). #19875 contains the JUnit 5 processing-fixture migration. After #19875 drops its two older server paths, its processing fixture changes must land or be stacked/rebased before CachingClusteredClientTest can run. This is the narrow prerequisite; this PR intentionally retains no JUnit 4/Hamcrest compatibility dependencies.

Part of #13948

Prior migration PRs: #19875, #19876, #19877, #19878, #19879, #19880.

Copilot AI lite review requested due to automatic review settings August 6, 2026 22:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@clintropolis

Copy link
Copy Markdown
Member

imo a 12k line PR is kind of hard to review, not to mention disruptive to any open PR that touches a test in the entire module (this is part of why i have broken up druid-processing into several smaller migrations)

@FrankChen021

Copy link
Copy Markdown
Member Author

imo a 12k line PR is kind of hard to review, not to mention disruptive to any open PR that touches a test in the entire module (this is part of why i have broken up druid-processing into several smaller migrations)

You are right. I didn't expect such a large change. I already asked Codex to analyze and split changes into multiple PRs based on modules. Codex is still monitoring the CI status of these PRs and will apply fixes automatically until CI is green. After that let's split the changes into multiple PRs again.

@FrankChen021

Copy link
Copy Markdown
Member Author

Expanded JUnit 5 migration scope and evidence

This update preserves the existing server migration in #19911 and adds the dependency-coupled services and embedded-tests test sources/POMs. The final PR boundary is server/**, services/**, and embedded-tests/** only. The embedded changes from commits adac0cc117 and 1869c51f68 were reused by filtering to embedded-tests/**; no multi-stage-query/** files were included.

Shared server test fixtures consumed by these modules, including TestDerbyConnector and LatchableEmitter, no longer expose JUnit 4 or Hamcrest types in their test-jar APIs. No JUnit 4/Hamcrest compatibility dependencies were reintroduced. Direct legacy JUnit4, Vintage, migration-support, JUnitParams, system-rules, and Hamcrest dependencies were removed from the services/embedded POMs once their imports were gone.

Scoped validation

  • mvn -ntp -pl server,services,embedded-tests test-compile -DskipTests -Dweb.console.skip=true -T1C — passed with checkstyle (0 violations), PMD, Enforcer, forbidden-API checks, and test compilation.
  • mvn -ntp -pl server,services,embedded-tests spotbugs:check -DskipTests -Dweb.console.skip=true -T1C — passed; server and services reported 0 BugInstances/0 errors, and embedded-tests has no main classes for SpotBugs.
  • Java/POM audits — no legacy org.junit (outside org.junit.jupiter), org.hamcrest, JUnit4 rule, or obsolete direct dependency matches under the three scoped modules.
  • Services tests — 228 tests passed when the Docker-only TestcontainerResourceTest was excluded. The complete invocation had exactly one environment error: Testcontainers could not find a valid Docker environment.
  • Reactor smoke test using the current server test-jar, CompactionTaskTest — 8 tests passed; no Hamcrest linkage error.
  • The project JUnit5 usage audit reports zero JUnit4/Vintage hits for server, services, and embedded-tests. Its remaining hits are in unrelated modules such as SQL, processing, indexing-service, MSQ, and extensions.

Known environment/baseline blockers

  • The full server run completed 3,906 tests with 1 failure and 26 skips. The failure is SegmentLoadingTest.testMaxSegmentsInNodeLoadingQueue at line 300 (CANCELLED_ACTIONS: expected 2, observed 0), reproduced in isolation with all four Surefire retries. Its migration diff is limited to JUnit imports/assertions and @BeforeEach; no migration-caused fixture change was identified, so it was not modified.
  • A broader services -am test compile reaches the un-migrated SQL tests and fails at BrokerSegmentMetadataCacheConcurrencyTest.java:107 and BrokerSegmentMetadataCacheTest.java:803 because temporaryFolder is now a java.io.File without newFolder(). This is outside this PR's scope.
  • A direct embedded-only full-test invocation is not reliable evidence in this worktree because it resolves the stale installed server test-jar; it reported NoClassDefFoundError: org/hamcrest/Matchers from that artifact along with Docker errors. The reactor compile/smoke result above uses the current server fixture. Docker-backed embedded tests remain unavailable here.

This PR is part of issue #13948 and is related to #19875, #19908, #19909, and #19910.

Published commit: 5e4efcfc5b.

@FrankChen021

Copy link
Copy Markdown
Member Author

CI follow-up for #13948

CI exposed that CliPeonTest.makePeonInjectorWithStubEmitter accepted the indexing-service TempDirExtension, leaking that test-only type to migrated consumers such as Kafka. This commit changes the helper to accept java.io.File, uses Jupiter @TempDir File in CliPeonTest, and writes the fixed task.json path under the supplied directory. No indexing-service, Kafka, SQL, MSQ, processing, or extensions files were changed; no JUnit4/Hamcrest compatibility was added.

Validation:

  • mvn -ntp -pl services -Dtest=org.apache.druid.cli.CliPeonTest -Dsurefire.failIfNoSpecifiedTests=false -Pskip-static-checks -Dweb.console.skip=true test: 11 tests, 0 failures/errors.
  • Module verify: Checkstyle 0, PMD pass, forbidden-API compile/test checks 0, Enforcer pass.
  • SpotBugs: 0 bugs/errors.
  • Scoped source audit: no JUnit4/Hamcrest imports in services; the changed file is the only file in this commit.
  • The broader -pl services -am reactor is blocked by two unrelated existing SQL test compile errors where temporaryFolder is a File but calls newFolder(); SQL was intentionally out of scope.

Commit: 64f2e3c0a5

@FrankChen021

Copy link
Copy Markdown
Member Author

CI dependency update

The fresh run on head 64f2e3c shows the helper boundary is fixed: no missing TempDirExtension or CliPeonTest type errors remain. The remaining indexing-service test errors are the expected merge-order boundary: unchanged indexing tests on master still annotate the server TestDerbyConnector JUnit5 callback with JUnit4 @rule, producing InvalidTestClass errors. SQL QTest jobs independently stop at the known temporaryFolder.newFolder() calls in the un-migrated SQL batch.

Please merge/rebase #19910 before evaluating this PR’s full reactor checks; #19910 owns the indexing consumers, while #19911 owns the server/services/embedded fixture migration. No compatibility dependency was reintroduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants