FrankChen021 commented on code in PR #19877:
URL: https://github.com/apache/druid/pull/19877#discussion_r3718166293
##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/VarianceGroupByQueryTest.java:
##########
@@ -150,13 +135,23 @@
.add("2011-04-02", "travel", 0d, 0d)
.build(query);
- Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, runner, query);
+ Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, mergedRunner, query);
TestHelper.assertExpectedObjects(expectedResults, results, "variance");
}
- @Test
- public void testGroupBy()
+ @MethodSource("constructorFeeder")
+ @ParameterizedTest(name = "{0}")
+ public void testGroupBy(
+ String testName,
Review Comment:
This is intentional rather than an unused parameter. JUnit 5 uses
`@ParameterizedTest(name = "{0}")` to consume the first argument as the
per-configuration display name. Removing `testName` would break that
display-name mapping, so it is retained. This finding is a false positive.
##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/VarianceGroupByQueryTest.java:
##########
@@ -150,13 +135,23 @@
.add("2011-04-02", "travel", 0d, 0d)
.build(query);
- Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, runner, query);
+ Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, mergedRunner, query);
TestHelper.assertExpectedObjects(expectedResults, results, "variance");
}
- @Test
- public void testGroupBy()
+ @MethodSource("constructorFeeder")
+ @ParameterizedTest(name = "{0}")
+ public void testGroupBy(
+ String testName,
+ GroupByQueryConfig config,
+ GroupByQueryRunnerFactory factory,
+ QueryRunner<ResultRow> runner
+ )
{
+ final QueryRunner<ResultRow> mergedRunner =
+ factory.mergeRunners(Execs.directExecutor(), ImmutableList.of(runner));
Review Comment:
This is a baseline/intentional use. The corresponding
`QueryRunnerFactory.mergeRunners` call already existed in `offical/master`
inside the old parameterized constructor; the JUnit 5 migration moved
equivalent setup into each parameterized test. I verified the master version
before this follow-up. I am leaving the deprecated API unchanged because no
safe, narrowly scoped replacement was established and changing it would expand
this migration cleanup. The test behavior is preserved. Validation after the
related parameter cleanup: 264 focused variance tests passed; Checkstyle
reported 0 violations and SpotBugs reported 0 bugs/errors.
##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/VarianceGroupByQueryTest.java:
##########
@@ -195,13 +190,23 @@
.add("2011-04-02", "travel", 1L, 126L, 0d, 0d)
.build(query);
- Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, runner, query);
+ Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, mergedRunner, query);
TestHelper.assertExpectedObjects(expectedResults, results, "groupBy");
}
- @Test
- public void testPostAggHavingSpec()
+ @MethodSource("constructorFeeder")
+ @ParameterizedTest(name = "{0}")
+ public void testPostAggHavingSpec(
+ String testName,
Review Comment:
This is intentional rather than an unused parameter. JUnit 5 uses
`@ParameterizedTest(name = "{0}")` to consume the first argument as the
per-configuration display name. Removing `testName` would break that
display-name mapping, so it is retained. This finding is a false positive.
##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/VarianceGroupByQueryTest.java:
##########
@@ -195,13 +190,23 @@
.add("2011-04-02", "travel", 1L, 126L, 0d, 0d)
.build(query);
- Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, runner, query);
+ Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, mergedRunner, query);
TestHelper.assertExpectedObjects(expectedResults, results, "groupBy");
}
- @Test
- public void testPostAggHavingSpec()
+ @MethodSource("constructorFeeder")
+ @ParameterizedTest(name = "{0}")
+ public void testPostAggHavingSpec(
+ String testName,
+ GroupByQueryConfig config,
+ GroupByQueryRunnerFactory factory,
+ QueryRunner<ResultRow> runner
+ )
{
+ final QueryRunner<ResultRow> mergedRunner =
+ factory.mergeRunners(Execs.directExecutor(), ImmutableList.of(runner));
Review Comment:
This is a baseline/intentional use. The corresponding
`QueryRunnerFactory.mergeRunners` call already existed in `offical/master`
inside the old parameterized constructor; the JUnit 5 migration moved
equivalent setup into each parameterized test. I verified the master version
before this follow-up. I am leaving the deprecated API unchanged because no
safe, narrowly scoped replacement was established and changing it would expand
this migration cleanup. The test behavior is preserved. Validation after the
related parameter cleanup: 264 focused variance tests passed; Checkstyle
reported 0 violations and SpotBugs reported 0 bugs/errors.
##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/VarianceGroupByQueryTest.java:
##########
@@ -250,13 +255,23 @@
.add("2011-04-01", "premium", 6L, 4416L, 252279.2020389339,
502.27403082275106)
.build(query);
- results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, query);
+ results = GroupByQueryRunnerTestHelper.runQuery(factory, mergedRunner,
query);
TestHelper.assertExpectedObjects(expectedResults, results, "limitSpec");
}
- @Test
- public void testGroupByZtestPostAgg()
+ @MethodSource("constructorFeeder")
+ @ParameterizedTest(name = "{0}")
+ public void testGroupByZtestPostAgg(
+ String testName,
Review Comment:
This is intentional rather than an unused parameter. JUnit 5 uses
`@ParameterizedTest(name = "{0}")` to consume the first argument as the
per-configuration display name. Removing `testName` would break that
display-name mapping, so it is retained. This finding is a false positive.
##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/VarianceGroupByQueryTest.java:
##########
@@ -250,13 +255,23 @@
.add("2011-04-01", "premium", 6L, 4416L, 252279.2020389339,
502.27403082275106)
.build(query);
- results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, query);
+ results = GroupByQueryRunnerTestHelper.runQuery(factory, mergedRunner,
query);
TestHelper.assertExpectedObjects(expectedResults, results, "limitSpec");
}
- @Test
- public void testGroupByZtestPostAgg()
+ @MethodSource("constructorFeeder")
+ @ParameterizedTest(name = "{0}")
+ public void testGroupByZtestPostAgg(
+ String testName,
+ GroupByQueryConfig config,
+ GroupByQueryRunnerFactory factory,
+ QueryRunner<ResultRow> runner
+ )
{
+ final QueryRunner<ResultRow> mergedRunner =
+ factory.mergeRunners(Execs.directExecutor(), ImmutableList.of(runner));
Review Comment:
This is a baseline/intentional use. The corresponding
`QueryRunnerFactory.mergeRunners` call already existed in `offical/master`
inside the old parameterized constructor; the JUnit 5 migration moved
equivalent setup into each parameterized test. I verified the master version
before this follow-up. I am leaving the deprecated API unchanged because no
safe, narrowly scoped replacement was established and changing it would expand
this migration cleanup. The test behavior is preserved. Validation after the
related parameter cleanup: 264 focused variance tests passed; Checkstyle
reported 0 violations and SpotBugs reported 0 bugs/errors.
##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/VarianceGroupByQueryTest.java:
##########
@@ -291,13 +306,23 @@
.add("2011-04-01", "premium", 3L, 2900.0, 726.632270328514,
527994.4562827706, 36.54266309285626)
.build(query);
- Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, runner, query);
+ Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, mergedRunner, query);
TestHelper.assertExpectedObjects(expectedResults, results, "groupBy");
}
- @Test
- public void testGroupByTestPvalueZscorePostAgg()
+ @MethodSource("constructorFeeder")
+ @ParameterizedTest(name = "{0}")
+ public void testGroupByTestPvalueZscorePostAgg(
+ String testName,
Review Comment:
This is intentional rather than an unused parameter. JUnit 5 uses
`@ParameterizedTest(name = "{0}")` to consume the first argument as the
per-configuration display name. Removing `testName` would break that
display-name mapping, so it is retained. This finding is a false positive.
##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/VarianceGroupByQueryTest.java:
##########
@@ -291,13 +306,23 @@
.add("2011-04-01", "premium", 3L, 2900.0, 726.632270328514,
527994.4562827706, 36.54266309285626)
.build(query);
- Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, runner, query);
+ Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, mergedRunner, query);
TestHelper.assertExpectedObjects(expectedResults, results, "groupBy");
}
- @Test
- public void testGroupByTestPvalueZscorePostAgg()
+ @MethodSource("constructorFeeder")
+ @ParameterizedTest(name = "{0}")
+ public void testGroupByTestPvalueZscorePostAgg(
+ String testName,
+ GroupByQueryConfig config,
+ GroupByQueryRunnerFactory factory,
+ QueryRunner<ResultRow> runner
+ )
{
+ final QueryRunner<ResultRow> mergedRunner =
+ factory.mergeRunners(Execs.directExecutor(), ImmutableList.of(runner));
Review Comment:
This is a baseline/intentional use. The corresponding
`QueryRunnerFactory.mergeRunners` call already existed in `offical/master`
inside the old parameterized constructor; the JUnit 5 migration moved
equivalent setup into each parameterized test. I verified the master version
before this follow-up. I am leaving the deprecated API unchanged because no
safe, narrowly scoped replacement was established and changing it would expand
this migration cleanup. The test behavior is preserved. Validation after the
related parameter cleanup: 264 focused variance tests passed; 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]
