Copilot commented on code in PR #19877:
URL: https://github.com/apache/druid/pull/19877#discussion_r3717426184
##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/VarianceTopNQueryTest.java:
##########
@@ -38,36 +38,35 @@
import org.apache.druid.query.topn.TopNResultValue;
import org.apache.druid.segment.TestHelper;
import org.apache.druid.testing.InitializedNullHandlingTest;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
-@RunWith(Parameterized.class)
public class VarianceTopNQueryTest extends InitializedNullHandlingTest
{
- @Parameterized.Parameters(name = "{0}")
public static Iterable<Object[]> constructorFeeder()
{
return
QueryRunnerTestHelper.transformToConstructionFeeder(TopNQueryRunnerTest.queryRunners(true));
}
- private final QueryRunner<Result<TopNResultValue>> runner;
+ private QueryRunner<Result<TopNResultValue>> runner;
- public VarianceTopNQueryTest(
+ public void initVarianceTopNQueryTest(
QueryRunner<Result<TopNResultValue>> runner
)
{
this.runner = runner;
}
Review Comment:
`initVarianceTopNQueryTest` is only used to assign the `runner` field and
adds indirection. Assigning the field directly in the parameterized test keeps
the setup local and reduces the chance of future mistakes when adding more
tests.
##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/VarianceTimeseriesQueryTest.java:
##########
@@ -45,22 +44,20 @@
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;
-@RunWith(Parameterized.class)
public class VarianceTimeseriesQueryTest extends InitializedNullHandlingTest
{
- @Parameterized.Parameters(name = "{0}:descending={1}")
public static Iterable<Object[]> constructorFeeder()
{
return
StreamSupport.stream(TimeseriesQueryRunnerTest.constructorFeeder().spliterator(),
false)
.map(constructor -> new Object[]{constructor[0],
constructor[1], constructor[2], constructor[3]})
.collect(Collectors.toList());
}
- private final QueryRunner runner;
- private final boolean descending;
- private final Druids.TimeseriesQueryBuilder queryBuilder;
+ private QueryRunner runner;
+ private boolean descending;
+ private Druids.TimeseriesQueryBuilder queryBuilder;
- public VarianceTimeseriesQueryTest(
+ public void initVarianceTimeseriesQueryTest(
Review Comment:
`initVarianceTimeseriesQueryTest` stores `runner` into a field that is never
used, and it takes `runner` / `aggregatorFactories` parameters that are unused
in the initializer. This adds dead state and makes the parameterization harder
to follow.
##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/VarianceGroupByQueryTest.java:
##########
@@ -61,19 +60,17 @@
/**
*
*/
-@RunWith(Parameterized.class)
public class VarianceGroupByQueryTest extends InitializedNullHandlingTest
{
@MonotonicNonNull
private static TestGroupByBuffers BUFFER_POOLS = null;
- private final GroupByQueryConfig config;
- private final QueryRunner<Row> runner;
- private final GroupByQueryRunnerFactory factory;
- private final String testName;
- private final GroupByQuery.Builder queryBuilder;
+ private GroupByQueryConfig config;
+ private QueryRunner<Row> runner;
+ private GroupByQueryRunnerFactory factory;
+ private String testName;
+ private GroupByQuery.Builder queryBuilder;
Review Comment:
These instance fields (`config`, `runner`, `factory`, `testName`) are
assigned in `initVarianceGroupByQueryTest`, but there are no reads of them
anywhere else in the class (only `queryBuilder` is used). This suggests the
JUnit 4 constructor setup got partially migrated and left unused mutable state,
which can be misleading and may hide a change in how runners are
merged/executed.
##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/VarianceTimeseriesQueryTest.java:
##########
@@ -73,9 +70,16 @@ public VarianceTimeseriesQueryTest(
.context(ImmutableMap.of("vectorize", vectorize
? "force" : "false"));
}
- @Test
- public void testTimeseriesWithNullFilterOnNonExistentDimension()
+ @MethodSource("constructorFeeder")
+ @ParameterizedTest(name = "{0}:descending={1}")
+ public void testTimeseriesWithNullFilterOnNonExistentDimension(
+ QueryRunner runner,
+ boolean descending,
+ boolean vectorize,
+ List<AggregatorFactory> aggregatorFactories
+ )
{
+ initVarianceTimeseriesQueryTest(runner, descending, vectorize,
aggregatorFactories);
Review Comment:
After removing the unused `runner`/`aggregatorFactories` parameters from
`initVarianceTimeseriesQueryTest`, this call should pass only the values
actually used to initialize the test state.
This issue also appears on line 138 of the same file.
##########
extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/basic/authentication/LdapUserPrincipalTest.java:
##########
@@ -44,29 +44,34 @@ public class LdapUserPrincipalTest extends TestCase
Instant.ofEpochMilli(CREATED_MILLIS)
);
+ @Test
public void testIsNotExpired()
{
- Assert.assertFalse(PRINCIPAL.isExpired(1, 10, CREATED_MILLIS + 500));
+ Assertions.assertFalse(PRINCIPAL.isExpired(1, 10, CREATED_MILLIS + 500));
}
+ @Test
public void testIsObviouslyExpired()
{
// real clock now should be so much bigger than CREATED_MILLIS....so it
must have expired...
- Assert.assertTrue(PRINCIPAL.isExpired(100, 1000));
+ Assertions.assertTrue(PRINCIPAL.isExpired(100, 1000));
}
+ @Test
public void testIsExpiredWhenMaxDurationIsSmall()
{
- Assert.assertTrue(PRINCIPAL.isExpired(10, 1, CREATED_MILLIS + 1001));
+ Assertions.assertTrue(PRINCIPAL.isExpired(10, 1, CREATED_MILLIS + 1001));
}
+ @Test
public void testIsExpiredWhenDurationIsSmall()
{
- Assert.assertTrue(PRINCIPAL.isExpired(1, 10, CREATED_MILLIS + 1001));
+ Assertions.assertTrue(PRINCIPAL.isExpired(1, 10, CREATED_MILLIS + 1001));
}
+ @Test
public void testIsExpiredWhenDurationsAreSmall()
Review Comment:
There is trailing whitespace after the method declaration, which can cause
noisy diffs and fail some style checks.
--
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]
