Skip to content

[AMORO-3974][common] Migrate self-contained tests from JUnit 4 to JUnit 5#4199

Open
lintingbin wants to merge 1 commit intoapache:masterfrom
lintingbin:fix-3974-junit5-migration
Open

[AMORO-3974][common] Migrate self-contained tests from JUnit 4 to JUnit 5#4199
lintingbin wants to merge 1 commit intoapache:masterfrom
lintingbin:fix-3974-junit5-migration

Conversation

@lintingbin
Copy link
Copy Markdown
Contributor

What is this PR for?

Closes part of #3974.

Replaces JUnit 4 (org.junit.*) imports with JUnit 5 (org.junit.jupiter.api.*) in the self-contained unit tests of the amoro-common module:

  • TestLocalExecutionEngine
  • TestSimpleFuture
  • JacksonUtilTest
  • MemorySizeTest
  • TestPoolConfig
  • MockZookeeperServer

@Test(expected=...) is replaced by Assertions.assertThrows(...). @Before/@After become @BeforeEach/@AfterEach. JUnit 4's Assert.assertX("msg", expected, actual) is rewritten as JUnit 5's Assertions.assertX(expected, actual, "msg"). MemorySizeTest previously used Hamcrest assertThat(x, is(y)) (pulled in transitively by JUnit 4); those checks are rewritten with Assertions.assertEquals.

What is the limitation?

This PR is intentionally scoped: shared test bases and rule helpers in amoro-common are not migrated, because they are still extended (or referenced as @Rule/@ClassRule) by JUnit 4 tests in other modules. Migrating them in isolation would break those modules. The remaining files are:

  • TestAms (extends org.junit.rules.ExternalResource) — used as @Rule/@ClassRule in amoro-ams, amoro-format-iceberg, amoro-format-mixed, amoro-format-paimon, amoro-optimizer-common, amoro-openapi-sdk.
  • TestHMS (extends ExternalResource, owns a JUnit 4 TemporaryFolder) — same situation.
  • AmoroCatalogTestBase and TestAmoroCatalogBase — abstract bases extended by TestIcebergAmoroCatalog, TestMixedIcebergFormatCatalog, TestPaimonAmoroCatalog, etc. that use @RunWith(Parameterized.class).
  • TestServerTableDescriptor — abstract base extended by TestIcebergServerTableDescriptor and TestPaimonServerTableDescriptor, both using @RunWith(Parameterized.class).
  • AmoroRunListener (extends org.junit.runner.notification.RunListener) — wired into the surefire <properties> of amoro-mixed-flink-common as a JUnit 4 listener.

These will be migrated together with their consumers in a follow-up so the junit:junit dependency can be dropped from the parent POM.

Type of change

  • Improvement (test infrastructure)

How was this patch tested?

./mvnw -pl amoro-common test — all 96 tests pass under the JUnit Platform provider.

…it 5

Migrate the JUnit 4 test classes in amoro-common that have no
external JUnit 4 consumers to JUnit 5 (org.junit.jupiter):

- TestLocalExecutionEngine
- TestSimpleFuture
- JacksonUtilTest
- MemorySizeTest
- TestPoolConfig
- MockZookeeperServer

Shared test base classes and rule helpers (TestAms, TestHMS,
AmoroCatalogTestBase, TestAmoroCatalogBase, TestServerTableDescriptor,
AmoroRunListener) are kept on JUnit 4 because they are still extended
or referenced as @Rule/@ClassRule by JUnit 4 tests in other modules
(amoro-ams, amoro-format-iceberg, amoro-format-paimon,
amoro-format-mixed-flink, etc.). They will be migrated together with
those consumers in a follow-up so the JUnit 4 dependency can be dropped.
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 1, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 29.88%. Comparing base (ec2a448) to head (6345254).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4199      +/-   ##
============================================
- Coverage     29.88%   29.88%   -0.01%     
+ Complexity     4288     4287       -1     
============================================
  Files           679      679              
  Lines         55060    55066       +6     
  Branches       7032     7033       +1     
============================================
- Hits          16455    16454       -1     
- Misses        37375    37383       +8     
+ Partials       1230     1229       -1     
Flag Coverage Δ
core 29.88% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lintingbin
Copy link
Copy Markdown
Contributor Author

@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:

  • [AMORO-3974] Migrate JUnit 4 to JUnit 5 in amoro-common module #4004 rewrote the shared helpers (TestAms/TestHMS → plain classes; AmoroCatalogTestBase@TempDir/@BeforeEach; AmoroRunListenerTestExecutionListener). 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.
  • [AMORO-3974] Fix JUnit 5 migration logic in iceberg module #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.

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.

2 participants