Copilot commented on code in PR #19859:
URL: https://github.com/apache/druid/pull/19859#discussion_r3703956089
##########
processing/src/test/java/org/apache/druid/segment/IndexIOTest.java:
##########
@@ -236,48 +231,35 @@ public static List<Map> filterNullValues(List<Map<String,
Object>> mapList)
return Lists.transform(mapList, (Function<Map, Map>) input ->
Maps.filterValues(input, Objects::nonNull));
}
- @Parameter(0)
- public Collection<Map<String, Object>> events1;
-
- @Parameter(1)
- public Collection<Map<String, Object>> events2;
-
- @Parameter(2)
- public Class<? extends Exception> exception;
-
-
- final IncrementalIndex incrementalIndex1 = new
OnheapIncrementalIndex.Builder()
- .setIndexSchema(
- new IncrementalIndexSchema.Builder()
- .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
- .withMetrics(new CountAggregatorFactory("count"))
- .withDimensionsSpec(
- new
DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
- )
- .build()
- )
- .setMaxRowCount(1000000)
- .build();
-
- final IncrementalIndex incrementalIndex2 = new
OnheapIncrementalIndex.Builder()
- .setIndexSchema(
- new IncrementalIndexSchema.Builder()
- .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
- .withMetrics(new CountAggregatorFactory("count"))
- .withDimensionsSpec(
- new
DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
- )
- .build()
- )
- .setMaxRowCount(1000000)
- .build();
-
- IndexableAdapter adapter1;
- IndexableAdapter adapter2;
+ private static IncrementalIndex createIncrementalIndex()
+ {
+ return new OnheapIncrementalIndex.Builder()
+ .setIndexSchema(
+ new IncrementalIndexSchema.Builder()
+ .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
+ .withMetrics(new CountAggregatorFactory("count"))
+ .withDimensionsSpec(
+ new
DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
+ )
+ .build()
+ )
+ .setMaxRowCount(1000000)
+ .build();
+ }
- @BeforeEach
- public void setUp()
+ @ParameterizedTest(
+ name = "[{index}] schema={0}, rows={1}, dimensions={2}"
+ )
+ @MethodSource("constructionFeeder")
+ public void testRowValidatorEquals(
+ Collection<Map<String, Object>> events1,
+ Collection<Map<String, Object>> events2,
+ Class<? extends Exception> exception
+ ) throws Exception
Review Comment:
The parameterized display name labels don’t match the actual parameters:
`{0}`/`{1}` are `events1`/`events2`, and `{2}` is an exception class (not
`dimensions`). This makes reports misleading; update the name template to
reflect the real argument meanings (or use `Named` arguments and
`{argumentsWithNames}` to keep names accurate and concise).
##########
processing/src/test/java/org/apache/druid/segment/IndexIOTest.java:
##########
@@ -236,48 +231,35 @@ public static List<Map> filterNullValues(List<Map<String,
Object>> mapList)
return Lists.transform(mapList, (Function<Map, Map>) input ->
Maps.filterValues(input, Objects::nonNull));
}
- @Parameter(0)
- public Collection<Map<String, Object>> events1;
-
- @Parameter(1)
- public Collection<Map<String, Object>> events2;
-
- @Parameter(2)
- public Class<? extends Exception> exception;
-
-
- final IncrementalIndex incrementalIndex1 = new
OnheapIncrementalIndex.Builder()
- .setIndexSchema(
- new IncrementalIndexSchema.Builder()
- .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
- .withMetrics(new CountAggregatorFactory("count"))
- .withDimensionsSpec(
- new
DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
- )
- .build()
- )
- .setMaxRowCount(1000000)
- .build();
-
- final IncrementalIndex incrementalIndex2 = new
OnheapIncrementalIndex.Builder()
- .setIndexSchema(
- new IncrementalIndexSchema.Builder()
- .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
- .withMetrics(new CountAggregatorFactory("count"))
- .withDimensionsSpec(
- new
DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
- )
- .build()
- )
- .setMaxRowCount(1000000)
- .build();
-
- IndexableAdapter adapter1;
- IndexableAdapter adapter2;
+ private static IncrementalIndex createIncrementalIndex()
+ {
+ return new OnheapIncrementalIndex.Builder()
+ .setIndexSchema(
+ new IncrementalIndexSchema.Builder()
+ .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
+ .withMetrics(new CountAggregatorFactory("count"))
+ .withDimensionsSpec(
+ new
DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
+ )
+ .build()
+ )
+ .setMaxRowCount(1000000)
+ .build();
+ }
- @BeforeEach
- public void setUp()
+ @ParameterizedTest(
+ name = "[{index}] schema={0}, rows={1}, dimensions={2}"
+ )
Review Comment:
Including full `Collection<Map<...>>` values in the testcase name can
produce very large and potentially non-deterministic strings (e.g., map
iteration order), inflating XML and making reports noisy/unreliable. Prefer
naming based on stable, compact identifiers (like case ids, sizes, or
`Named.of(...)` summaries) rather than dumping entire argument payloads.
##########
processing/src/test/java/org/apache/druid/segment/IndexIOTest.java:
##########
@@ -236,48 +231,35 @@ public static List<Map> filterNullValues(List<Map<String,
Object>> mapList)
return Lists.transform(mapList, (Function<Map, Map>) input ->
Maps.filterValues(input, Objects::nonNull));
}
- @Parameter(0)
- public Collection<Map<String, Object>> events1;
-
- @Parameter(1)
- public Collection<Map<String, Object>> events2;
-
- @Parameter(2)
- public Class<? extends Exception> exception;
-
-
- final IncrementalIndex incrementalIndex1 = new
OnheapIncrementalIndex.Builder()
- .setIndexSchema(
- new IncrementalIndexSchema.Builder()
- .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
- .withMetrics(new CountAggregatorFactory("count"))
- .withDimensionsSpec(
- new
DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
- )
- .build()
- )
- .setMaxRowCount(1000000)
- .build();
-
- final IncrementalIndex incrementalIndex2 = new
OnheapIncrementalIndex.Builder()
- .setIndexSchema(
- new IncrementalIndexSchema.Builder()
- .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
- .withMetrics(new CountAggregatorFactory("count"))
- .withDimensionsSpec(
- new
DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
- )
- .build()
- )
- .setMaxRowCount(1000000)
- .build();
-
- IndexableAdapter adapter1;
- IndexableAdapter adapter2;
+ private static IncrementalIndex createIncrementalIndex()
+ {
+ return new OnheapIncrementalIndex.Builder()
+ .setIndexSchema(
+ new IncrementalIndexSchema.Builder()
+ .withMinTimestamp(DEFAULT_INTERVAL.getStart().getMillis())
+ .withMetrics(new CountAggregatorFactory("count"))
+ .withDimensionsSpec(
+ new
DimensionsSpec(DimensionsSpec.getDefaultSchemas(Arrays.asList("dim0", "dim1")))
+ )
+ .build()
+ )
+ .setMaxRowCount(1000000)
Review Comment:
The literal `1000000` is a new magic number in a helper used by the test.
Consider extracting it to a `private static final int MAX_ROW_COUNT = ...` (or
reusing an existing constant in the file, if any) to make intent explicit and
avoid future drift if the value needs to change.
##########
pom.xml:
##########
@@ -2062,6 +2062,9 @@
<trimStackTrace>${surefire.trimStackTrace}</trimStackTrace>
<!-- our tests are very verbose, let's keep the volume
down -->
<redirectTestOutputToFile>true</redirectTestOutputToFile>
+ <statelessTestsetReporter
implementation="org.apache.maven.plugin.surefire.extensions.junit5.JUnit5Xml30StatelessReporter">
+
<usePhrasedTestCaseMethodName>true</usePhrasedTestCaseMethodName>
+ </statelessTestsetReporter>
Review Comment:
This changes Surefire XML reporting behavior globally (and may increase XML
verbosity/size for all parameterized tests across the build). If the motivation
is specific to `IndexIOTest`/`processing`, consider scoping this to the
relevant module/profile or guarding it behind a property so other modules and
CI consumers aren’t forced into the new reporting mode.
##########
pom.xml:
##########
@@ -2062,6 +2062,9 @@
<trimStackTrace>${surefire.trimStackTrace}</trimStackTrace>
<!-- our tests are very verbose, let's keep the volume
down -->
<redirectTestOutputToFile>true</redirectTestOutputToFile>
+ <statelessTestsetReporter
implementation="org.apache.maven.plugin.surefire.extensions.junit5.JUnit5Xml30StatelessReporter">
+
<usePhrasedTestCaseMethodName>true</usePhrasedTestCaseMethodName>
+ </statelessTestsetReporter>
Review Comment:
The configuration references a concrete reporter implementation class; the
build will fail if the configured Maven Surefire plugin version on the plugin
classpath doesn’t include this extension. It would be safer to (a) ensure the
Surefire plugin version is explicitly pinned to a version known to provide
`JUnit5Xml30StatelessReporter`, and/or (b) add a short comment in the pom
noting the minimum required Surefire version for this config.
--
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]
