chore : add tests #637
chore : add tests #637
Conversation
WalkthroughThis pull request introduces modifications across multiple Kafka-related test and configuration files. The changes primarily focus on enhancing test coverage for Kafka message publishing and receiving, updating method signatures, and adjusting interface visibility. The modifications span several files in the Kafka DSL integration and Avro producer test suites, improving the overall testing infrastructure for Kafka-based message handling. Changes
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
kafka-dsl-integration/src/test/java/com/example/integration/kafkadsl/KafkaDslApplicationTests.java (2)
25-39: Consider enhancing single message tests with additional assertions.While the basic functionality is tested, consider adding:
- Message headers verification
- Error scenarios (e.g., topic doesn't exist)
- Cleanup between tests to ensure message isolation
53-79: Consider adding topic readiness check.Tests might fail if the new topic isn't fully created and ready. Consider:
- Adding a helper method to wait for topic readiness
- Verifying topic existence before sending messages
- Adding retry logic for topic creation edge cases
kafka-avro/spring-boot-kafka-avro-producer/src/test/java/com/example/springbootkafkaavro/PersonKafkaPublishingIntegrationTests.java (1)
Line range hint
71-89: Consider enhancing concurrent publishing test robustness.The concurrent publishing test could be improved:
- Consider using CountDownLatch to ensure all messages are processed
- Add verification for message order preservation within same key
- Add timeout handling for edge cases
- Consider testing with different concurrency levels
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
kafka-avro/spring-boot-kafka-avro-producer/src/test/java/com/example/springbootkafkaavro/PersonKafkaPublishingIntegrationTests.java(3 hunks)kafka-dsl-integration/src/main/java/com/example/integration/kafkadsl/config/KafkaGateway.java(1 hunks)kafka-dsl-integration/src/test/java/com/example/integration/kafkadsl/ContainerConfiguration.java(1 hunks)kafka-dsl-integration/src/test/java/com/example/integration/kafkadsl/KafkaDslApplicationTests.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- kafka-dsl-integration/src/test/java/com/example/integration/kafkadsl/ContainerConfiguration.java
🔇 Additional comments (5)
kafka-dsl-integration/src/main/java/com/example/integration/kafkadsl/config/KafkaGateway.java (1)
10-10: LGTM! Interface visibility change is appropriate.Making the interface public is justified as it needs to be accessed from test classes. The interface provides a clean abstraction for Kafka messaging operations.
kafka-dsl-integration/src/test/java/com/example/integration/kafkadsl/KafkaDslApplicationTests.java (2)
13-23: LGTM! Well-structured test class setup.The test class is properly configured with necessary dependencies and appropriate Spring Boot test annotations.
41-51: Test might be flaky due to Kafka message ordering assumptions.The test assumes messages will be received in the exact order they were sent. However, Kafka only guarantees ordering within a partition. Consider:
- Using a message identifier to match sent/received messages without relying on order
- Adding partition key to ensure ordering if required
- Collecting all received messages first, then verifying all expected messages are present
kafka-avro/spring-boot-kafka-avro-producer/src/test/java/com/example/springbootkafkaavro/PersonKafkaPublishingIntegrationTests.java (2)
71-71: LGTM! Appropriate removal of unnecessary throws clause.Good cleanup - the method doesn't throw checked exceptions, so the throws clause was unnecessary.
141-141: LGTM! Good use of method references.The change from lambda expressions to method references improves code readability while maintaining the same functionality.
Also applies to: 156-156
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.java (1)
Line range hint
41-77: Fix the message count assertion in integration test.The pipeline failure indicates a message count mismatch: expected 6 but received 7 messages. This could be due to:
- Race condition in message processing
- Test order changes affecting the message count
// 4 from topic1 and 3 from topic2 on startUp, plus 1 from test await().pollInterval(Duration.ofSeconds(1)) .atMost(Duration.ofSeconds(30)) - .untilAsserted(() -> assertThat(receiver2.getLatch().getCount()).isEqualTo(initialCount - 1)); + .untilAsserted(() -> { + long currentCount = receiver2.getLatch().getCount(); + assertThat(currentCount) + .as("Expected message count to decrease by 1, initial: %d, current: %d", + initialCount, currentCount) + .isEqualTo(initialCount - 1); + });
🧹 Nitpick comments (2)
spring-modulith-outbox-pattern/src/test/java/com/example/outboxpattern/common/listener/OrderListener.java (1)
42-48: Enhance error logging with additional context.Good addition of topic information in DLT handler. Consider adding the failure reason to provide complete context for debugging.
@DltHandler - public void notifyDLT(OrderRecord event, @Header(KafkaHeaders.RECEIVED_TOPIC) String topic) { + public void notifyDLT( + OrderRecord event, + @Header(KafkaHeaders.RECEIVED_TOPIC) String topic, + @Header(KafkaHeaders.DLT_EXCEPTION_MESSAGE) String errorMessage + ) { log.error( - "Order processing failed, received in DLT - OrderId: {}, Status: {}, Items: {} from topic: {}", + "Order processing failed, received in DLT - OrderId: {}, Status: {}, Items: {} from topic: {}, Error: {}", event.id(), event.status(), event.orderItems(), - topic); + topic, + errorMessage); dlqLatch.countDown(); }spring-modulith-outbox-pattern/src/test/java/com/example/outboxpattern/order/internal/OrderModuleIntTests.java (1)
58-78: LGTM! Comprehensive test for multiple items.The test effectively verifies the order creation with multiple items and follows the established testing pattern.
However, consider adding assertions for:
- Total order amount
- Item quantities
.toArriveAndVerify(event -> { assertThat(event.orderItems().get(0).productCode()).isEqualTo("Coffee"); assertThat(event.orderItems().get(1).productCode()).isEqualTo("Tea"); + assertThat(event.orderItems().get(0).quantity()).isEqualTo(100); + assertThat(event.orderItems().get(1).quantity()).isEqualTo(50); + assertThat(event.orderItems().get(0).price()).isEqualTo(BigDecimal.TEN); + assertThat(event.orderItems().get(1).price()).isEqualTo(BigDecimal.valueOf(5)); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.java(7 hunks)spring-modulith-outbox-pattern/src/test/java/com/example/outboxpattern/common/listener/OrderListener.java(3 hunks)spring-modulith-outbox-pattern/src/test/java/com/example/outboxpattern/order/internal/OrderModuleIntTests.java(1 hunks)
🧰 Additional context used
📓 Learnings (1)
kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.java (1)
Learnt from: rajadilipkolli
PR: rajadilipkolli/kafka-experiments#624
File: kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.java:54-54
Timestamp: 2025-01-10T13:51:30.594Z
Learning: In Spring Boot Kafka integration tests using TestContainers, always wait for the KafkaContainer to be running using `kafkaContainer.isRunning()` before starting the tests to ensure Kafka is fully initialized.
🪛 GitHub Actions: spring-boot-kafka-sample
kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.java
[error] 55-55: Integration test failure: Expected 6 messages but received 7 messages within 30 seconds timeout period
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (21)
🔇 Additional comments (5)
spring-modulith-outbox-pattern/src/test/java/com/example/outboxpattern/common/listener/OrderListener.java (1)
25-28: LGTM! Well-configured retry mechanism.The retry configuration is well-structured with:
- 2 retry attempts
- Exponential backoff (1000ms initial delay with 2.0 multiplier)
- Clear topic suffixing strategy
kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.java (4)
Line range hint
126-148: LGTM! Comprehensive container state verification.The test effectively verifies the initial state of all Kafka listener containers.
Line range hint
184-214: LGTM! Thorough error handling test.Good validation of error response structure for invalid container operations.
Line range hint
216-254: LGTM! Well-structured invalid operation test.The test effectively validates the API's response to invalid operations with proper error details.
41-41: Verify test order changes impact.The reordering of tests (101, 102) might affect the message count expectations. This could be related to the pipeline failure showing 7 messages instead of expected 6.
Also applies to: 60-60
✅ Verification successful
Test order is correctly structured and does not affect message counting
The test ordering using @order(101) and @order(102) is appropriate as:
- Tests verify independent scenarios (normal message processing vs dead letter queue)
- Each test uses separate countdown latches for verification
- Proper timing controls are in place with adequate timeouts (30s and 15s respectively)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any timing-dependent assertions in the tests rg -A 5 "await\(\)|latch\.getCount\(\)" kafka-spring-boot/boot-kafka-sample/src/test/java/Length of output: 2014
Script:
#!/bin/bash # Check test method signatures and their order annotations ast-grep --pattern 'class KafkaSampleIntegrationTest { $$$ @Order($_) $_ void $name($$$) { $$$ } $$$ }'Length of output: 121
Script:
#!/bin/bash # Check test methods and their order annotations with context rg -B 2 -A 5 "@Order\([0-9]+\)" kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.javaLength of output: 1608
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spring-modulith-outbox-pattern/src/test/java/com/example/outboxpattern/order/internal/OrderModuleIntTests.java (1)
74-81: Consider enhancing assertions and using more robust item access.While the assertions are comprehensive, consider these improvements:
- Use
assertThat(event.orderItems()).hasSize(2)to explicitly verify the number of items- Consider using stream operations or finding items by product code instead of index-based access
- Add verification for the total order amount
Here's a suggested improvement:
.toArriveAndVerify(event -> { + assertThat(event.orderItems()).hasSize(2); + var coffeeItem = event.orderItems().stream() + .filter(item -> item.productCode().equals("Coffee")) + .findFirst() + .orElseThrow(); + var teaItem = event.orderItems().stream() + .filter(item -> item.productCode().equals("Tea")) + .findFirst() + .orElseThrow(); + - assertThat(event.orderItems().get(0).productCode()).isEqualTo("Coffee"); - assertThat(event.orderItems().get(1).productCode()).isEqualTo("Tea"); - assertThat(event.orderItems().get(0).quantity()).isEqualTo(100); - assertThat(event.orderItems().get(1).quantity()).isEqualTo(50); - assertThat(event.orderItems().get(0).productPrice()).isEqualTo(BigDecimal.TEN); - assertThat(event.orderItems().get(1).productPrice()).isEqualTo(BigDecimal.valueOf(5)); + assertThat(coffeeItem.quantity()).isEqualTo(100); + assertThat(coffeeItem.productPrice()).isEqualTo(BigDecimal.TEN); + assertThat(teaItem.quantity()).isEqualTo(50); + assertThat(teaItem.productPrice()).isEqualTo(BigDecimal.valueOf(5)); + + var expectedTotal = new BigDecimal("1250"); // (100 * 10) + (50 * 5) + assertThat(event.totalAmount()).isEqualTo(expectedTotal); });kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.java (1)
Line range hint
156-191: Consider splitting the container start/stop test.While the test is well-implemented, consider splitting
stopAndStartContainersinto two separate test methods:stopContainerandstartContainer. This would:
- Improve test isolation
- Make failures more specific
- Follow single responsibility principle
Example split:
- @Test - @Order(2) - void stopAndStartContainers() throws Exception { + @Test + @Order(2) + void stopContainer() throws Exception { String expectedJson = """ { "topic_2_Listener": true, "topic_2_Listener-retry": true, "topic_1_Listener": true, "topic_2_Listener-dlt": %s } """; this.mockMvcTester .post() .uri("/listeners") .content(this.objectMapper.writeValueAsString( new KafkaListenerRequest("topic_2_Listener-dlt", Operation.STOP))) .contentType(MediaType.APPLICATION_JSON) .assertThat() .hasStatusOk() .hasContentType(MediaType.APPLICATION_JSON) .bodyJson() .isEqualTo(expectedJson.formatted(false)); + } + + @Test + @Order(3) + void startContainer() throws Exception { + String expectedJson = + """ + { + "topic_2_Listener": true, + "topic_2_Listener-retry": true, + "topic_1_Listener": true, + "topic_2_Listener-dlt": %s + } + """; this.mockMvcTester .post() .uri("/listeners") .content(this.objectMapper.writeValueAsString( new KafkaListenerRequest("topic_2_Listener-dlt", Operation.START))) .contentType(MediaType.APPLICATION_JSON) .assertThat() .hasStatusOk() .hasContentType(MediaType.APPLICATION_JSON) .bodyJson() .isEqualTo(expectedJson.formatted(true)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.java(6 hunks)spring-modulith-outbox-pattern/src/test/java/com/example/outboxpattern/order/internal/OrderModuleIntTests.java(1 hunks)
🧰 Additional context used
📓 Learnings (1)
kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.java (1)
Learnt from: rajadilipkolli
PR: rajadilipkolli/kafka-experiments#624
File: kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.java:54-54
Timestamp: 2025-01-10T13:51:30.594Z
Learning: In Spring Boot Kafka integration tests using TestContainers, always wait for the KafkaContainer to be running using `kafkaContainer.isRunning()` before starting the tests to ensure Kafka is fully initialized.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build (21)
🔇 Additional comments (5)
spring-modulith-outbox-pattern/src/test/java/com/example/outboxpattern/order/internal/OrderModuleIntTests.java (2)
Line range hint
1-55: LGTM! Well-structured test infrastructure.The test class demonstrates good practices:
- Proper use of Spring Modulith test framework
- Clean separation of concerns with configuration imports
- Appropriate mocking of external dependencies
59-66: LGTM! Consistent mock setup.The Kafka mock setup follows the same pattern as the existing test, maintaining consistency in the test suite.
kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.java (3)
41-41: LGTM! Test reordering provides better organization.The reordering of tests from 1,2 to 101,102 maintains their relative sequence while providing space for additional tests. This is a good practice for test organization.
Also applies to: 68-68
56-63: Great improvement in assertion message clarity!The enhanced assertion message now includes both initial and current count values, which will make debugging test failures much easier.
Line range hint
134-155: LGTM! Well-structured test cases with comprehensive assertions.The new test methods are well-organized with clear naming, specific assertions, and proper error handling. The use of
ProblemDetailfor error responses follows Spring Boot best practices.Also applies to: 192-223, 224-264
| @Order(101) | ||
| void sendAndReceiveMessage() throws Exception { | ||
| long initialCount = receiver2.getLatch().getCount(); | ||
| this.mockMvcTester | ||
| .post() | ||
| .uri("/messages") | ||
| .content(this.objectMapper.writeValueAsString(new MessageDTO("test_1", "junitTest"))) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .exchange() | ||
| .assertThat() | ||
| .hasStatusOk(); | ||
|
|
||
| // 4 from topic1 and 3 from topic2 on startUp, plus 1 from test | ||
| await().pollInterval(Duration.ofSeconds(1)) | ||
| .atMost(Duration.ofSeconds(30)) | ||
| .untilAsserted(() -> assertThat(receiver2.getLatch().getCount()).isEqualTo(initialCount - 1)); | ||
| .untilAsserted(() -> { | ||
| long currentCount = receiver2.getLatch().getCount(); | ||
| assertThat(currentCount) | ||
| .as( | ||
| "Expected message count to decrease by 1, initial: %d, current: %d", | ||
| initialCount, currentCount) | ||
| .isEqualTo(initialCount - 1); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Add container readiness check before tests.
The current ContainerConfig relies solely on Spring Boot's container lifecycle management. While @ServiceConnection provides basic container management, an explicit readiness check would provide stronger guarantees against test flakiness, as seen in other modules of this project.
Consider adding a @BeforeAll method:
@BeforeAll
static void waitForKafka(@Autowired KafkaContainer kafkaContainer) {
await()
.atMost(Duration.ofSeconds(30))
.until(kafkaContainer::isRunning);
}🔗 Analysis chain
Add container readiness check before tests.
Based on previous learning from PR #624, it's important to verify that the Kafka container is running before executing tests to prevent flaky behavior.
Consider adding a @BeforeAll method:
@BeforeAll
static void waitForKafka(@Autowired KafkaContainer kafkaContainer) {
await()
.atMost(Duration.ofSeconds(30))
.until(kafkaContainer::isRunning);
}🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for KafkaContainer usage and configuration
echo "=== Searching for KafkaContainer usage ==="
rg -l "KafkaContainer" --type java
echo -e "\n=== Examining test class implementation ==="
cat "kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/KafkaSampleIntegrationTest.java"
echo -e "\n=== Looking for container readiness checks ==="
rg "isRunning|waitingFor|await.*until.*Running" --type java
Length of output: 13330
Script:
#!/bin/bash
# Examine the container configuration
echo "=== Examining ContainerConfig implementation ==="
cat "kafka-spring-boot/boot-kafka-sample/src/test/java/com/example/springbootkafkasample/common/ContainerConfig.java"
Length of output: 963
Summary by CodeRabbit
Release Notes
New Features
Tests
Refactor
Chores