Copilot commented on code in PR #19881:
URL: https://github.com/apache/druid/pull/19881#discussion_r3717543512
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/SketchAggregationWithSimpleDataTest.java:
##########
@@ -98,20 +95,20 @@ public static Collection<?> constructorFeeder()
return constructors;
}
- @Before
+ @BeforeEach
public void setup() throws Exception
{
SketchModule.registerSerde();
sm = new SketchModule();
try (
- final AggregationTestHelper toolchest =
AggregationTestHelper.createGroupByQueryAggregationTestHelper(
+ final AggregationTestHelper toolchest =
AggregationTestHelper.createGroupByQueryAggregationTestHelperWithTempDir(
sm.getJacksonModules(),
config,
tempFolder
Review Comment:
`@BeforeEach setup()` uses the `config` field, but `config` is only assigned
via `initSketchAggregationWithSimpleDataTest(...)`, which is called inside each
`@ParameterizedTest`. In JUnit 5, `@BeforeEach` runs before the parameterized
test method is invoked, so `config` will be null here (likely NPE) and/or the
per-invocation config passed to the test won't actually be applied to the
index/toolchest setup.
Move this setup into a helper method that takes
`GroupByQueryConfig`/`vectorize` parameters and call it at the start of each
`@ParameterizedTest` (or build the index/toolchest with locals inside each
test), instead of relying on mutable fields populated after `@BeforeEach`.
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchMergeAggregatorFactoryTest.java:
##########
@@ -82,50 +83,56 @@ public void setUp()
vectorFactory = new
TestVectorColumnSelectorFactory().addCapabilities(FIELD_NAME,
columnCapabilities);
}
- @Test(expected = AggregatorFactoryNotMergeableException.class)
- public void testGetMergingFactoryBadName() throws Exception
+ @Test
+ public void testGetMergingFactoryBadName()
{
- HllSketchMergeAggregatorFactory other = new
HllSketchMergeAggregatorFactory(
- NAME + "-diff",
- FIELD_NAME,
- LG_K,
- TGT_HLL_TYPE,
- STRING_ENCODING,
- SHOULD_FINALIZE,
- ROUND
- );
- targetRound.getMergingFactory(other);
+ Assertions.assertThrows(AggregatorFactoryNotMergeableException.class, ()
-> {
+ HllSketchMergeAggregatorFactory other = new
HllSketchMergeAggregatorFactory(
+ NAME + "-diff",
+ FIELD_NAME,
+ LG_K,
+ TGT_HLL_TYPE,
+ STRING_ENCODING,
+ SHOULD_FINALIZE,
+ ROUND
+ );
+ targetRound.getMergingFactory(other);
+ });
}
- @Test(expected = AggregatorFactoryNotMergeableException.class)
- public void testGetMergingFactoryBadType() throws Exception
+ @Test
+ public void testGetMergingFactoryBadType()
{
- HllSketchBuildAggregatorFactory other = new
HllSketchBuildAggregatorFactory(
- NAME,
- FIELD_NAME,
- LG_K,
- TGT_HLL_TYPE,
- STRING_ENCODING,
- SHOULD_FINALIZE,
- ROUND
- );
- targetRound.getMergingFactory(other);
+ Assertions.assertThrows(AggregatorFactoryNotMergeableException.class, ()
-> {
+ HllSketchBuildAggregatorFactory other = new
HllSketchBuildAggregatorFactory(
+ NAME,
+ FIELD_NAME,
+ LG_K,
+ TGT_HLL_TYPE,
+ STRING_ENCODING,
+ SHOULD_FINALIZE,
+ ROUND
+ );
+ targetRound.getMergingFactory(other);
+ });
}
- @Test(expected = AggregatorFactoryNotMergeableException.class)
- public void testGetMergingFactoryDifferentStringEncoding() throws Exception
+ @Test
+ public void testGetMergingFactoryDifferentStringEncoding()
{
- HllSketchMergeAggregatorFactory other = new
HllSketchMergeAggregatorFactory(
- NAME,
- FIELD_NAME,
- LG_K,
- TGT_HLL_TYPE,
- StringEncoding.UTF8,
- SHOULD_FINALIZE,
- ROUND
- );
- HllSketchAggregatorFactory result = (HllSketchAggregatorFactory)
targetRound.getMergingFactory(other);
- Assert.assertEquals(LG_K, result.getLgK());
+ Assertions.assertThrows(AggregatorFactoryNotMergeableException.class, ()
-> {
+ HllSketchMergeAggregatorFactory other = new
HllSketchMergeAggregatorFactory(
+ NAME,
+ FIELD_NAME,
+ LG_K,
+ TGT_HLL_TYPE,
+ StringEncoding.UTF8,
+ SHOULD_FINALIZE,
+ ROUND
+ );
+ HllSketchAggregatorFactory result = (HllSketchAggregatorFactory)
targetRound.getMergingFactory(other);
+ Assertions.assertEquals(LG_K, result.getLgK());
Review Comment:
Inside this `assertThrows`, the `result` assignment and `assertEquals` are
unreachable because `targetRound.getMergingFactory(other)` is expected to
throw. Keeping assertions after the throwing call is misleading and makes it
harder to understand what the test is validating.
Remove the dead code and just invoke the throwing method within the lambda.
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/oldapi/OldApiSketchAggregationTest.java:
##########
@@ -92,15 +88,17 @@ public static Collection<?> constructorFeeder()
return constructors;
}
- @After
+ @AfterEach
public void teardown() throws IOException
{
helper.close();
}
Review Comment:
`teardown()` unconditionally calls `helper.close()`, but `helper` is only
initialized inside `initOldApiSketchAggregationTest(...)` which runs inside
each test method. If initialization fails (or if a future test is added that
forgets to call `init...`), `@AfterEach` will throw an NPE and can mask the
real failure.
Guard the close call (and optionally null out the field) so cleanup is
resilient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
