test: migrate datasketches extension tests to JUnit 5 - #19881
Conversation
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.
Reviewed 80 of 80 changed files.
This is an automated review by Codex GPT-5.6-Sol
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 80 of 80 changed files. The shared temporary-folder helper update preserves both legacy and Jupiter callers.
This is an automated review by Codex GPT-5.6-Sol
|
Fixed in 690542e: removed the unnecessary throws IOException declaration and import from BaseSketchBuildSegmentMetadataQueryTest.newFolder(...). Error Prone reported CheckedExceptionNotThrown during strict compilation because FileUtils.createTempDirInLocation(...) does not throw a checked exception. Targeted strict compilation for extensions-core/datasketches and its required modules passed with BUILD SUCCESS. |
There was a problem hiding this comment.
Pull request overview
This PR migrates the extensions-core/datasketches module’s test suite from JUnit 4 to JUnit 5 (Jupiter), updating assertions, parameterized tests, and temp-directory handling. It also extends the shared processing test-jar AggregationTestHelper to support JUnit 5 @TempDir callers while keeping JUnit 4 compatibility for remaining migrations.
Changes:
- Add JUnit 5-friendly temp-directory support to
AggregationTestHelpervia a provider abstraction and new*WithTempDirfactory methods. - Migrate DataSketches extension tests from JUnit 4 APIs (rules, lifecycle annotations, assertions) to JUnit 5 equivalents.
- Remove
junit-jupiter-migrationsupportand the Vintage engine from the DataSketches module POM, while retaining the JUnit 4 API dependency for compatibility with shared test-jar signatures.
Reviewed changes
Copilot reviewed 80 out of 80 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| processing/src/test/java/org/apache/druid/query/aggregation/AggregationTestHelper.java | Introduces a temp-folder provider and adds *WithTempDir helpers for Jupiter @TempDir. |
| extensions-core/datasketches/src/test/java/org/apache/druid/segment/DatasketchesProjectionTest.java | Converts parameterized projections test from JUnit 4 to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/util/ToObjectVectorColumnProcessorFactoryTest.java | Migrates lifecycle + assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchTTestPostAggregatorTest.java | Replaces ExpectedException with assertThrows and JUnit 5 assertions. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchToVariancesPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchToStringPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchToQuantilesSketchPostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchToNumEntriesPostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchToMetricsSumEstimatePostAggregatorTest.java | Migrates assertions / assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchToMeansPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchToEstimatePostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchToEstimateAndBoundsPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchToBase64StringPostAggregatorTest.java | Migrates assertions / assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchSetOpPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchOperationsTest.java | Migrates assertions / assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchObjectStrategyTest.java | Migrates assertions / assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchConstantPostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchAggregatorFactoryTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/ThetaSketchBuildSegmentMetadataQueryTest.java | Migrates segment metadata assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/sql/ThetaSketchSqlAggregatorTest.java | Migrates assertions/assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/SketchToStringPostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/SketchSetPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/SketchHolderTest.java | Migrates assertions/assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/SketchHolderObjectStrategyTest.java | Migrates assertions/assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/SketchEstimateWithErrorBoundsTest.java | Migrates serde test assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/SketchAggregatorFactoryTest.java | Migrates lifecycle + assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/SketchAggregationWithSimpleDataTest.java | Converts parameterized ingestion/query tests to JUnit 5 + @TempDir. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/oldapi/OldApiSketchAggregationTest.java | Converts old-API aggregation parameterized tests to JUnit 5 + @TempDir. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/BufferHashGrouperUsingSketchMergeAggregatorFactoryTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/quantiles/sql/DoublesSketchSqlAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/quantiles/DoublesSketchToStringPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/quantiles/DoublesSketchToRankPostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/quantiles/DoublesSketchToQuantilesPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/quantiles/DoublesSketchToQuantilePostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/quantiles/DoublesSketchToHistogramPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/quantiles/DoublesSketchToCDFPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/quantiles/DoublesSketchOperationsTest.java | Migrates assertions/assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/quantiles/DoublesSketchMergeAggregatorFactoryTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/quantiles/DoublesSketchComplexMetricSerdeTest.java | Migrates assertions/assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/quantiles/DoublesSketchAggregatorFactoryTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllFloatsSketchToStringPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllFloatsSketchToRankPostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllFloatsSketchToQuantilesPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllFloatsSketchToQuantilePostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllFloatsSketchToHistogramPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllFloatsSketchToCDFPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllFloatsSketchOperationsTest.java | Migrates assertions/assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllFloatsSketchMergeAggregatorFactoryTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllFloatsSketchComplexMetricSerdeTest.java | Migrates assertions/assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllFloatsSketchAggregatorFactoryTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllDoublesSketchToStringPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllDoublesSketchToRankPostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllDoublesSketchToQuantilesPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllDoublesSketchToQuantilePostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllDoublesSketchToHistogramPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllDoublesSketchToCDFPostAggregatorTest.java | JUnit 5 migration (assertThrows, assertions). |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllDoublesSketchOperationsTest.java | Migrates assertions/assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllDoublesSketchMergeAggregatorFactoryTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllDoublesSketchComplexMetricSerdeTest.java | Migrates assertions/assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/kll/KllDoublesSketchAggregatorFactoryTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchSqlAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchUnionPostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchToStringPostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchToEstimateWithBoundsPostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchToEstimatePostAggregatorTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchMergeAggregatorFactoryTest.java | Migrates lifecycle + exception tests to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchHolderTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchHolderObjectStrategyTest.java | Migrates assertions/assertThrows to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchBuildUtilTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchBuildSegmentMetadataQueryTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchBuildAggregatorFactoryTest.java | Migrates assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchAggregatorFactoryTest.java | Migrates lifecycle + assertions to JUnit 5. |
| extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/BaseSketchBuildSegmentMetadataQueryTest.java | Migrates to JUnit 5 @TempDir and assertions; updates temp dir creation. |
| extensions-core/datasketches/pom.xml | Removes Vintage/migrationsupport; retains JUnit 4 API dependency for shared test-jar signature compatibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try ( | ||
| final AggregationTestHelper toolchest = AggregationTestHelper.createGroupByQueryAggregationTestHelper( | ||
| final AggregationTestHelper toolchest = AggregationTestHelper.createGroupByQueryAggregationTestHelperWithTempDir( | ||
| sm.getJacksonModules(), | ||
| config, | ||
| tempFolder |
There was a problem hiding this comment.
Fixed in commit 6d8055b3f8. I removed the mutable config/vectorize lifecycle state and changed setup to setup(GroupByQueryConfig config), called at the start of each of the four @ParameterizedTest methods. Each parameterized invocation now builds its indexes with its own config after JUnit 5 supplies the parameters; vectorize remains the test method parameter used for the query context.
Validation:
mvn -ntp test -pl extensions-core/datasketches -am -Dtest='org.apache.druid.query.aggregation.datasketches.theta.SketchAggregationWithSimpleDataTest,org.apache.druid.query.aggregation.datasketches.theta.oldapi.OldApiSketchAggregationTest,org.apache.druid.query.aggregation.datasketches.hll.HllSketchMergeAggregatorFactoryTest' -Dsurefire.failIfNoSpecifiedTests=false -Pskip-static-checks -Dweb.console.skip=true -T1C
Result: 86 tests passed, 0 failures/errors/skips. Checkstyle and SpotBugs also passed.
| @AfterEach | ||
| public void teardown() throws IOException | ||
| { | ||
| helper.close(); | ||
| } |
There was a problem hiding this comment.
Fixed in commit 6d8055b3f8. teardown() now checks helper != null, closes it when initialized, and clears the field afterward. This prevents cleanup from masking an initialization failure and avoids retaining a closed helper.
The focused OldApiSketchAggregationTest run passed all 36 parameterized tests. The combined focused command covered this class and the other two reviewed classes and passed 86 tests; Maven Checkstyle and SpotBugs both reported no violations or errors.
| HllSketchAggregatorFactory result = (HllSketchAggregatorFactory) targetRound.getMergingFactory(other); | ||
| Assertions.assertEquals(LG_K, result.getLgK()); |
There was a problem hiding this comment.
Fixed in commit 6d8055b3f8. The unreachable result assignment and assertion were removed from testGetMergingFactoryDifferentStringEncoding; the lambda now only invokes the method expected to throw.
The focused HllSketchMergeAggregatorFactoryTest run passed all 18 tests. The combined focused DataSketches command passed 86 tests, and Maven Checkstyle and SpotBugs passed with 0 violations/errors.
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 80 of 80 changed files. The later commits preserve test-count parity and correctly handle strict-compilation cleanup, parameterized setup ordering, and teardown null safety.
This is an automated review by Codex GPT-5.6-Sol
Description
Migrates the
extensions-core/datasketchestest suite from JUnit 4 to JUnit 5.This is Batch 7 of the extensions-core migration and is part of #13948.
Depends on the earlier migration batches in #19875, #19876, #19877, #19878, #19879, and #19880.
The migration replaces JUnit 4 assertions, lifecycle methods, and temporary-folder rules with Jupiter equivalents. The shared
AggregationTestHelpersupports the migrated@TempDircallers while retaining its legacy overload for the remaining migration work.Tests
Result: 1,049 tests passed; 0 failures, errors, or skips.