KAFKA-6474: Rewrite tests to use new public TopologyTestDriver [cleanup] #4939

h314to · 2018-04-27T17:07:03Z

This implements the suggestions made after the previous PR for KAFKA-6474 was merged.

The majority of changes deals with using try-with-resources and a new method in StreamsTestUtils to set the test properties and instantiate the TopologyTestDriver, thus allowing the removal of the cumbersome @Before and @After methods.

I also replaced stringSerde and intSerde variables with (almost equally succinct) inline calls to Serdes.String() and Serdes.Integer().

Add method to create test properties to StreamsTestUtils
Make TopologyTestDriver protected constructor package-private
Add comment suggesting the use of TopologyTestDriver to KStreamTestDriver
Cleanup:
- GlobalKTableJoinsTest
- KGroupedStreamImplTest
- KGroupedTableImplTest
- KStreamBranchTest
- KStreamFilterTest
- KStreamFlatMapTest
- KStreamFlatMapValuesTest
- KStreamForeachTest
- KStreamGlobalKTableJoinTest
- KStreamGlobalKTableLeftJoinTest
- KStreamImplTest
- KStreamKStreamJoinTest
- KStreamKStreamLeftJoinTest
- KStreamGlobalKTableLeftJoinTest
- KStreamKTableJoinTest
- KStreamKTableLeftJoinTest
- KStreamMapTest
- KStreamMapValuesTest
- KStreamPeekTest
- StreamsBuilderTest
- KStreamSelectKeyTest
- KStreamTransformTest
- KStreamTransformValuesTest
- KStreamWindowAggregateTest
- KTableForeachTest

vvcephei · 2018-04-27T20:42:46Z

...rc/test/java/org/apache/kafka/streams/kstream/internals/KStreamGlobalKTableLeftJoinTest.java

        driver = new TopologyTestDriver(builder.build(), props);
    }

+    @After
+    public void cleanup() {
+        if (driver != null) {


This should never happen, right? maybe we just don't check it and get an NPE if somehow driver gets set to null after the setup.

Yes, since the driver in always created in @Before, something would have to be very wrong with a test for the driver to be null. I'll get rid of that check on @After.

vvcephei · 2018-04-27T20:46:24Z

streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamKTableJoinTest.java

        driver = new TopologyTestDriver(builder.build(), props, 0L);
    }

+    @After
+    public void cleanup() {
+        if (driver != null) {


ditto on (what I think is) the impossibility of this condition being false.

vvcephei · 2018-04-27T20:46:38Z

streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamKTableLeftJoinTest.java

        driver = new TopologyTestDriver(builder.build(), props, 0L);
    }

+    @After
+    public void cleanup() {
+        if (driver != null) {


vvcephei

LGTM! Sorry for the extra work, but thanks for cleaning this stuff up.

I just had one minor thought that checking for the driver potentially being null should be unnecessary, so maybe we should just take it out. But it's your call.

Thanks again,
-John

vvcephei · 2018-04-27T21:18:23Z

@guozhangwang @bbejeck Can you take a look also? These were changes I requested after the last PR got merged.

h314to · 2018-04-27T23:46:13Z

No problem @vvcephei . I already knew my way around these classes so implementing your suggestions was fairly straightforward. The code is definitely much clearer now. Thanks!

bbejeck

Sorry for the late response. Thanks for the follow-up PR @h314to. LGTM.

bbejeck · 2018-05-04T15:49:00Z

@h314to, sorry for the bother, but can you rebase this?

guozhangwang · 2018-05-04T16:40:13Z

@h314to Could you rebase the PR again so it can be merged?

About KStreamTestDriver itself, are you planning to remove it after fixing all its dependent tests in another PR?

h314to · 2018-05-05T14:46:56Z

Yes, no problem. I'll squash the commits and rebase on the current trunk.

@guozhangwang I'm currently working on migrating the remaining classes from KStreamTestDriver to TopologyTestDriver, which I'll submit a subsequent PR (there's only about 8 classes left). I'm not sure if all the functionality in KStreamTestDriver is currently supported by TopologyTestDriver. If so, I was going to remove it like the Jira ticket requests. But I'll follow the community's best wisdom on this. It might be wise to keep it deprecated for a while before removing it. WDYT?

@after

* Add method to create test properties to StreamsTestUtils * Make TopologyTestDriver protected constructor package-private * Add comment suggesting the use of TopologyTestDriver to KStreamTestDriver * Cleanup: - GlobalKTableJoinsTest - KGroupedStreamImplTest - KGroupedTableImplTest - KStreamBranchTest - KStreamFilterTest - KStreamFlatMapTest - KStreamFlatMapValuesTest - KStreamForeachTest - KStreamGlobalKTableJoinTest - KStreamGlobalKTableLeftJoinTest - KStreamImplTest - KStreamKStreamJoinTest - KStreamKStreamLeftJoinTest - KStreamGlobalKTableLeftJoinTest - KStreamKTableJoinTest - KStreamKTableLeftJoinTest - KStreamMapTest - KStreamMapValuesTest - KStreamPeekTest - StreamsBuilderTest - KStreamSelectKeyTest - KStreamTransformTest - KStreamTransformValuesTest - KStreamWindowAggregateTest - KTableForeachTest * Address reviewer's comments: - Remove check for null driver in @after

guozhangwang · 2018-05-05T18:56:59Z

@h314to Thanks for the update @h314to , much appreciated.

About deprecating KStreamTestDriver, since it is not part of the public API we do not need to mark it deprecated, but I'm curious to learn which functionality gap do you see between it and the TopologyTestDriver? If there are we could consider adding them into the latter.

h314to · 2018-05-07T14:52:34Z

@guozhangwang I think the functionality gaps are mostly due to the balance between keeping the TopologyTestDriver clean and no-nonsense (which obviously has priority, since it is part of the public API), while still having access to stuff needed for internal testing.

For instance, we need to access the StreamTask processor context to init a KTableValueGetter, but it is probably not wise to expose it publicly. This was causing me trouble in several test classes. In the end I got around it by making it protected and providing access via a wrapper class that extends TopologyTestDriver, like I did for the constructor here. This will be included in my next PR, which will follow shortly.

guozhangwang · 2018-05-07T16:17:00Z

@h314to Thanks for the information. That makes sense to me.

guozhangwang · 2018-05-07T16:21:29Z

Merged to trunk.

…up] (apache#4939) * Add method to create test properties to StreamsTestUtils * Make TopologyTestDriver protected constructor package-private * Add comment suggesting the use of TopologyTestDriver to KStreamTestDriver * Cleanup: - GlobalKTableJoinsTest - KGroupedStreamImplTest - KGroupedTableImplTest - KStreamBranchTest - KStreamFilterTest - KStreamFlatMapTest - KStreamFlatMapValuesTest - KStreamForeachTest - KStreamGlobalKTableJoinTest - KStreamGlobalKTableLeftJoinTest - KStreamImplTest - KStreamKStreamJoinTest - KStreamKStreamLeftJoinTest - KStreamGlobalKTableLeftJoinTest - KStreamKTableJoinTest - KStreamKTableLeftJoinTest - KStreamMapTest - KStreamMapValuesTest - KStreamPeekTest - StreamsBuilderTest - KStreamSelectKeyTest - KStreamTransformTest - KStreamTransformValuesTest - KStreamWindowAggregateTest - KTableForeachTest Reviewers: John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

vvcephei reviewed Apr 27, 2018

View reviewed changes

vvcephei approved these changes Apr 27, 2018

View reviewed changes

bbejeck approved these changes May 3, 2018

View reviewed changes

h314to force-pushed the fix/KAFKA-6474-cleanup branch from dc57565 to 87ffe7a Compare May 5, 2018 16:18

guozhangwang merged commit 6f641fe into apache:trunk May 7, 2018

h314to deleted the fix/KAFKA-6474-cleanup branch May 7, 2018 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-6474: Rewrite tests to use new public TopologyTestDriver [cleanup] #4939

KAFKA-6474: Rewrite tests to use new public TopologyTestDriver [cleanup] #4939

h314to commented Apr 27, 2018

vvcephei Apr 27, 2018

h314to Apr 27, 2018 •

edited

Loading

vvcephei Apr 27, 2018

vvcephei Apr 27, 2018

vvcephei left a comment

vvcephei commented Apr 27, 2018

h314to commented Apr 27, 2018

bbejeck left a comment

bbejeck commented May 4, 2018

guozhangwang commented May 4, 2018 •

edited

Loading

h314to commented May 5, 2018

guozhangwang commented May 5, 2018

h314to commented May 7, 2018 •

edited

Loading

guozhangwang commented May 7, 2018

guozhangwang commented May 7, 2018

KAFKA-6474: Rewrite tests to use new public TopologyTestDriver [cleanup] #4939

KAFKA-6474: Rewrite tests to use new public TopologyTestDriver [cleanup] #4939

Conversation

h314to commented Apr 27, 2018

vvcephei Apr 27, 2018

Choose a reason for hiding this comment

h314to Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

vvcephei Apr 27, 2018

Choose a reason for hiding this comment

vvcephei Apr 27, 2018

Choose a reason for hiding this comment

vvcephei left a comment

Choose a reason for hiding this comment

vvcephei commented Apr 27, 2018

h314to commented Apr 27, 2018

bbejeck left a comment

Choose a reason for hiding this comment

bbejeck commented May 4, 2018

guozhangwang commented May 4, 2018 • edited Loading

h314to commented May 5, 2018

guozhangwang commented May 5, 2018

h314to commented May 7, 2018 • edited Loading

guozhangwang commented May 7, 2018

guozhangwang commented May 7, 2018

h314to Apr 27, 2018 •

edited

Loading

guozhangwang commented May 4, 2018 •

edited

Loading

h314to commented May 7, 2018 •

edited

Loading