-
Notifications
You must be signed in to change notification settings - Fork 368
[AMORO-3974] Migrate JUnit 4 to JUnit 5 in amoro-common module #4004
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Thank you for your contribution to the new unit test support. You can run |
|
I’ve completed the code formatting. |
|
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 |
|
Thank you for the clear direction! I'll proceed with refactoring to align with the JUnit 5 approach used in this repository. cc @czy006 |
|
CI Error |
|
test is failing cc @Jungkihong07 |
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-commonmodule 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):
AmoroRunListener.java- ConvertedRunListener→TestExecutionListenerTestServerTableDescriptor.java- Converted@Rule,@ClassRule→@BeforeAll/@AfterAlland@TempDirMemorySizeTest.java- Converted@Test(expected=...)→assertThrows()JacksonUtilTest.javaTestSimpleFuture.javaTestAmoroCatalogBase.javaAmoroCatalogTestBase.javaTestPoolConfig.javaMockZookeeperServer.javaKey Changes:
org.junit.*→org.junit.jupiter.api.*@Before/After→@BeforeEach/AfterEach,@BeforeClass/AfterClass→@BeforeAll/AfterAll(static)Assert.*→Assertions.*@Test(expected=...)→Assertions.assertThrows()@Rule→@RegisterExtensionor@TempDirAmoroRunListenerconversionDetailed Implementation:
Assert.*calls withAssertions.*, updated imports fromorg.junit.*toorg.junit.jupiter.api.*@Before,@After) and related methods (setupCatalogJUnit4(),cleanCatalogJUnit4()), simplified to use only JUnit 5 lifecycle annotations (@BeforeEach,@AfterEach) with@TempDirfor temporary directory managementextends ExternalResource(JUnit 4 Rule), converted to plain Java class, replacedTemporaryFolderwithjava.nio.file.FilesandPathfor temporary directory handlingextends ExternalResource(JUnit 4 Rule), converted to plain Java classMigration Strategy:
createHelper()factory method pattern inAmoroCatalogTestBasefor JUnit 5 test classesHow was this patch tested?
amoro-commonmodule:./mvnw test -pl amoro-common./mvnw clean install -pl amoro-common -amDocumentation
Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable)