KAFKA-15913: Migrate async consumer tests to mocks #14930

lucasbru · 2023-12-05T16:01:05Z

Use mocks to test the AsyncKafkaConsumer

Eliminate the use of ConsumerTestBuilder
Mock all resources that were previously retrieved via leaking
the background thread with mockito spys
Always use the default constructor of AsyncKafkaConsumer as
much as possible, inject mocks via factories.
Timeouts are mocked directly by timeout exceptions instead
of waiting for futures to time out.

I did not port the autocommit mocking code, because it was mostly
testing the integration of foreground and background thread (or
making the spy's work which broke during the autocommit on close)
and is currently being reimplemented anyway.

New test runs 10x faster.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…ig tests

clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java

cadonna

Thanks for the PR, @lucasbru !

Here my feedback.

clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java

cadonna · 2023-12-11T15:36:18Z

clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java

-                (short) 1,
-                Errors.NONE).responseBody());
-        }
+    private void mockCommitApplicationEventException(Exception ex) {


I think calling this method (and its siblings) something like completeCommitApplicationEventExceptionally() (completeCommitApplicationEventSuccessfully() for the good case) would make the code better readable.

cadonna · 2023-12-11T15:45:15Z

clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java

        assertDoesNotThrow(() -> consumer.commitAsync());
-        future.completeExceptionally(Errors.FENCED_INSTANCE_ID.exception());
    }


Isn't here something missing? The next call to consumer.commitAsync() should throw, right? Shouldn't we verify that?

Good point. There were a couple of poor tests and I didn't feel like fixing it all, but this one is a good one to fix. Done

cadonna · 2023-12-11T15:52:44Z

clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java

-            verify(applicationEventHandler).add(ArgumentMatchers.isA(FetchCommittedOffsetsApplicationEvent.class));
-        }
+        assertDoesNotThrow(() -> consumer.committed(topicPartitionOffsets.keySet(), Duration.ofMillis(1000)));
+        verify(applicationEventHandler).addAndGet(ArgumentMatchers.isA(FetchCommittedOffsetsApplicationEvent.class), any());


I am missing the verification of the return value of committed().

Also didn't have it before but I fixed it.

cadonna · 2023-12-11T16:29:18Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java

@@ -482,6 +497,47 @@ private void process(final GroupMetadataUpdateEvent event) {
                requestManagersSupplier);
    }

+    // auxiliary interface for testing
+    interface ApplicationEventHandlerSupplier {


It is nor clear to me why we need a supplier here. Could you elaborate?

I want to inject a custom implementation of the applicationEventHandler. It needs to work for both prod and testing so that we can actually test the code we are running. The instantiation for prod depends on arguments that are created inside the constructor of AsyncKafkaConsumer. If I do not define this supplier, I cannot inject the constructor of the "real" ApplicationEventHandler for prod.

clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java

cadonna · 2023-12-11T16:56:27Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java

+    // Visible for testing
+    SubscriptionState subscriptions() {
+        return subscriptions;
+    }


Is there a way to avoid this?

I can add callback to the constructor that registers the SubscriptionState with the calling context (the test)

I can add another factory to inject our own instance of SubscriptionState

cadonna

LGTM!

We can work on the open test-specific questions in a follow-up PR. Now it is important to get a first version of the spy-free unit tests merged.

Thanks, @lucasbru !

make fenced exception test more unflaky. assert( ... instanceof ... ) -> assertIsInstanceOf(...)

Use mocks to test the AsyncKafkaConsumer Eliminate the use of ConsumerTestBuilder Mock all resources that were previously retrieved via leaking the background thread with mockito spys Always use the default constructor of AsyncKafkaConsumer as much as possible, inject mocks via factories. Timeouts are mocked directly by timeout exceptions instead of waiting for futures to time out. I did not port the autocommit mocking code, because it was mostly testing the integration of foreground and background thread (or making the spy's work which broke during the autocommit on close) and is currently being reimplemented anyway. New test runs 10x faster. Reviewers: Bruno Cadonna <[email protected]>

The purpose of this PR is to remove ConsumerTestBuilder.java since it is no longer needed. The following PRs have eliminated the use of ConsumerTestBuilder: #14930 #16140 #16200 #16312 Reviewers: Chia-Ping Tsai <[email protected]>

lucasbru force-pushed the ctr_migrate_test branch from 1531135 to cb624ef Compare December 8, 2023 15:48

lucasbru and others added 16 commits December 11, 2023 12:44

migrate epoch update to mocks

8159e51

migrate fenced exception

5b95d03

migrate committed tests

7dcd8ae

migrate subscribed tests

966c18f

migrate offset tests

407a85b

migrate wakeup tests

b40e5c9

migrate assing tests

4ff9b57

Add test testCommitAsync_NullCallback as testCommitAsyncWithNullCallback

283cf51

use most of the normal construction code for testing and migrate conf…

53b0fdf

…ig tests

fix rebase errors, port remote assignor and group metadata tests

5630c42

port long poll test

ab35f4e

migrate commit callback tests

4d3fea9

migrate wakeup committed

b00135e

reorder

90a1f5d

updates

60037dd

checkstyle

6377a9d

lucasbru force-pushed the ctr_migrate_test branch from 3d59e41 to 6377a9d Compare December 11, 2023 11:45

lucasbru changed the title ~~Draft: Migrate async consumer tests to mocks~~ KAFKA-15913: Migrate async consumer tests to mocks Dec 11, 2023

lucasbru self-assigned this Dec 11, 2023

lucasbru requested a review from cadonna December 11, 2023 11:49

lucasbru marked this pull request as ready for review December 11, 2023 11:57

cadonna added ctr Consumer Threading Refactor (KIP-848) tests Test fixes (including flaky tests) labels Dec 11, 2023

lucasbru commented Dec 11, 2023

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java Outdated Show resolved Hide resolved

cadonna reviewed Dec 11, 2023

View reviewed changes

cadonna mentioned this pull request Dec 11, 2023

KAFKA-15991: Fix flaky test testGroupIdNotNullAndValid #14944

Closed

3 tasks

lucasbru added 2 commits December 11, 2023 20:23

PR comments

66a0c15

Merge remote-tracking branch 'upstream/trunk' into ctr_migrate_test

c6a5ee0

lucasbru mentioned this pull request Dec 12, 2023

KAFKA-15696: Refactor closing consumer #14937

Merged

cadonna approved these changes Dec 12, 2023

View reviewed changes

lucasbru added 6 commits December 12, 2023 10:32

remove unused code.

e6430ca

make fenced exception test more unflaky. assert( ... instanceof ... ) -> assertIsInstanceOf(...)

Merge remote-tracking branch 'upstream/trunk' into ctr_migrate_test

9648ca3

optimize imports

59e385b

checkstyle

dde99b3

Merge remote-tracking branch 'upstream/trunk' into ctr_migrate_test

dc4f96d

merge fixes

5e734fe

lucasbru merged commit 0cb7d74 into apache:trunk Dec 18, 2023
1 check failed

philipnee added the consumer label Dec 21, 2023

brenden20 mentioned this pull request Jul 25, 2024

MINOR: Remove ConsumerTestBuilder.java #16691

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-15913: Migrate async consumer tests to mocks #14930

KAFKA-15913: Migrate async consumer tests to mocks #14930

lucasbru commented Dec 5, 2023 •

edited

Loading

cadonna left a comment

cadonna Dec 11, 2023

cadonna Dec 11, 2023

lucasbru Dec 11, 2023

cadonna Dec 11, 2023

lucasbru Dec 11, 2023

cadonna Dec 11, 2023

lucasbru Dec 11, 2023

cadonna Dec 11, 2023

lucasbru Dec 11, 2023

cadonna left a comment

KAFKA-15913: Migrate async consumer tests to mocks #14930

KAFKA-15913: Migrate async consumer tests to mocks #14930

Conversation

lucasbru commented Dec 5, 2023 • edited Loading

Committer Checklist (excluded from commit message)

cadonna 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

cadonna left a comment

Choose a reason for hiding this comment

lucasbru commented Dec 5, 2023 •

edited

Loading