FrankChen021 commented on code in PR #19917:
URL: https://github.com/apache/druid/pull/19917#discussion_r3736580963
##########
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:
Fixed in commit
[`d047bae69f`](https://github.com/FrankChen021/druid/commit/d047bae69f): the
local exception variable is now declared `final`. The JUnit 4 base version used
`ExpectedException` and did not have this local, so this issue was introduced
by the migration. Focused TopN tests passed (2/2), and the changed modules
passed compilation and static checks.
##########
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:
Fixed in commit
[`d047bae69f`](https://github.com/FrankChen021/druid/commit/d047bae69f):
`validateEvents` now asserts equality of the expected and actual feed key sets
before looking up `expectedEvents` or performing per-event count checks. The
existing frequency comparison is unchanged, preserving the original
`containsInAnyOrder` multiset semantics while avoiding an NPE for different
keys. Focused Kafka tests passed (7/7), and the changed modules passed
compilation and static checks.
##########
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:
No code change is needed here. The unused `InfluxdbEmitter influxdbEmitter`
assignments at all four locations already existed in the PR base (for example,
[`fe4377a4` line
249](https://github.com/apache/druid/blob/fe4377a48945449058ac62bc0971f2363431098b/extensions-contrib/influxdb-emitter/src/test/java/org/apache/druid/emitter/influxdb/InfluxdbEmitterTest.java#L249));
the migration only moved each constructor call into an `assertThrows` body.
The constructor invocation remains the behavior under test. This finding is
pre-existing and overlaps the duplicate same-file locations, so cleanup is out
of scope.
##########
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:
No code change is needed here. The PR base already asserted
`influxdbEmitterConfig.getPort()` after constructing
`influxdbEmitterConfigWithNullPort` ([`fe4377a4` line
124](https://github.com/apache/druid/blob/fe4377a48945449058ac62bc0971f2363431098b/extensions-contrib/influxdb-emitter/src/test/java/org/apache/druid/emitter/influxdb/InfluxdbEmitterConfigTest.java#L124));
this migration only changed `Assert.assertEquals` to
`Assertions.assertEquals`. The wrong-variable assertion is pre-existing and out
of scope for this JUnit migration.
##########
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:
No code change is needed here. The unused local assignments for the
null-hostname, null-username, and null-password constructor calls already
existed in the PR base ([`fe4377a4` lines 88, 172, and
192](https://github.com/apache/druid/blob/fe4377a48945449058ac62bc0971f2363431098b/extensions-contrib/influxdb-emitter/src/test/java/org/apache/druid/emitter/influxdb/InfluxdbEmitterConfigTest.java#L85-L207));
the migration only moved each constructor call into an `assertThrows` body.
The constructors must still execute to verify the expected exceptions. This is
a pre-existing/duplicate unused-assignment finding and cleanup is out of scope.
--
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]
