amoro icon indicating copy to clipboard operation
amoro copied to clipboard

[AMORO-3974] Migrate JUnit 4 to JUnit 5 in amoro-common module

Open Jungkihong07 opened this issue 2 weeks ago • 6 comments

Why are the changes needed?

JUnit 4 is no longer actively maintained, and JUnit 5 provides a more modern and powerful testing framework. This PR migrates all JUnit 4 tests in the amoro-common module to JUnit 5, enabling the use of the latest testing framework and reducing technical debt.

Close #3974.

Brief change log

JUnit 4 → JUnit 5 Migration for amoro-common Module

Migrated Test Files (9 files):

  1. AmoroRunListener.java - Converted RunListenerTestExecutionListener
  2. TestServerTableDescriptor.java - Converted @Rule, @ClassRule@BeforeAll/@AfterAll and @TempDir
  3. MemorySizeTest.java - Converted @Test(expected=...)assertThrows()
  4. JacksonUtilTest.java
  5. TestSimpleFuture.java
  6. TestAmoroCatalogBase.java
  7. AmoroCatalogTestBase.java
  8. TestPoolConfig.java
  9. MockZookeeperServer.java

Key Changes:

  • Import changes: org.junit.*org.junit.jupiter.api.*
  • Annotation changes: @Before/After@BeforeEach/AfterEach, @BeforeClass/AfterClass@BeforeAll/AfterAll (static)
  • Assertions changes: Assert.*Assertions.*
  • Exception testing: @Test(expected=...)Assertions.assertThrows()
  • Rules → Extensions: @Rule@RegisterExtension or @TempDir
  • RunListener → TestExecutionListener: AmoroRunListener conversion

Detailed Implementation:

  • TestAmoroCatalogBase.java: Replaced all Assert.* calls with Assertions.*, updated imports from org.junit.* to org.junit.jupiter.api.*
  • AmoroCatalogTestBase.java: Removed JUnit 4 annotations (@Before, @After) and related methods (setupCatalogJUnit4(), cleanCatalogJUnit4()), simplified to use only JUnit 5 lifecycle annotations (@BeforeEach, @AfterEach) with @TempDir for temporary directory management
  • TestHMS.java: Removed extends ExternalResource (JUnit 4 Rule), converted to plain Java class, replaced TemporaryFolder with java.nio.file.Files and Path for temporary directory handling
  • TestAms.java: Removed extends ExternalResource (JUnit 4 Rule), converted to plain Java class

Migration Strategy:

  • Maintained backward compatibility: constructor-based initialization still works for JUnit 4 test classes in other modules
  • Added createHelper() factory method pattern in AmoroCatalogTestBase for JUnit 5 test classes
  • No breaking changes to existing test infrastructure

How was this patch tested?

  • [x] All migrated test files executed and passed (91 tests total)
  • [x] Full test suite for amoro-common module: ./mvnw test -pl amoro-common
  • [x] Full build verification: ./mvnw clean install -pl amoro-common -am
  • [x] RAT check: 0 unapproved files (1765 licenses approved)
  • [x] Checkstyle: 0 violations
  • [x] Code coverage: JaCoCo report generated successfully (701 classes analyzed)

Documentation

  • Does this pull request introduce a new feature? (no)

  • If yes, how is the feature documented? (not applicable)

Jungkihong07 avatar Dec 09 '25 21:12 Jungkihong07

Thank you for your contribution to the new unit test support. You can run mvn spotless:apply to format code .It will be reviewed after the CI runs @Jungkihong07

czy006 avatar Dec 10 '25 06:12 czy006

I’ve completed the code formatting. A review would be appreciated when you get a chance. @czy006

Jungkihong07 avatar Dec 10 '25 14:12 Jungkihong07

I noticed that some tests in the amoro-format-iceberg module are failing with the following error:

No ParameterResolver registered for parameter [org.apache.amoro.formats.AmoroCatalogTestHelper arg0] in constructor [public org.apache.amoro.formats.TestIcebergAmoroCatalog(org.apache.amoro.formats.AmoroCatalogTestHelper)].

Similar errors occur for several test methods in TestMixedIcebergFormatCatalog, which causes the amoro-format-iceberg module tests to fail and the subsequent modules to be skipped.

My understanding is that, in JUnit 4, AmoroCatalogTestHelper was likely injected via a custom runner, rule, or test utility, but after migrating to JUnit 5 there is no ParameterResolver (or equivalent mechanism) configured for this helper.

Before making further changes, I would like to ask for guidance on the preferred approach in this project:

Should I introduce a JUnit 5 ParameterResolver (and register it via @ExtendWith or the service loader) to provide AmoroCatalogTestHelper?

Or would you prefer to refactor these tests to avoid constructor injection and use another pattern that is more consistent with the existing JUnit 5 tests in this repository?

Once I know the preferred direction, I can update the tests accordingly.

Jungkihong07 avatar Dec 10 '25 16:12 Jungkihong07

我注意到 amoro-format-iceberg 模块中的一些测试失败,并出现以下错误:

构造函数 [public org.apache.amoro.formats.TestIcebergAmoroCatalog(org.apache.amoro.formats.AmoroCatalogTestHelper)] 中,参数 [org.apache.amoro.formats.AmoroCatalogTestHelper arg0] 没有注册 ParameterResolver。

TestMixedIcebergFormatCatalog 中的几个测试方法出现类似错误,导致 amoro-format-iceberg 模块测试失败,后续模块被跳过。

我的理解是,在 JUnit 4 中,AmoroCatalogTestHelper 很可能是通过自定义运行器、规则或测试实用程序注入的,但在迁移到 JUnit 5 之后,没有为该辅助程序配置 ParameterResolver(或等效机制)。

在进行进一步修改之前,我想请教一下本项目的最佳方案:

我是否应该引入 JUnit 5 ParameterResolver(并通过以下方式注册它)?@ExtendWith或者服务加载器)提供 AmoroCatalogTestHelper?

或者您是否更倾向于重构这些测试,以避免构造函数注入,并使用与此存储库中现有的 JUnit 5 测试更一致的其他模式?

一旦我知道最佳方向,我就可以相应地更新测试。

I noticed that some tests in the amoro-format-iceberg module are failing with the following error:

No ParameterResolver registered for parameter [org.apache.amoro.formats.AmoroCatalogTestHelper arg0] in constructor [public org.apache.amoro.formats.TestIcebergAmoroCatalog(org.apache.amoro.formats.AmoroCatalogTestHelper)].

Similar errors occur for several test methods in TestMixedIcebergFormatCatalog, which causes the amoro-format-iceberg module tests to fail and the subsequent modules to be skipped.

My understanding is that, in JUnit 4, AmoroCatalogTestHelper was likely injected via a custom runner, rule, or test utility, but after migrating to JUnit 5 there is no ParameterResolver (or equivalent mechanism) configured for this helper.

Before making further changes, I would like to ask for guidance on the preferred approach in this project:

Should I introduce a JUnit 5 ParameterResolver (and register it via @ExtendWith or the service loader) to provide AmoroCatalogTestHelper?

Or would you prefer to refactor these tests to avoid constructor injection and use another pattern that is more consistent with the existing JUnit 5 tests in this repository?

Once I know the preferred direction, I can update the tests accordingly.

If it were me, I would prefer to refactor it. It seems that Junit5 has made some new incompatible changes to ParameterResolver, so it's also feasible for us to achieve equivalent effects in a new way cc @Jungkihong07

czy006 avatar Dec 13 '25 07:12 czy006

Thank you for the clear direction! I'll proceed with refactoring to align with the JUnit 5 approach used in this repository. cc @czy006

Jungkihong07 avatar Dec 13 '25 18:12 Jungkihong07

CI Error Error: TestMixedIcebergFormatCatalog.testListDatabases » ParameterResolution No ParameterResolver registered for parameter [org.apache.amoro.formats.AmoroCatalogTestHelper<?> arg0] in constructor [public org.apache.amoro.formats.TestMixedIcebergFormatCatalog(org.apache.amoro.formats.AmoroCatalogTestHelper<?>)]. cc @Jungkihong07

czy006 avatar Dec 16 '25 06:12 czy006

test is failing cc @Jungkihong07

czy006 avatar Dec 23 '25 01:12 czy006