KAFKA-16000 Migrated MembershipManagerImplTest away from ConsumerTestBuilder by brenden20 · Pull Request #16312 · apache/kafka

brenden20 · 2024-06-12T20:49:07Z

I completely migrated MembershipManagerImplTest away from ConsumerTestBuilder and removed all spy objects. All tests passing, none were removed.

Updated mockStableMember()

Added a few stubs to make this method work

Removed verifyReconciliationNotTriggered(). My thinking here is that this method requires membershipManager to be a mock or spy, but we need to test with a real object

Similar to last commit, removed all instances of clearInvocations(membershipManager) and edited verifyReconciliationTriggered() to remove some calls to a non mock

Again removing methods/test where it is expecting a mock of MembershipManager

Added a bunch of stubbing

Created method createCoordinatorRequestManager(). It changes the commitRequestManager variable from a mock to an actual object, which needs to be done for many of the tests

Updated tests by commenting out some lines in methods and editing private methods

Updated tests to perform unit testing instead of integration testing

A few of the tests did not work without instantiated backgroundEventQueue and handler

…gnment() Updated testMemberJoiningCallsRebalanceListenerWhenReceivingEmptyAssignment() to work properly, it is an integration test

Updated testOnPartitionsLost() to work properly, again integration testing

Updated testReconciliationSkippedWhenSameAssignmentReceived()

Updated createCommitRequestManager() to enable autoCommit on the commitRequestManager. Also updated testAddedPartitionsNotEnabledAfterFailedOnPartitionsAssignedCallback() to work

Added a line to create commitRequestManager

brenden20 · 2024-06-21T21:16:06Z

Thank you for the feedback @lianetm, I just made all the suggested improvements!

philipnee · 2024-06-24T14:51:03Z


        assertEquals(MemberState.STALE, membershipManager.state());
-        verify(backgroundEventHandler).add(any(ConsumerRebalanceListenerCallbackNeededEvent.class));
+        assertFalse(backgroundEventQueue.isEmpty());


the original implementation was better, any reason for changing that? verify(backgroundEventHandler).add(any(ConsumerRebalanceListenerCallbackNeededEvent.class));

it was better because backgroundEventQueue is in the implementation. we just need to make sure the event was added to the handler.

Since backgroundEventHandler is an actual instance of an object, we cannot verify() on it. So in order to still test what it was trying to test, the two assert statements must be there instead

does changing backgroundEventHandler to a mock break a lot of tests?

Yeah, breaks 16/81, all test failures are on asserting an empty queue or number of events in the queue

lianetm

Thanks for the great improvement and all the updates @brenden20 ! Let's make sure @philipnee 's concerns are clarified but this LGTM. Thanks!

mjsax · 2024-06-24T18:15:01Z

+        commitRequestManager = mock(CommitRequestManager.class);
+        backgroundEventQueue = new LinkedBlockingQueue<>();
+        backgroundEventHandler = new BackgroundEventHandler(backgroundEventQueue);
+        logContext = new LogContext();


For my own education: why move this here and not keep logContext as final?

It was mostly to keep consistency with variables being private/private and final. Although I did not consider that I could make it private static final and it would still work without issue and keep the variables consistent. I am going to make that change now.

mjsax · 2024-06-24T18:16:15Z

        // Member should update the subscription and send ack when the delayed reconciliation
        // completes.
-        verify(subscriptionState).assignFromSubscribed(Collections.emptySet());
+        verify(subscriptionState).assignFromSubscribedAwaitingCallback(Collections.emptySet(), Collections.emptySet());


Why do we verify something else here? This PR only updates test code, so where does this change in behavior come from? Seems I am missing something.

assignFromSubscribed() is called from assignFromSubscribedAwaitingCallback(). Since subscriptionState was changed from a spy to a mock, we could not test for this behavior. So instead I changed the verify to be the calling function assignFromSubscribedAwaitingCallback() and the correct, expected parameters. If assignFromSubscribedAwaitingCallback() is getting the correct parameters, then it should implicitly verify that assignFromSubscribed() is being called with the correct parameter.

brenden20 · 2024-06-24T18:37:00Z

@mjsax thank you for the feedback! I made a small change with logContext and replied to your other comment. If there is anything else let me know!

brenden20 · 2024-06-28T18:48:28Z

All test failures are unrelated to the changes made in this PR

mjsax · 2024-06-28T19:13:42Z

Thanks for the PR @brenden20. Merged to trunk.

…tBuilder (apache#16312) Finishing migration of MembershipManagerImplTest away from ConsumerTestBuilder and removed all spy objects. Reviewers: Lianet Magrans <lianetmr@gmail.com>, Philip Nee <pnee@confluent.io>, Matthias J. Sax <matthias@confluent.io>

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 <chia7712@gmail.com>

brenden20 added 30 commits June 4, 2024 16:02

Changes to setup()

8ed218d

Removed spy objects

1433236

Updated mockStableMember()

2261825

Updated mockStableMember()

Update mockJoinAndReceiveAssignment()

932075b

Tiny update

18f5016

Made small update to createMemberInStableState()

d0c16fa

Updated mockPrepareLeavingStuckOnUserCallback()

3cada30

Added a few stubs to make this method work

Update testNewAssignmentIgnoredWhenStateIsPrepareLeaving()

ae9b2a5

Update testStaleMemberWaitsForCallbackToRejoinWhenTimerReset()

708faa4

Removed verifyReconciliationNotTriggered()

8732788

Removed verifyReconciliationNotTriggered(). My thinking here is that this method requires membershipManager to be a mock or spy, but we need to test with a real object

Method removal

4583d62

Similar to last commit, removed all instances of clearInvocations(membershipManager) and edited verifyReconciliationTriggered() to remove some calls to a non mock

Method/test removal

e6da81b

Again removing methods/test where it is expecting a mock of MembershipManager

Various changes

ee1e25e

Added a bunch of stubbing

Add TODO comments

1f92962

Created createCoordinatorRequestManager()

1eb39cd

Created method createCoordinatorRequestManager(). It changes the commitRequestManager variable from a mock to an actual object, which needs to be done for many of the tests

Large test update

8b95da6

Updated tests by commenting out some lines in methods and editing private methods

Minor updates for mocks

ccc3eb4

Merge branch 'trunk' into 16000

c6e603f

Small update, moved a line and instantiated a map

c64aa21

Updated a test form integration to unit testing

7eb5bd9

Removed TODOs and updated 2 tests

dd23ae3

Updated tests to perform unit testing instead of integration testing

Instantiated variables when needed

0045782

A few of the tests did not work without instantiated backgroundEventQueue and handler

Updated testMemberJoiningCallsRebalanceListenerWhenReceivingEmptyAssi…

16baee9

…gnment() Updated testMemberJoiningCallsRebalanceListenerWhenReceivingEmptyAssignment() to work properly, it is an integration test

Updated testOnPartitionsLost()

f88c630

Updated testOnPartitionsLost() to work properly, again integration testing

Updated verify statement to make into unit testing

1a105e5

More backgroundEventQueue/Handler

b39727e

Update MembershipManagerImplTest.java

c2f16a7

Add TODOs and updated test

43826b2

Updated testReconciliationSkippedWhenSameAssignmentReceived()

Update createCommitRequestManager()

4abf2e4

Updated createCommitRequestManager() to enable autoCommit on the commitRequestManager. Also updated testAddedPartitionsNotEnabledAfterFailedOnPartitionsAssignedCallback() to work

Fixed testRebalanceMetricsOnSuccessfulRebalance()

619b41d

Added a line to create commitRequestManager

brenden20 force-pushed the 16000 branch from 8d69507 to febf26d Compare June 21, 2024 21:15

lianetm reviewed Jun 21, 2024

View reviewed changes

Comment thread ...nts/src/test/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImplTest.java Outdated

lianetm reviewed Jun 21, 2024

View reviewed changes

Comment thread ...nts/src/test/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImplTest.java Outdated

philipnee reviewed Jun 24, 2024

View reviewed changes

Comment thread ...nts/src/test/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImplTest.java Outdated

Merge branch 'apache:trunk' into 16000

e53ad10

philipnee reviewed Jun 24, 2024

View reviewed changes

brenden20 added 2 commits June 24, 2024 10:27

Remove eq()'s

ef15fd7

Small fix

fe7e7ca

lianetm approved these changes Jun 24, 2024

View reviewed changes

mjsax reviewed Jun 24, 2024

View reviewed changes

brenden20 and others added 2 commits June 24, 2024 13:35

Changed logContext

05775f2

Merge branch 'apache:trunk' into 16000

de838b4

philipnee reviewed Jun 25, 2024

View reviewed changes

Comment thread ...nts/src/test/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImplTest.java Outdated

philipnee reviewed Jun 25, 2024

View reviewed changes

Comment thread ...nts/src/test/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImplTest.java

brenden20 and others added 7 commits June 25, 2024 09:49

Merge branch 'apache:trunk' into 16000

fc6e5d7

Merge branch 'apache:trunk' into 16000

70cd519

Merge branch 'apache:trunk' into 16000

9ea8178

Update MembershipManagerImplTest.java

39a6b06

Merge branch '16000' of https://github.com/brenden20/kafka into 16000

21f7e09

Merge branch 'apache:trunk' into 16000

f78788e

Merge branch 'apache:trunk' into 16000

baca710

mjsax merged commit 836f52b into apache:trunk Jun 28, 2024

brenden20 deleted the 16000 branch June 28, 2024 19:50

brenden20 mentioned this pull request Jul 25, 2024

MINOR: Remove ConsumerTestBuilder.java #16691

Merged

3 tasks

Conversation

brenden20 commented Jun 12, 2024

Uh oh!

brenden20 commented Jun 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lianetm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brenden20 commented Jun 24, 2024

Uh oh!

Uh oh!

Uh oh!

brenden20 commented Jun 28, 2024

Uh oh!

mjsax commented Jun 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants