MINOR: Clean up unreachable code in FetcherTest#19376
Conversation
github-actions
bot
added
triage
| @@ -3664,7 +3664,6 @@ public void testFetcherDontCacheAnyData() { | |||
| responseData.forEach((topicPartition, partitionData) -> assertEquals(records, partitionData.records())); | |||
| LinkedHashMap<TopicPartition, FetchResponseData.PartitionData> nonResponseData = fetchResponse.responseData(emptyMap(), version); | |||
| assertEquals(emptyMap().size(), nonResponseData.size()); | |||
There was a problem hiding this comment.
I suggest that we use assertTrue for this case.
IDEA will have a mark the post iteration is unnecessary after assert empty but it won't do the same thing for assert 0.
There was a problem hiding this comment.
Thanks, @chia7712 and @frankvicky, for reminding me about the missing comment.
I think the isEmpty approach helps prevent potential future mistakes, so I've adopted it.
| @@ -3664,7 +3664,6 @@ public void testFetcherDontCacheAnyData() { | |||
| responseData.forEach((topicPartition, partitionData) -> assertEquals(records, partitionData.records())); | |||
| LinkedHashMap<TopicPartition, FetchResponseData.PartitionData> nonResponseData = fetchResponse.responseData(emptyMap(), version); | |||
| assertEquals(emptyMap().size(), nonResponseData.size()); | |||
There was a problem hiding this comment.
Please also address https://github.com/apache/kafka/pull/16532/files#r2029018014
There was a problem hiding this comment.
Thanks for the reminder about the missing comment!
I've replied it on the previous review.
frankvicky
left a comment
frankvicky
left a comment
There was a problem hiding this comment.
@Parkerhiphop : Thanks for the patch. I have a comment.
| @@ -3664,7 +3664,6 @@ public void testFetcherDontCacheAnyData() { | |||
| responseData.forEach((topicPartition, partitionData) -> assertEquals(records, partitionData.records())); | |||
| LinkedHashMap<TopicPartition, FetchResponseData.PartitionData> nonResponseData = fetchResponse.responseData(emptyMap(), version); | |||
| assertEquals(emptyMap().size(), nonResponseData.size()); | |||
There was a problem hiding this comment.
nit: assertTrue(nonResponseData.isEmpty());
| @@ -3663,8 +3663,7 @@ public void testFetcherDontCacheAnyData() { | |||
| assertEquals(topicNames.size(), responseData.size()); | |||
| responseData.forEach((topicPartition, partitionData) -> assertEquals(records, partitionData.records())); | |||
| LinkedHashMap<TopicPartition, FetchResponseData.PartitionData> nonResponseData = fetchResponse.responseData(emptyMap(), version); | |||
There was a problem hiding this comment.
Could you also refactor this line? We can use Map.of() to replace emptyMap().
There was a problem hiding this comment.
Yes. I've replace the emptyMap() with Map.of()
FrankYang0529
left a comment
FrankYang0529
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the patch.
6 participants
This is from #16532's
comment:
The forEach loop in the assertion will never execute because
nonResponseDatais empty.This happens because the above assertion
emptyMap()has a size of 0,so there are no elements to iterate over.
Reviewers: PoAn Yang payang@apache.org, Ken Huang
s7133700@gmail.com, TaiJuWu tjwu1217@gmail.com, TengYao Chi
kitingiao@gmail.com, Chia-Ping Tsai chia7712@gmail.com