KAFKA-20942: Refactor empty catch blocks in clients module tests to use assertThrows - #23187
Conversation
github-actions
Bot
added
triage
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
|
Hi everyone! |
aadf7b9 to
9abbf1b
Compare
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
|
Hi! @hjtiun852 |
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
9abbf1b to
1ba3136
Compare
|
A label of 'needs-attention' was automatically added to this PR in order to raise the |
m1a2st
left a comment
m1a2st
left a comment
There was a problem hiding this comment.
Could you also take a look at the following tests?
- OffsetFetcherTest
- ShareConsumeRequestManagerTest
- TopicMetadataFetcherTest
- RecordAccumulatorTest
- SenderTest
- SaslChannelBuilderTest
- ProtocolSerializationTest
- RawTaggedFieldWriterTest
- AbstractLegacyRecordBatchTest
- LeaveGroupRequestTest
| try { | ||
| pool.allocate(2, maxBlockTimeMs); | ||
| fail("The buffer allocated more memory than its maximum value 2"); | ||
| } catch (BufferExhaustedException e) { | ||
| // this is good | ||
| } catch (InterruptedException e) { | ||
| // this can be neglected | ||
| } | ||
| assertThrows(BufferExhaustedException.class, () -> pool.allocate(2, maxBlockTimeMs)); |
There was a problem hiding this comment.
This change drops the InterruptedException catch branch that was here before. The original code accepted both BufferExhaustedException and InterruptedException as valid outcomes.
There was a problem hiding this comment.
your right! BufferExhaustedException and InterruptedException are both included in latest commit.
| } | ||
|
|
||
| @Test | ||
| @SuppressWarnings("deprecation") |
There was a problem hiding this comment.
Thanks for pointing out! Removed.
…se assertThrows * Empty catch block can swallow unexpected errors, bugs or exceptions during test execution. * Refactored 35 tests from clients module to use JUnit 5's `assertThrows` for strict and safe exception handling. Signed-off-by: Kapil Hadoltikar <hadoltikar.kapil@gmail.com>
1ba3136 to
8f49941
Compare
|
Thanks for the thorough review! |
2 participants
Empty catch blocks can swallow unexpected errors, bugs, or exceptions
during test execution.
Refactored empty catch blocks in the clients module tests to use JUnit
5's assertThrows for strict and safe exception handling.
Changes
Refactored empty catch blocks to use JUnit 5's assertThrows across
several test areas in the clients module:
Admin & Consumer:
DescribeUserScramCredentialsResultTest,consumer internals
AbstractCoordinatorTest,ConsumerCoordinatorTest,FetcherTest,FetchRequestManagerTest, andKafkaConsumerTest.Producer & Internals:
BufferPoolTest,ProducerBatchTest,ProducerMetadataTest,TransactionManagerTest, andKafkaProducerTest.Common Config, Metrics & Network:
ConfigDefTest,TopicTest,ApiMessageTypeTest,KafkaMbeanTest,SensorTest, andSelectorTest.Security & Utils:
FileRecordsTest,Sasl,OAuthbearer, andScramMessagesTest,SslFactoryTest,JaasContextTest,ImplicitLinkedHashCollectionTest, andUtilsTest.Testing
Ran ./gradlew :clients:test locally and verified all tests passed.
All CI checks passed successfully.
Reviewers: Ken Huang s7133700@gmail.com