New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Example to test actual implementation with TopologyTestDriver #219

Merged

bbejeck merged 13 commits into confluentinc:5.2.0-post from jukkakarvanen:TopologyTestDriver_tests

Apr 17, 2019

Contributor

jukkakarvanen commented Apr 2, 2019

Readme states: "These examples are also a good starting point to learn how to implement your own end-to-end integration tests.". I disagree. See testing instructions:
https://docs.confluent.io/current/streams/developer-guide/test-streams.html

Now the Integration Tests are mainly copy/paste of actual implementation and not testing actual implementation class at all. The most of the test could also be done without EmbeddedKafka with TopologyTestDriver.

In this pull request there is a new WordCountLambdaExampleTest added utilizing TopologyTestDriver.
To be able to test WordCountLambdaExample implementation of WordCountLambdaExample main method needed to be refactored, so actual configuration and Stream building are own methods.

WordCountLambdaIntegrationTest class is also modified to utilize Stream logic of actual application and that way avoid code duplication.

The same logic should be applied also to other tests.

jukkakarvanen added 10 commits

April 2, 2019 14:38


          TopologyTestDriver based unit test, Refactored Sample to support Unit…

49ffbd2

…Test


          Comments added

24432c1


          Readme update

9e84af3


          Integration test to use actual implementation instead of duplicating …

e83d1ac

…code.


          Readme update

b360dd7


          Code clean up

cc79ba0


          Code clean up

b4d792d


          Readme update

89515f0


          checkstyle checks

d72334c


          checkstyle checks

5e5cd38

ghost commented Apr 2, 2019

It looks like @jukkakarvanen hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

jukkakarvanen mentioned this pull request

TopologyTestDriver based unit tests kaiwaehner/kafka-streams-machine-learning-examples#11

Closed

Contributor Author

jukkakarvanen commented Apr 2, 2019

[clabot:check]

ghost commented Apr 2, 2019

@confluentinc It looks like @jukkakarvanen just signed our Contributor License Agreement. 👍

Always at your service,

clabot

jukkakarvanen changed the title ~~Example for actual implementaion test with TopologyTestDriver~~ Example to test actual implementation with TopologyTestDriver

bbejeck requested changes

View reviewed changes

Contributor

bbejeck left a comment

Thanks for the contribution @jukkakarvanen! Over this looks good I just have a few minor comments.

README.md Outdated

-                that demonstrate end-to-end data pipelines.  Here, we use a testing framework to automatically spawn embedded Kafka
+              * **Examples under [src/test/](src/test/)**: These examples should test applications under [src/main/](src/main/).
+                Unit Tests with TopologyTestDriver test the stream logic without external system dependencies.
+                The integration tests use a testing framework to automatically spawn embedded Kafka

Contributor

bbejeck Apr 8, 2019

nit: integration tests use a testing framework to automatically spawn -> integration tests use an embedded Kafka

Contributor Author

jukkakarvanen Apr 13, 2019

Changed.

src/test/java/io/confluent/examples/streams/WordCountLambdaExampleTest.java Outdated

+                private ConsumerRecordFactory<String, String> recordFactory = new ConsumerRecordFactory<>(new StringSerializer(), new StringSerializer());
+                @Before
+                public void setup() throws IllegalAccessException, ClassNotFoundException, InstantiationException {

Contributor

bbejeck Apr 12, 2019

Can remove the throws clause and all exceptions

Contributor Author

jukkakarvanen Apr 13, 2019

Done

src/test/java/io/confluent/examples/streams/WordCountLambdaExampleTest.java Outdated

+              /**
+               * Stream processing unit test of {@link WordCountLambdaExample}, using TopologyTestDriver.
+               *
+               * @author Jukka Karvanen / jukinimi.com

Contributor

bbejeck Apr 12, 2019

nit: we usually don't have any author names in the javadoc

Contributor Author

jukkakarvanen Apr 13, 2019

Removed

src/test/java/io/confluent/examples/streams/WordCountLambdaExampleTest.java Outdated

		}


		/** Read one Record from output topic.

Contributor

bbejeck Apr 12, 2019

nit: move the text one line below the start of the javadoc

Contributor Author

jukkakarvanen Apr 13, 2019

Done

src/test/java/io/confluent/examples/streams/WordCountLambdaExampleTest.java Outdated

+                  return testDriver.readOutput(WordCountLambdaExample.outputTopic, stringDeserializer, longDeserializer);
+                }
+                /** Read counts from output to map.

Contributor

bbejeck Apr 12, 2019

ditto here and elsewhere below

Contributor Author

jukkakarvanen Apr 13, 2019

Done

src/test/java/io/confluent/examples/streams/WordCountLambdaExampleTest.java

+               */
+              package io.confluent.examples.streams;
+              import org.apache.kafka.clients.producer.ProducerRecord;

Contributor

bbejeck Apr 12, 2019

nit: remove wildcard imports

Contributor Author

jukkakarvanen Apr 13, 2019

Ok, Changed IDEA settings.

Contributor Author

jukkakarvanen Apr 13, 2019

Done

src/main/java/io/confluent/examples/streams/WordCountLambdaExample.java Outdated

@@ @@ -111,8 +111,48 @@ @@
                */
               public class WordCountLambdaExample {
+                static final String inputTopic = "inputTopic";
+                static final String outputTopic = "outputTopic";
                 public static void main(final String[] args) throws Exception {

Contributor

bbejeck Apr 12, 2019

nit: can rid of throws Exception

Contributor Author

jukkakarvanen Apr 13, 2019

Removed (Old problem,)

src/main/java/io/confluent/examples/streams/WordCountLambdaExample.java Outdated

@@ @@ -111,8 +111,48 @@ @@
                */
               public class WordCountLambdaExample {
+                static final String inputTopic = "inputTopic";

Contributor

bbejeck Apr 12, 2019

These topic names need to match the names of the directions in the javadoc
inputTopic= streams-plaintext-input
outputTopic = streams-wordcount-output

Contributor Author

jukkakarvanen Apr 13, 2019

Good catch. Fixed.

Contributor Author

jukkakarvanen Apr 13, 2019

Problem was inherited from WordCountLambdaIntegrationTest.java

bbejeck requested review from vvcephei, mjsax, ableegoldman, cadonna and guozhangwang

April 12, 2019 14:54


          Changes based on the Review of bbejeck

8bb78c1

vvcephei reviewed

View reviewed changes

Member

vvcephei left a comment

Hey @jukkakarvanen ,

You have an excellent point. I'm wondering if we should even keep the integration test at all. The application logic of the WordCountExample depends purely on Streams; it has no logical interactions with the broker. Streams already guarantees that it works with the broker, so it seems like the only necessary test is to verify the application logic, which is better done with the TopologyTestDriver.

What do you think about removing the integration test entirely?

(the code looks good to me, by the way)


          Updated comment

1e87f30

Contributor Author

jukkakarvanen commented Apr 13, 2019

No idea why previous test failed due to WordCountScalaIntegrationTest failure and now it is successful again.

Member

vvcephei commented Apr 13, 2019

These integration tests are quite flaky, which is part of my motivation to just switch to TopologyTestDriver.

Contributor Author

jukkakarvanen commented Apr 14, 2019

You have an excellent point. I'm wondering if we should even keep the integration test at all. The application logic of the WordCountExample depends purely on Streams; it has no logical interactions with the broker. Streams already guarantees that it works with the broker, so it seems like the only necessary test is to verify the application logic, which is better done with the TopologyTestDriver.

What do you think about removing the integration test entirely?

@vvcephei, Yes, I don't see the point of the this kind of integration test with EmbeddedKafka at all, if you want to test Kafka Stream application logic. I understand the use case of EmbeddedSingleNodeKafkaCluster if the application contains also consumers or producers, but not in pure Kafka Streams.

Maybe somebody knowing the internals of the TopologyTestDriver could comment is there possibility of difference, but I have found TopologyTestDriver a fast and reliable way of the testing the Kafka Stream application.

Member

mjsax commented Apr 14, 2019

It's historical reasons. When the examples where created, there was no TopologyTestDriver yet. And we never invested time to rewrite the tests later.

mjsax mentioned this pull request

Add custom stream-table join example where the table triggers updates, too #232

Merged

Contributor Author

jukkakarvanen commented Apr 15, 2019

FYI: There are PR apache/kafka#6569 to some point of time get rid of try / catch in tearDown, I needed to add because I am working with Windows.

I also working to propose some extra Helper class to make simplify the use of TopologyTestDriver to be added kafka-streams-test-utils (and possible independent package for old Kafka versions)

I added an example of how those class could be used in this WordCountLambdaExampleTest in separate brach of my repo: jukkakarvanen/kafka-streams-examples@TopologyTestDriver_tests...jukkakarvanen:InputOutputTopic

Member

vvcephei commented Apr 15, 2019

Thanks @jukkakarvanen ,

It sounds like we can actually go ahead and just get rid of the WordCountScalaIntegrationTest and just use your new test instead.

Thanks also for those other PRs. I'd say, we should just consider them all independently. If we merge the other ones first, we can update this PR, but if we merge this PR first, we can always use a new PR to improve the test further.

bbejeck approved these changes

View reviewed changes

Contributor

bbejeck commented Apr 16, 2019

@jukkakarvanen thanks for the contribution!
To follow up on @vvcephei's comment If we can get rid of the WordCountScalaIntegrationTest in this PR then we can just the new test instead


          WordCountLambdaIntegrationTest removed based on @bbejeck comment

1fe965b

Contributor Author

jukkakarvanen commented Apr 16, 2019

@bbejeck WordCountScalaIntegrationTest class and references to it removed

Contributor

bbejeck commented Apr 17, 2019

I pulled down the PR and verified the WordCountLambdaExample

bbejeck merged commit 5f331ca into confluentinc:5.2.0-post

Contributor

bbejeck commented Apr 17, 2019

Thanks @jukkakarvanen !

Contributor

bbejeck commented Apr 17, 2019

merged with pint

Member

mjsax commented Apr 18, 2019

Sorry that I am late to the game. TopologyTestDriver was introduced in AK 1.1, thus I am wondering if this should have gone to 4.1.0-post? Maybe we can still backport it?

jukkakarvanen mentioned this pull request

README Link pointing to removed file #244

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vvcephei vvcephei left review comments

bbejeck bbejeck approved these changes

mjsax Awaiting requested review from mjsax

ableegoldman Awaiting requested review from ableegoldman

cadonna Awaiting requested review from cadonna

guozhangwang Awaiting requested review from guozhangwang

Labels

None yet