FrankChen021 commented on code in PR #19877:
URL: https://github.com/apache/druid/pull/19877#discussion_r3717603875
##########
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:
Fixed in commit `f14a72a939`. Removed the one-use
`initVarianceTopNQueryTest` method and mutable field. The parameterized
`runner` is now passed directly to `assertExpectedResults`, which still applies
the same `TopNQueryQueryToolChest.mergeResults` behavior.
Validation: the focused Maven run passed all 288 variance tests, including
24 `VarianceTopNQueryTest` executions, plus all 5 basic-security tests;
Checkstyle reported 0 violations and SpotBugs reported 0 bugs/errors.
##########
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:
Fixed in commit `f14a72a939`. Removed the unused `runner` field and removed
`runner`/`aggregatorFactories` from `initVarianceTimeseriesQueryTest`; the
initializer now receives only the values needed for the test state. Query
execution still uses the parameterized local `runner`.
Validation: the focused Maven run passed all 288 variance tests, including
104 `VarianceTimeseriesQueryTest` executions, plus all 5 basic-security tests;
Checkstyle reported 0 violations and SpotBugs reported 0 bugs/errors.
##########
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:
Fixed in commit `f14a72a939`. Removed the unused `config`, `runner`,
`factory`, and `testName` instance fields and the dead initializer. Each
parameterized test now locally merges its supplied runner with the same direct
executor and creates its own context-aware `queryBuilder`, preserving both
runner execution and the vectorization context.
Validation: the focused Maven run passed all 288 variance tests, including
160 `VarianceGroupByQueryTest` executions, plus all 5 basic-security tests;
Checkstyle reported 0 violations and SpotBugs reported 0 bugs/errors.
##########
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:
Fixed in commit `f14a72a939`. `initVarianceTimeseriesQueryTest` now accepts
only the `descending` and `vectorize` values it uses, and both call sites were
updated. The parameterized `runner` and `aggregatorFactories` arguments remain
on the test methods because they are supplied by the constructor feeder, but
are no longer passed into the initializer.
Validation: the focused Maven run passed all 288 variance tests (104
timeseries, 160 group-by, and 24 top-N) plus all 5 `LdapUserPrincipalTest`
tests; Checkstyle reported 0 violations and SpotBugs reported 0 bugs/errors.
##########
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:
Fixed in commit `f14a72a939` by removing the trailing whitespace from the
method declaration.
Validation: `LdapUserPrincipalTest` passed all 5 tests in the focused Maven
run; the variance suite also passed all 288 tests. Checkstyle reported 0
violations and SpotBugs reported 0 bugs/errors.
--
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]
