GH-666: Single global embedded Kafka with JUnit 5 #2308
Conversation
|
If this is OK, I'll go ahead with docs and perhaps some sample how to enable and configure this via Gradle which might be really a top interest from end-users. (Perhaps the number of issue is so bad that it took for me a couple days to figure out how to test this 😢 😄 ) |
|
Looks like your IDE is messing with Git again: 20bf523 |
There was a problem hiding this comment.
I don't think I can review this adequately because I am not familiar with the concepts. Perhaps we can ask @sbrannen to take a look?
No problem, Gary! From your side I just need 👍 about a general idea to have such a global embedded Kafka for the whole test plan as it is requested by users and as they demonstrate it via their custom Gradle plugins. Re. Git commit: I think I had several original commits before pushing and probably when I wanted to push to PR as a single commit I accidentally included one of yours then rebased. |
Yes; I think it's a good idea. |
...main/java/org/springframework/kafka/test/junit/GlobalEmbeddedKafkaTestExecutionListener.java
Show resolved
Hide resolved
...main/java/org/springframework/kafka/test/junit/GlobalEmbeddedKafkaTestExecutionListener.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,3 @@ | |||
| spring.kafka.embedded.count=2 | |||
| spring.embedded.kafka.brokers.property=spring.kafka.bootstrap-servers | |||
There was a problem hiding this comment.
Should this instead be spring.kafka.embedded.broker.properties.location?
Or is that a property not used by your TestExecutionListener?
There was a problem hiding this comment.
This is used by the broker itself (it tells the broker to update that system property - useful in Boot tests) - @artembilan I think we should add docs about this, especially for Boot users.
There was a problem hiding this comment.
Oh! Sorry, you just answered to Sam's question :shame:
But yeah, I have added more in docs anyway 😄
...java/org/springframework/kafka/test/junit/GlobalEmbeddedKafkaTestExecutionListenerTests.java
Outdated
Show resolved
Hide resolved
|
Thank you, @sbrannen , for consultation and review! From here I guess we don't need to bother you any more and @garyrussell simple can continue to review the rest of the changes. |
There was a problem hiding this comment.
The sample-05 tests fail for me
[ERROR] Errors:
[ERROR] Sample05Application1Tests.testKafkaListener:49 » ConditionTimeout Condition with com.example.Sample05Application1Tests was not fulfilled within 10 seconds.
[ERROR] Sample05Application2Tests.testKafkaTemplateSend:53 » Kafka Send failed
[INFO]
[ERROR] Tests run: 2, Failures: 0, Errors: 2, Skipped: 0
More info for the second one (because it waits for the future)...
Caused by: org.apache.kafka.common.errors.TimeoutException: Topic nonExistingTopic not present in metadata after 1000 ms.
samples/sample-05/src/test/java/com/example/Sample05Application1Tests.java
Show resolved
Hide resolved
| @@ -0,0 +1,3 @@ | |||
| spring.kafka.embedded.count=2 | |||
| spring.embedded.kafka.brokers.property=spring.kafka.bootstrap-servers | |||
There was a problem hiding this comment.
This is used by the broker itself (it tells the broker to update that system property - useful in Boot tests) - @artembilan I think we should add docs about this, especially for Boot users.
Sorry, I forgot to mention that you need to |
|
I already did that - it won't compile otherwise. |
Sorry, it is not clear what is your concern over here. This indeed was my intention to demonstrate how that property is populated by the embedded property and can be used by Spring Boot auto-configuration.
We do have some mentioning about that setting in our docs: https://docs.spring.io/spring-kafka/docs/current/reference/html/#testing. Thanks |
Fixes spring-projects#666 * Introduce a `GlobalEmbeddedKafkaTestExecutionListener` which is registered by the service loader from JUnit Platform * Make its activation conditional based on the `spring.kafka.global.embedded.enabled` system property * Expose some other configuration properties for the target global `EmbeddedKafkaBroker` * Verify its functionality via manual `Launcher.execute()` * Add more `@DirtiesContext` to some tests in the `spring-kafka-test` which don't close their embedded brokers on the exit
this is part of JUnit platform the test plan is going to be run * Add missed new line in the end of `junit-platform.properties` * Rethrow an exception from the local launcher in the `GlobalEmbeddedKafkaTestExecutionListenerTests` to fail if its suite test classes have failed.
* Move system properties clean up in the `GlobalEmbeddedKafkaTestExecutionListenerTests` into `@AfterAll` * Add docs for this new feature * Add `sample-05` to demonstrate global embedded Kafka in action via Maven configuration
|
Fixed the first one with |
|
Of course, the second one fails - The future will be completed with the timeout exception. |
|
I'm getting that |
That's correct. See JavaDocs of that |
|
Right, but it fails under maven as well; we should be asserting the root cause as being the timeout exception due to metadata not present. This is the full stack trace: |
That's correct as well. The sample is about showing a global embedded Kafka, not how to right tests. I also say that explicitly in the
I'm not sure why you still insist that it has to pass... |
* Add new lines into new files * Add `spring.kafka.consumer.auto-offset-reset=earliest` to make a consumer to start from the beginning of the partition: fixes a race condition when publishing happens before consumer is started * Make `mvnw` as an executable * Add `spring.embedded.kafka.brokers.property` explanation into docs
| @Test | ||
| void testKafkaTemplateSend() throws ExecutionException, InterruptedException, TimeoutException { | ||
| SendResult<String, String> sendResult = | ||
| this.kafkaTemplate.send("nonExistingTopic", "fake data").get(10, TimeUnit.SECONDS); | ||
|
|
||
| assertThat(sendResult).isNotNull(); | ||
| } |
There was a problem hiding this comment.
Because it's ugly seeing all those errors on the command line - I think this is better:
| @Test | |
| void testKafkaTemplateSend() throws ExecutionException, InterruptedException, TimeoutException { | |
| SendResult<String, String> sendResult = | |
| this.kafkaTemplate.send("nonExistingTopic", "fake data").get(10, TimeUnit.SECONDS); | |
| assertThat(sendResult).isNotNull(); | |
| } | |
| @Test | |
| void testKafkaTemplateSend() { | |
| Throwable thrown = catchThrowable(() -> | |
| this.kafkaTemplate.send("nonExistingTopic", "fake data").get(10, TimeUnit.SECONDS)); | |
| assertThatExceptionOfType(TimeoutException.class).isThrownBy(() -> { | |
| throw thrown.getCause(); | |
| }) | |
| .withMessageContaining("Topic nonExistingTopic") | |
| .withMessageContaining("metadata"); | |
| } |
There was a problem hiding this comment.
😸 I still think my idea to demonstrate a robustness of our solution even if some tests are failing in between is the way to go for this sample.
On the other hand we just can remove this sample altogether: for me clean finish for the plan is OK even if the report is about failure.
The successful tests was really not a goal for me in this sample, but rather how global embedded Kafka works.
See also GlobalEmbeddedKafkaTestExecutionListenerTests: one if its tests is also deliberately failing.
There was a problem hiding this comment.
I just don't like to see the big stack trace, and having multiple tests clearly demonstrates that there is only one broker; reducing to 1 test, we'll lose that.
Co-authored-by: Gary Russell <[email protected]>
* Remove `deliberately failing` sentence from the JavaDoc an README * Clean up unused imports
Fixes #666
GlobalEmbeddedKafkaTestExecutionListenerwhich is registeredby the service loader from JUnit Platform
spring.kafka.global.embedded.enabledsystem property
EmbeddedKafkaBrokerLauncher.execute()@DirtiesContextto some tests in thespring-kafka-testwhich don't closetheir embedded brokers on the exit