test: migrate extension-core format and lookup tests to JUnit 5 - #19876
Conversation
github-actions
Bot
added
Area - Streaming Ingestion
Area - Lookups
Area - Dependencies
labels
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| Assertions.assertEquals(150000, config.getMaxRowsInMemory()); | ||
| Assertions.assertEquals(5_000_000, config.getMaxRowsPerSegment().intValue()); | ||
| Assertions.assertEquals(new Period("PT10M"), config.getIntermediatePersistPeriod()); | ||
| Assertions.assertEquals(0, config.getMaxPendingPersists()); |
There was a problem hiding this comment.
Evaluated as baseline: this deprecated getMaxPendingPersists call already existed on master before this JUnit migration. This PR only changes test framework usage, so deprecated API cleanup is separate scope and is left unchanged.
| Assertions.assertEquals(100, config.getMaxRowsInMemory()); | ||
| Assertions.assertEquals(100, config.getMaxRowsPerSegment().intValue()); | ||
| Assertions.assertEquals(new Period("PT1H"), config.getIntermediatePersistPeriod()); | ||
| Assertions.assertEquals(100, config.getMaxPendingPersists()); |
There was a problem hiding this comment.
Evaluated as baseline: this deprecated getMaxPendingPersists call already existed on master before this JUnit migration. This PR only changes test framework usage, so deprecated API cleanup is separate scope and is left unchanged.
| Assertions.assertEquals(base.getMaxTotalRows(), deserialized.getMaxTotalRows()); | ||
| Assertions.assertEquals(base.getIntermediatePersistPeriod(), deserialized.getIntermediatePersistPeriod()); | ||
| Assertions.assertNull(deserialized.getBasePersistDirectory()); | ||
| Assertions.assertEquals(base.getMaxPendingPersists(), deserialized.getMaxPendingPersists()); |
There was a problem hiding this comment.
Evaluated as baseline: this deprecated getMaxPendingPersists call already existed on master before this JUnit migration. This PR only changes test framework usage, so deprecated API cleanup is separate scope and is left unchanged.
| Assertions.assertEquals(base.getMaxTotalRows(), deserialized.getMaxTotalRows()); | ||
| Assertions.assertEquals(base.getIntermediatePersistPeriod(), deserialized.getIntermediatePersistPeriod()); | ||
| Assertions.assertNull(deserialized.getBasePersistDirectory()); | ||
| Assertions.assertEquals(base.getMaxPendingPersists(), deserialized.getMaxPendingPersists()); |
There was a problem hiding this comment.
Evaluated as baseline: this deprecated getMaxPendingPersists call already existed on master before this JUnit migration. This PR only changes test framework usage, so deprecated API cleanup is separate scope and is left unchanged.
| Assertions.assertEquals(base.getMaxTotalRows(), deserialized.getMaxTotalRows()); | ||
| Assertions.assertEquals(base.getIntermediatePersistPeriod(), deserialized.getIntermediatePersistPeriod()); | ||
| Assertions.assertNull(deserialized.getBasePersistDirectory()); | ||
| Assertions.assertEquals(base.getMaxPendingPersists(), deserialized.getMaxPendingPersists()); |
There was a problem hiding this comment.
Evaluated as baseline: this deprecated getMaxPendingPersists call already existed on master before this JUnit migration. This PR only changes test framework usage, so deprecated API cleanup is separate scope and is left unchanged.
| Assertions.assertEquals(base.getMaxTotalRows(), deserialized.getMaxTotalRows()); | ||
| Assertions.assertEquals(base.getIntermediatePersistPeriod(), deserialized.getIntermediatePersistPeriod()); | ||
| Assertions.assertNull(deserialized.getBasePersistDirectory()); | ||
| Assertions.assertEquals(base.getMaxPendingPersists(), deserialized.getMaxPendingPersists()); |
There was a problem hiding this comment.
Evaluated as baseline: this deprecated getMaxPendingPersists call already existed on master before this JUnit migration. This PR only changes test framework usage, so deprecated API cleanup is separate scope and is left unchanged.
| Assertions.assertEquals(100L, (long) copy.getMaxTotalRows()); | ||
| Assertions.assertEquals(new Period("PT3S"), copy.getIntermediatePersistPeriod()); | ||
| Assertions.assertNull(copy.getBasePersistDirectory()); | ||
| Assertions.assertEquals(4, copy.getMaxPendingPersists()); |
There was a problem hiding this comment.
Evaluated as baseline: this deprecated getMaxPendingPersists call already existed on master before this JUnit migration. This PR only changes test framework usage, so deprecated API cleanup is separate scope and is left unchanged.
| Assertions.assertEquals(150000, config.getMaxRowsInMemory()); | ||
| Assertions.assertEquals(5_000_000, config.getMaxRowsPerSegment().intValue()); | ||
| Assertions.assertEquals(new Period("PT10M"), config.getIntermediatePersistPeriod()); | ||
| Assertions.assertEquals(0, config.getMaxPendingPersists()); |
There was a problem hiding this comment.
Evaluated as baseline: this deprecated getMaxPendingPersists call already existed on master before this JUnit migration. This PR only changes test framework usage, so deprecated API cleanup is separate scope and is left unchanged.
| Assertions.assertEquals(100, config.getMaxRowsInMemory()); | ||
| Assertions.assertEquals(100, config.getMaxRowsPerSegment().intValue()); | ||
| Assertions.assertEquals(new Period("PT1H"), config.getIntermediatePersistPeriod()); | ||
| Assertions.assertEquals(100, config.getMaxPendingPersists()); |
There was a problem hiding this comment.
Evaluated as baseline: this deprecated getMaxPendingPersists call already existed on master before this JUnit migration. This PR only changes test framework usage, so deprecated API cleanup is separate scope and is left unchanged.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 1 |
| Total | 3 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 1 |
| Total | 3 |
Reviewed 54 of 54 changed files.
This is an automated review by Codex GPT-5.6-Sol
| { | ||
| @Override | ||
| public String getStorageDirectory() | ||
| Throwable exception = assertThrows(IllegalStateException.class, () -> { |
There was a problem hiding this comment.
[P2] Limit the exception assertion to killAll
The assertThrows lambda now includes filesystem setup, the successful killer.kill(...) path, its assertions, cleanup, and killAll(). If an earlier operation starts throwing the same IllegalStateException, this test passes early, skips the behavior it was meant to verify, and can leave /tmp/dataSourceNew behind. Keep setup, kill, assertions, and cleanup outside the lambda and wrap only killer.killAll().
There was a problem hiding this comment.
Fixed in cc8fcd7. Filesystem setup, kill, assertions, and cleanup remain outside assertThrows; only killer.killAll() is asserted. HdfsDataSegmentKillerTest passed all 10 tests.
| .configuration(CONFIGURATION) | ||
| .inputSourceConfig(DEFAULT_INPUT_SOURCE_CONFIG) | ||
| .build(); | ||
| Throwable exception = assertThrows(IllegalArgumentException.class, () -> { |
There was a problem hiding this comment.
[P2] Keep the valid HDFS case outside assertThrows
The lambda first builds a valid hdfs input source and then the invalid file: source. If the valid construction regresses and throws the expected IllegalArgumentException, the assertion succeeds before the invalid protocol is tested. Build the allowed source before assertThrows and put only the rejected file: construction inside it.
There was a problem hiding this comment.
Fixed in cc8fcd7. The valid hdfs source is built before assertThrows, and the lambda contains only the rejected file: source construction. HdfsInputSourceTest passed all 35 nested tests.
| } | ||
|
|
||
| @After | ||
| @AfterEach |
There was a problem hiding this comment.
[P3] Close each generated off-heap cache
Each parameterized method now invokes inputData() separately and creates a fresh OffHeapLoadingCache, but teardown only calls invalidateAll(). close() is what deletes the cache map from the static direct-memory MapDB, so this migration multiplies retained off-heap maps until JVM shutdown. Close the current cache in @AfterEach.
There was a problem hiding this comment.
Fixed in cc8fcd7. tearDown now calls close() after invalidateAll(), releasing each off-heap cache map. LoadingCacheTest passed all 18 tests.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the updated code for correctness, edge cases, concurrency, and integration risks; no issues found.
Reviewed 54 of 54 changed files. All three prior findings are resolved: both exception assertions are scoped to the rejected operation, and each generated off-heap cache is closed during teardown.
This is an automated review by Codex GPT-5.6-Sol
There was a problem hiding this comment.
Pull request overview
This PR continues the extension-core JUnit 4 → JUnit 5 migration (#13948), updating tests across multiple extensions to run on JUnit Jupiter (including assertions, lifecycle annotations, parameterized tests, and temp-dir support), while keeping transitional JUnit 4 dependencies only where shared fixtures still require them.
Changes:
- Migrate Parquet extension tests to JUnit 5 (Jupiter assertions +
@TempDir) and update the module test dependencies. - Migrate cached-single lookup tests to JUnit 5, converting JUnit4 parameterized tests/rules to Jupiter parameterized tests and
assertThrows. - Migrate HDFS storage and Kinesis indexing-service tests to JUnit 5, including replacing JUnit4 rules/timeouts/system-property rules with Jupiter equivalents.
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| extensions-core/parquet-extensions/src/test/java/org/apache/druid/data/input/parquet/WikiParquetReaderTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/parquet-extensions/src/test/java/org/apache/druid/data/input/parquet/TimestampsParquetReaderTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/parquet-extensions/src/test/java/org/apache/druid/data/input/parquet/ParquetToJsonTest.java | Replace TemporaryFolder with @TempDir and migrate assertions to Jupiter. |
| extensions-core/parquet-extensions/src/test/java/org/apache/druid/data/input/parquet/ParquetReaderResourceLeakTest.java | Replace TemporaryFolder rule with @TempDir and migrate assertions to Jupiter. |
| extensions-core/parquet-extensions/src/test/java/org/apache/druid/data/input/parquet/ParquetInputFormatTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/parquet-extensions/src/test/java/org/apache/druid/data/input/parquet/NestedColumnParquetReaderTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/parquet-extensions/src/test/java/org/apache/druid/data/input/parquet/FlattenSpecParquetReaderTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/parquet-extensions/src/test/java/org/apache/druid/data/input/parquet/DecimalParquetReaderTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/parquet-extensions/src/test/java/org/apache/druid/data/input/parquet/CompatParquetReaderTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/parquet-extensions/pom.xml | Update test dependencies to include JUnit Jupiter artifacts. |
| extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/PollingLookupTest.java | Convert JUnit4 parameterized/rule usage to Jupiter parameterized tests and lifecycle. |
| extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/PollingLookupSerDeserTest.java | Convert JUnit4 parameterized test to Jupiter parameterized test for serde coverage. |
| extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/PollingLookupFactoryTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java | Replace JUnit4 rule-based exception checks with Jupiter assertions. |
| extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupFactoryTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/jdbc/JdbcDataFetcherUrlCheckTest.java | Replace ExpectedException rule with assertThrows and convert nested suites to @Nested. |
| extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/jdbc/JdbcDataFetcherTest.java | Replace JUnit4 rules with explicit setup/teardown and Jupiter lifecycle + assertions. |
| extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/CacheRefKeeperTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/cache/loading/OnHeapLoadingCacheTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/cache/loading/OffHeapLoadingCacheTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/cache/loading/LoadingCacheTest.java | Convert JUnit4 parameterized tests to Jupiter parameterized tests and lifecycle. |
| extensions-core/lookups-cached-single/pom.xml | Add JUnit Jupiter test dependencies while keeping JUnit4 for transitional TestDerbyConnector usage. |
| extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTuningConfigTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfigTest.java | Replace rule-based exception assertions with assertThrows and Jupiter assertions. |
| extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisSamplerSpecTest.java | Replace JUnit4 timeout with Jupiter @Timeout and migrate assertions. |
| extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisRegionTest.java | Replace @Before/expected-exception usage with Jupiter lifecycle + assertThrows. |
| extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisRecordSupplierTest.java | Replace JUnit4 lifecycle/expected-exception usage with Jupiter lifecycle + assertThrows. |
| extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisIOConfigTest.java | Replace rule-based exception assertions with assertThrows and migrate assertions. |
| extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisIndexTaskTuningConfigTest.java | Replace rule-based exception assertions with assertThrows and migrate assertions. |
| extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisIndexTaskSerdeTest.java | Replace system-rules property rule with explicit Jupiter setup/teardown for system properties. |
| extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisDataSourceMetadataTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/kinesis-indexing-service/pom.xml | Add JUnit Jupiter test dependencies; retain JUnit4 where shared fixtures still use rules. |
| extensions-core/hdfs-storage/src/test/java/org/apache/druid/storage/hdfs/HdfsStorageDruidModuleTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/hdfs-storage/src/test/java/org/apache/druid/storage/hdfs/HdfsKerberosConfigTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/hdfs-storage/src/test/java/org/apache/druid/storage/hdfs/HdfsDataSegmentPusherTest.java | Replace JUnit4 rules with @TempDir + assertThrows and migrate assertions. |
| extensions-core/hdfs-storage/src/test/java/org/apache/druid/storage/hdfs/HdfsDataSegmentPullerTest.java | Replace JUnit4 class/per-test lifecycle with Jupiter @BeforeAll/@AfterAll/@BeforeEach/@AfterEach. |
| extensions-core/hdfs-storage/src/test/java/org/apache/druid/storage/hdfs/HdfsDataSegmentKillerTest.java | Replace rule-based exception assertions with assertThrows and migrate assertions. |
| extensions-core/hdfs-storage/src/test/java/org/apache/druid/segment/loading/HdfsFileTimestampVersionFinderTest.java | Replace JUnit4 lifecycle annotations with Jupiter equivalents and migrate assertions. |
| extensions-core/hdfs-storage/src/test/java/org/apache/druid/inputsource/hdfs/HdfsInputSourceTest.java | Replace JUnit4 rules/parameterized structure with Jupiter @Nested, @TempDir, and assertThrows. |
| extensions-core/hdfs-storage/src/test/java/org/apache/druid/inputsource/hdfs/HdfsInputSourceConfigTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/hdfs-storage/src/test/java/org/apache/druid/inputsource/hdfs/HdfsInputSourceAdapterTest.java | Switch assertions/annotations from JUnit 4 to JUnit 5. |
| extensions-core/hdfs-storage/src/test/java/org/apache/druid/indexing/common/tasklogs/HdfsTaskLogsTest.java | Replace TemporaryFolder rule with @TempDir and migrate assertions. |
| extensions-core/hdfs-storage/pom.xml | Update test dependencies to include JUnit Jupiter artifacts. |
| extensions-core/avro-extensions/src/test/java/org/apache/druid/data/input/AvroStreamInputFormatTest.java | Replace JUnit4 lifecycle/exception assertions with Jupiter equivalents. |
| extensions-core/avro-extensions/src/test/java/org/apache/druid/data/input/avro/SchemaRegistryBasedAvroBytesDecoderTest.java | Replace JUnit4 lifecycle/exception assertions with Jupiter equivalents; keep Hamcrest assertions. |
| extensions-core/avro-extensions/src/test/java/org/apache/druid/data/input/avro/InlineSchemasAvroBytesDecoderTest.java | Replace JUnit4 exception assertions with Jupiter assertThrows and migrate assertions. |
| extensions-core/avro-extensions/src/test/java/org/apache/druid/data/input/avro/InlineSchemaAvroBytesDecoderTest.java | Replace JUnit4 exception assertions with Jupiter assertThrows and migrate assertions. |
| extensions-core/avro-extensions/src/test/java/org/apache/druid/data/input/avro/AvroOCFReaderTest.java | Replace TemporaryFolder rule with @TempDir and migrate assertions. |
| extensions-core/avro-extensions/src/test/java/org/apache/druid/data/input/avro/AvroOCFInputFormatTest.java | Replace JUnit4 lifecycle with Jupiter lifecycle and migrate assertions. |
| extensions-core/avro-extensions/pom.xml | Update test dependencies to include JUnit Jupiter artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <dependency> | ||
| <groupId>org.junit.jupiter</groupId> | ||
| <artifactId>junit-jupiter</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Fixed in commit 4676b86 by normalizing the newly added JUnit dependency block to the surrounding two-space POM indentation. Checkstyle passed with 0 violations, and the module-scoped SpotBugs check passed with 0 bugs/errors.
| @JsonCreator | ||
| public MockDataFetcher() | ||
| public void initPollingLookupSerDeserTest() | ||
| { | ||
| } |
There was a problem hiding this comment.
Actionable. The JUnit migration had converted the original no-argument MockDataFetcher constructor into an instance void method while retaining @JsonCreator. I restored it as public MockDataFetcher(), so @JsonCreator now targets a valid constructor and the intended Jackson subtype behavior is preserved. PollingLookupSerDeserTest passed both parameterized cases (2 tests, 0 failures); Checkstyle and SpotBugs also passed.
| <dependency> | ||
| <groupId>org.junit.jupiter</groupId> | ||
| <artifactId>junit-jupiter</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Fixed in commit 4676b86 by normalizing the newly added JUnit dependency block to the surrounding four-space POM indentation. Checkstyle passed with 0 violations, and the module-scoped SpotBugs check passed with 0 bugs/errors.
Part of #13948.
Builds on PR #19875.
Description
This PR migrates the JUnit 4 tests in the second extension-core batch to JUnit 5:
The tests now use Jupiter assertions, lifecycle annotations, parameterized tests, nested tests, and temporary-directory support. Transitional JUnit 4 test dependencies remain only where migrated tests inherit shared JUnit 4 fixtures from indexing-service or use the shared TestDerbyConnector utility; those dependencies can be removed after the shared fixtures are migrated.
Validation
Ran the complete changed-test set for all five modules with Maven. The reactor completed successfully with no failures, errors, or flakes.
Related migration PR