Copilot commented on code in PR #19917:
URL: https://github.com/apache/druid/pull/19917#discussion_r3736353497
##########
extensions-contrib/virtual-columns/src/test/java/org/apache/druid/segment/MapVirtualColumnTopNTest.java:
##########
@@ -82,25 +81,25 @@ public void setup() throws IOException
@Test
public void testWithMapColumn()
{
- final TopNQuery query = new TopNQuery(
- new TableDataSource(QueryRunnerTestHelper.DATA_SOURCE),
- VirtualColumns.create(
- new MapVirtualColumn("keys", "values", "params")
- ),
- new DefaultDimensionSpec("params", "params"), // params is the map type
- new NumericTopNMetricSpec("count"),
- 1,
- new
MultipleIntervalSegmentSpec(ImmutableList.of(Intervals.of("2011/2012"))),
- null,
- Granularities.ALL,
- ImmutableList.of(new CountAggregatorFactory("count")),
- null,
- null
- );
-
- expectedException.expect(UnsupportedOperationException.class);
- expectedException.expectMessage("Map column doesn't support getRow()");
- runner.run(QueryPlus.wrap(query)).toList();
+ Throwable exception = assertThrows(UnsupportedOperationException.class, ()
-> {
Review Comment:
Local variables that are not reassigned should be declared `final` (per
AGENTS.md code style).
##########
extensions-contrib/virtual-columns/src/test/java/org/apache/druid/segment/MapVirtualColumnGroupByTest.java:
##########
@@ -146,11 +146,11 @@ public void testWithMapColumn()
null
);
- Throwable t = Assert.assertThrows(
+ Throwable t = Assertions.assertThrows(
Review Comment:
Local variables that are not reassigned should be declared `final` (per
AGENTS.md code style).
##########
extensions-contrib/influxdb-emitter/src/test/java/org/apache/druid/emitter/influxdb/InfluxdbEmitterConfigTest.java:
##########
@@ -121,7 +125,7 @@ public void testConfigWithNullPort()
null
);
int expectedPort = 8086;
- Assert.assertEquals(expectedPort, influxdbEmitterConfig.getPort());
+ Assertions.assertEquals(expectedPort, influxdbEmitterConfig.getPort());
}
Review Comment:
This assertion is checking the `influxdbEmitterConfig` field from `setUp()`
rather than the newly-created `influxdbEmitterConfigWithNullPort`, so it
doesn't actually validate the null-port defaulting behavior.
##########
extensions-contrib/kafka-emitter/src/test/java/org/apache/druid/emitter/kafka/KafkaEmitterTest.java:
##########
@@ -632,13 +639,19 @@ private void validateEvents(
final Map<String, List<EventMap>> feedToActualEvents
)
{
- Assert.assertEquals(feedToExpectedEvents.size(),
feedToActualEvents.size());
+ Assertions.assertEquals(feedToExpectedEvents.size(),
feedToActualEvents.size());
for (final Map.Entry<String, List<EventMap>> actualEntry :
feedToActualEvents.entrySet()) {
final String feed = actualEntry.getKey();
final List<EventMap> actualEvents = actualEntry.getValue();
final List<EventMap> expectedEvents = feedToExpectedEvents.get(feed);
- assertThat(actualEvents, containsInAnyOrder(expectedEvents.toArray(new
Map[0])));
+ Assertions.assertEquals(expectedEvents.size(), actualEvents.size());
+ Assertions.assertTrue(
+ expectedEvents.stream().allMatch(
+ expectedEvent -> Collections.frequency(expectedEvents,
expectedEvent)
+ == Collections.frequency(actualEvents,
expectedEvent)
Review Comment:
`validateEvents` currently relies on `Collections.frequency` inside a
stream, which is harder to read and can produce a confusing NPE if
`feedToActualEvents` contains a feed not present in `feedToExpectedEvents`.
Consider asserting the feed key exists and comparing per-event counts instead.
##########
extensions-contrib/influxdb-emitter/src/test/java/org/apache/druid/emitter/influxdb/InfluxdbEmitterTest.java:
##########
@@ -219,97 +221,105 @@ public void testMetricIsInDefaultDimensionWhitelist()
String expected =
"druid_metric,service=druid/historical,hostname=localhost,dataSource=wikipedia,taskType=index
druid_time=1234 1509357600000000000"
+ "\n";
String actual = influxdbEmitter.transformForInfluxSystems(event);
- Assert.assertEquals(expected, actual);
+ Assertions.assertEquals(expected, actual);
}
@Test
public void testJacksonModules()
{
- Assert.assertTrue(new
InfluxdbEmitterModule().getJacksonModules().isEmpty());
+ Assertions.assertTrue(new
InfluxdbEmitterModule().getJacksonModules().isEmpty());
}
- @Test(expected = IllegalStateException.class)
+ @Test
public void testBuildInfluxdbClientWithHttpsProtocolAndNoTrustStore()
{
- InfluxdbEmitterConfig config = new InfluxdbEmitterConfig(
- "localhost",
- 8086,
- "https",
- null,
- null,
- null,
- "dbname",
- 10000,
- 15000,
- 30000,
- "adam",
- "password",
- null
- );
- InfluxdbEmitter influxdbEmitter = new InfluxdbEmitter(config);
+ assertThrows(IllegalStateException.class, () -> {
+ InfluxdbEmitterConfig config = new InfluxdbEmitterConfig(
+ "localhost",
+ 8086,
+ "https",
+ null,
+ null,
+ null,
+ "dbname",
+ 10000,
+ 15000,
+ 30000,
+ "adam",
+ "password",
+ null
+ );
+ InfluxdbEmitter influxdbEmitter = new InfluxdbEmitter(config);
Review Comment:
The `InfluxdbEmitter` local variable inside this `assertThrows` block is
never used; the assignment can be removed to keep the test focused on the side
effect (constructor throwing).
This issue also appears in the following locations of the same file:
- line 275
- line 298
- line 321
##########
extensions-contrib/influxdb-emitter/src/test/java/org/apache/druid/emitter/influxdb/InfluxdbEmitterConfigTest.java:
##########
@@ -79,27 +81,29 @@ public void testInfluxdbEmitterConfigObjectsAreDifferent()
"password",
null
);
- Assert.assertNotEquals(influxdbEmitterConfig,
influxdbEmitterConfigComparison);
+ Assertions.assertNotEquals(influxdbEmitterConfig,
influxdbEmitterConfigComparison);
}
- @Test(expected = NullPointerException.class)
+ @Test
public void testConfigWithNullHostname()
{
- InfluxdbEmitterConfig influxdbEmitterConfigWithNullHostname = new
InfluxdbEmitterConfig(
- null,
- 8080,
- null,
- null,
- null,
- null,
- "dbname",
- 10000,
- 15000,
- 30000,
- "adam",
- "password",
- null
- );
+ assertThrows(NullPointerException.class, () -> {
+ InfluxdbEmitterConfig influxdbEmitterConfigWithNullHostname = new
InfluxdbEmitterConfig(
Review Comment:
The local variable in this `assertThrows` lambda is unused; you can
construct the config without assigning it to a variable.
This issue also appears in the following locations of the same file:
- line 177
- line 199
--
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]
