KAFKA-4594: Annotate integration tests and provide gradle build targets to run subsets of tests #2695

dguy · 2017-03-16T14:42:03Z

This uses junit Categories to identify integration tests. Adds 2 new build targets: integrationTest and testAll. The test target will just run the unit tests

dguy · 2017-03-16T14:45:40Z

@ijuma please take a look. New target to run integration tests is integrationTest
There is at least one test in core LogCleanerIntegrationTest that is not annotated. This is because it is Parameterized, so uses RunWith already. I didn't really feel like re-writing the test

ijuma

Thanks for the PR, I had a quick look and left a couple of comments.

A bit unfortunate that @Categories and @RunWith don't work together, but the benefit seems worth it.

ijuma · 2017-03-16T14:45:10Z

build.gradle

+    maxParallelForks = userMaxForks ?: Runtime.runtime.availableProcessors()
+
+    minHeapSize = "256m"
+    maxHeapSize = "2048m"


I wonder if we should reduce maxHeapSize for the unit tests.

ijuma · 2017-03-16T14:45:35Z

build.gradle

  }

+  task testAll(dependsOn: ['test', 'integrationTest'])


testAll currently runs all the tests for all Scala versions. So looks like this clashes with that. Could we have unitTest, integrationTest, test (unit and integration for the default Scala version) and testAll (unit and integration for all Scala versions)?

ok - didn't realise that. I'll have a look

ijuma · 2017-03-16T14:47:09Z

core/src/test/scala/integration/kafka/admin/BrokerApiVersionsCommandTest.scala


+@Category(Array(classOf[IntegrationTest]))


For the Core tests, could we just annotate ZooKeeperTestHarness? Seems like that would do the job.

I tried that, but it doesn't seem to like it because it is a Trait

Discussed this offline, seems like we can simply change the traits to be abstract classes. The code compiles fine.

asfbot · 2017-03-16T14:51:27Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2218/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-16T15:00:45Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2219/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-03-16T15:00:47Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2221/
Test PASSed (JDK 8 and Scala 2.11).

dguy · 2017-03-16T16:42:58Z

@ijuma done

asfbot · 2017-03-16T17:26:12Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2228/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-16T17:29:02Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2229/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-03-16T17:29:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2231/
Test PASSed (JDK 8 and Scala 2.11).

ijuma · 2017-03-16T17:39:57Z

Thanks for the updates, the overall approach looks good. Can we also update Readme.md to include information about the new tasks?

asfbot · 2017-03-16T18:26:38Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2234/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-03-16T19:10:04Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2231/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-16T19:37:36Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2232/
Test PASSed (JDK 8 and Scala 2.12).

ijuma · 2017-03-17T11:05:22Z

@enothereska, can you please take a look at the changes in the Streams tests? @dguy, there are some conflicts due to a recently merged Streams PR.

asfbot · 2017-03-17T12:03:56Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2246/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-03-17T12:05:14Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2243/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-17T12:22:51Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2244/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-03-17T12:22:57Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2247/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-03-17T12:23:01Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2244/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-03-17T12:30:07Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2248/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-03-17T12:30:09Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2245/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-03-17T12:30:18Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2245/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-03-17T14:44:42Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2248/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-03-17T14:45:44Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2248/
Test PASSed (JDK 7 and Scala 2.10).

enothereska · 2017-03-17T15:25:55Z

jenkins.sh

@@ -0,0 +1,16 @@
+#!/bin/bash


Why is this script needed?

It's to avoid duplication in various Jenkins jobs. We can just invoke this script and pass any extra parameters (like Scala version).

enothereska · 2017-03-17T15:28:04Z

This seems to indicate we can have both Parametrized and Categories? junit-team/junit4@37a8aab

ijuma · 2017-03-17T15:39:47Z

@enothereska Great find. I found it too and was happy about it until I noticed that it was only fixed in 4.13, which hasn't been released yet.

enothereska · 2017-03-17T15:53:58Z

Ok, LGTM, I guess we might have to revisit when Junit 5 comes out, since 4.13 will probably never come out.

asfbot · 2017-03-17T17:00:42Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2251/
Test FAILed (JDK 8 and Scala 2.11).

ijuma · 2017-03-20T11:49:11Z

build.gradle

@@ -892,6 +927,7 @@ project(':connect:api') {
    testCompile libs.junit

    testRuntime libs.slf4jlog4j
+    testCompile project(':clients').sourceSets.test.output


@ewencp, are you OK with this and similar changes to the connect test dependencies?

Yeah, I'm not worried about changes in test dependencies.

ijuma

Thanks for the updates. I had a closer and left a few additional comments. I think it's ready to merge after that.

ijuma · 2017-03-20T11:50:46Z

jenkins.sh

+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+./gradlew clean compileJava compileScala compileTestJava compileTestScala checkstyleMain checkstyleTest unitTest integrationTest --no-daemon -Dorg.gradle.project.testLoggingEvents=started,passed,skipped,failed $@


Can we please add a comment explaining the purpose of the script for other people that came across it?

Also, I think there should be double quotes surrounding $@. This is necessary in case there are spaces involved.

ijuma · 2017-03-20T11:51:46Z

streams/src/test/java/org/apache/kafka/streams/integration/FanoutIntegrationTest.java

@@ -116,7 +105,6 @@ public void shouldFanoutTheInput() throws Exception {
        streamsConfiguration.put(StreamsConfig.VALUE_SERDE_CLASS_CONFIG, Serdes.String().getClass().getName());
        streamsConfiguration.put(StreamsConfig.COMMIT_INTERVAL_MS_CONFIG, COMMIT_INTERVAL_MS);
        streamsConfiguration.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
-        streamsConfiguration.put(StreamsConfig.CACHE_MAX_BYTES_BUFFERING_CONFIG, cacheSizeBytes);


Did we decide that there's not much benefit in running the test with multiple cache sizes? What is the default cache size?

There is no benefit in this test as it doesn't use the cache at all, i.e., only KTables use it.

Thanks for the explanation.

ijuma · 2017-03-20T11:53:46Z

...ams/src/test/java/org/apache/kafka/streams/integration/KStreamKTableJoinIntegrationTest.java

@@ -93,9 +90,10 @@ public void before() throws InterruptedException {
        streamsConfiguration.put(StreamsConfig.VALUE_SERDE_CLASS_CONFIG, Serdes.String().getClass().getName());
        streamsConfiguration.put(StreamsConfig.COMMIT_INTERVAL_MS_CONFIG, COMMIT_INTERVAL_MS);
        streamsConfiguration.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
+        streamsConfiguration.put(StreamsConfig.COMMIT_INTERVAL_MS_CONFIG, 300);


Seems like this line should be deleted since we have the same config a couple of lines above?

ijuma · 2017-03-20T11:55:55Z

...ams/src/test/java/org/apache/kafka/streams/integration/KStreamKTableJoinIntegrationTest.java

-        userClicksProducerConfig.put(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG, LongSerializer.class);
-        IntegrationTestUtils.produceKeyValuesSynchronously(userClicksTopic, userClicks, userClicksProducerConfig, mockTime);
-
-        //


This may be obvious to others, but for my benefit, why was this code moved?

I'll change it back as it isn't actually needed. Just how i like to see it laid out.

ijuma · 2017-03-20T11:59:52Z

streams/src/test/java/org/apache/kafka/streams/integration/QueryableStateIntegrationTest.java

@@ -147,8 +136,8 @@ public void before() throws IOException, InterruptedException {
        streamsConfiguration.put(StreamsConfig.STATE_DIR_CONFIG, TestUtils.tempDirectory("qs-test").getPath());
        streamsConfiguration.put(StreamsConfig.KEY_SERDE_CLASS_CONFIG, Serdes.String().getClass());
        streamsConfiguration.put(StreamsConfig.VALUE_SERDE_CLASS_CONFIG, Serdes.String().getClass());
-        streamsConfiguration.put(StreamsConfig.CACHE_MAX_BYTES_BUFFERING_CONFIG, cacheSizeBytes);
-        streamsConfiguration.put(StreamsConfig.COMMIT_INTERVAL_MS_CONFIG, 1000);
+        streamsConfiguration.put(StreamsConfig.VALUE_SERDE_CLASS_CONFIG, Serdes.String().getClass());


Seems like we have this line twice?

ijuma · 2017-03-20T12:05:08Z

build.gradle

+    testLogging {
+      events = userTestLoggingEvents ?: ["passed", "skipped", "failed"]
+      showStandardStreams = userShowStandardStreams ?: false
+      exceptionFormat = 'full'


Would it make sense to introduce variables for all these magic values (e.g. "full", ["passed", "skipped", "failed"], etc.)? This way if someone wants to change that, they don't have to change it 3 times (or worse, change it in one place when they meant to change it in every place.

ijuma · 2017-03-20T12:06:22Z

README.md

-    ./gradlew core:test --tests kafka.api.ProducerFailureHandlingTest.testCannotSendToInternalTopic
-    ./gradlew clients:test --tests org.apache.kafka.clients.MetadataTest.testMetadataUpdateWaitTime
+    ./gradlew core:unitTest --tests kafka.api.ProducerFailureHandlingTest.testCannotSendToInternalTopic
+    ./gradlew clients:unitTest --tests org.apache.kafka.clients.MetadataTest.testMetadataUpdateWaitTime


Seems like core:test is better since if you are running a particular test, you don't care if it's unit or integration? I'd change within a unit test to within a unit/integration test in the header instead.

I'd say the same for the Running a particular unit test section. We can simply say Running a particular unit/integration test.

asfbot · 2017-03-20T15:17:16Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2271/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-20T15:18:41Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2274/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-03-20T15:18:46Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2271/
Test PASSed (JDK 8 and Scala 2.12).

ijuma

Thanks, LGTM.

ijuma · 2017-03-21T10:15:23Z

Merged to trunk after tweaking the Readme.md and comment in jenkins.sh slightly. I will give it a week before updating the Jenkins PRs jobs to avoid breaking in-progress PRs.

seperate integration from unit tests

87bfd97

ijuma reviewed Mar 16, 2017

View reviewed changes

dguy added 3 commits March 16, 2017 16:15

change integration test harness traits to abstract classes

1b2cfe1

undo changes

e34e65b

one more i forgot to revert

c55c42e

update readme

6876fb3

resolve merge conflict

42908fc

add jenkins script

519f1be

add common jenkins build arguments

f97a6f1

add license to jenkins.sh

101a434

enothereska reviewed Mar 17, 2017

View reviewed changes

ijuma reviewed Mar 20, 2017

View reviewed changes

address comments

697028c

ijuma approved these changes Mar 21, 2017

View reviewed changes

asfgit closed this in fef7fca Mar 21, 2017

dguy deleted the junit-categories branch March 30, 2017 11:10

C0urante mentioned this pull request Aug 11, 2023

KAFKA-14682: Report Mockito unused stubbings during Jenkins build #14186

Merged

3 tasks


		@Category(Array(classOf[IntegrationTest]))

KAFKA-4594: Annotate integration tests and provide gradle build targets to run subsets of tests #2695

KAFKA-4594: Annotate integration tests and provide gradle build targets to run subsets of tests #2695

Conversation

dguy commented Mar 16, 2017

dguy commented Mar 16, 2017

ijuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asfbot commented Mar 16, 2017

asfbot commented Mar 16, 2017

asfbot commented Mar 16, 2017

dguy commented Mar 16, 2017

asfbot commented Mar 16, 2017

asfbot commented Mar 16, 2017

asfbot commented Mar 16, 2017

ijuma commented Mar 16, 2017

asfbot commented Mar 16, 2017

asfbot commented Mar 16, 2017

asfbot commented Mar 16, 2017

ijuma commented Mar 17, 2017

asfbot commented Mar 17, 2017

asfbot commented Mar 17, 2017

asfbot commented Mar 17, 2017

asfbot commented Mar 17, 2017

asfbot commented Mar 17, 2017

asfbot commented Mar 17, 2017

asfbot commented Mar 17, 2017

asfbot commented Mar 17, 2017

asfbot commented Mar 17, 2017

asfbot commented Mar 17, 2017

Choose a reason for hiding this comment

ijuma Mar 17, 2017 • edited

Choose a reason for hiding this comment

enothereska commented Mar 17, 2017

ijuma commented Mar 17, 2017

enothereska commented Mar 17, 2017

asfbot commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asfbot commented Mar 20, 2017

asfbot commented Mar 20, 2017

asfbot commented Mar 20, 2017

ijuma left a comment

Choose a reason for hiding this comment

ijuma commented Mar 21, 2017

ijuma Mar 17, 2017 •

edited