KAFKA-14132: Replace PowerMock and EasyMock with Mockito in streams tests #12821

clolov · 2022-11-04T12:00:48Z

Batch 1 of the tests detailed in https://issues.apache.org/jira/browse/KAFKA-14132 which use PowerMock/EasyMock and need to be moved to Mockito.

streams/src/test/java/org/apache/kafka/streams/processor/internals/RepartitionTopicsTest.java

streams/src/test/java/org/apache/kafka/streams/processor/internals/StateManagerUtilTest.java

clolov · 2022-11-04T14:24:32Z

streams/src/test/java/org/apache/kafka/streams/processor/internals/StateManagerUtilTest.java

-    }
-
-    @Test
-    public void shouldNotWipeStateStoresIfUnableToLockTaskDirectory() throws IOException {


This test appears to be just wrong, unless I am reading the code incorrectly. Because we use expect(stateDirectory.lock(taskId)).andReturn(false); we never reach the code path which would be using Utils.delete. As such, I just removed the test.

As far as I understand the test, the author wanted to fail the test with an AssertionError if Utils.delete is called after the lock could not be acquired. With Mockito, you can verify that Utils.delete is never called.

Sorry, I do not understand. I think this test and the one above can be collapsed in the test I have written. I still verify that deletion did not happen by checking that a few methods were never interacted with.

As in, it appears a bit strange to mock everything on a path to the Utils.delete when this logic short-circuits if we use expect(stateDirectory.lock(taskId)).andReturn(false);.

try { if (stateDirectory.lock(id)) { <---- WE JUMP FROM HERE... try { stateMgr.close(); <---- WE VERIFY THIS IS NOT CALLED } catch (final ProcessorStateException e) { firstException.compareAndSet(null, e); } finally { try { if (wipeStateStore) { log.debug("Wiping state stores for {} task {}", taskType, id); // we can just delete the whole dir of the task, including the state store images and the checkpoint files, // and then we write an empty checkpoint file indicating that the previous close is graceful and we just // need to re-bootstrap the restoration from the beginning Utils.delete(stateMgr.baseDir() <---- WE VERIFY THIS IS NOT CALLED); } } finally { stateDirectory.unlock(id); <---- WE VERIFY THIS IS NOT CALLED } } } } catch (final IOException e) { final ProcessorStateException exception = new ProcessorStateException( String.format("%sFatal error while trying to close the state manager for task %s", logPrefix, id), e ); firstException.compareAndSet(null, exception); } <---- ...ALL THE WAY TO HERE

I see what you mean, but unit tests should also document the behavior of production code and be robust against code changes. In this case, we have indeed to separate cases:

When exactly-once is enabled and the state manager is closed dirty and the lock is not owned then the state manager should not be closed, the directory should not be wiped and the directory should not be unlocked.

When exactly-once is disabled and the lock is not owned then the state manager should not be closed and the directory should not be unlocked.

So, I think what you did is fine, although would not verify that stateManager.baseDir() is never called but that Utils.delete() is never called. However, you need to run that test once with:

StateManagerUtil.closeStateManager( logger, "logPrefix:", false, true, stateManager, stateDirectory, TaskType.ACTIVE);

and once with

StateManagerUtil.closeStateManager( logger, "logPrefix:", true, false, stateManager, stateDirectory, TaskType.ACTIVE);

Okay, I agree, I will create a new commit testing both scenarios.

clolov · 2022-11-04T14:25:18Z

Hey @cadonna 👋! Adding you for visibility as these are Streams-related tests.

cadonna

@clolov Thanks for the PR!

Here my feedback!

cadonna · 2022-11-14T14:29:20Z

streams/src/test/java/org/apache/kafka/streams/processor/internals/StateManagerUtilTest.java

-    }
-
-    @Test
-    public void shouldNotWipeStateStoresIfUnableToLockTaskDirectory() throws IOException {


As far as I understand the test, the author wanted to fail the test with an AssertionError if Utils.delete is called after the lock could not be acquired. With Mockito, you can verify that Utils.delete is never called.

streams/src/test/java/org/apache/kafka/streams/processor/internals/StateManagerUtilTest.java

clolov · 2022-11-15T11:47:54Z

Thank you for the review @cadonna. I will aim to address it today!

divijvaidya · 2022-12-01T14:36:32Z

@clolov please fix the checkstyle failures in this PR!

cadonna

@clolov Thanks for the updates and sorry for the long silence on this PR!

I had one major comment and some nits.

cadonna · 2022-12-12T17:12:43Z

streams/src/test/java/org/apache/kafka/streams/processor/internals/StateManagerUtilTest.java

-    }
-
-    @Test
-    public void shouldNotWipeStateStoresIfUnableToLockTaskDirectory() throws IOException {


I see what you mean, but unit tests should also document the behavior of production code and be robust against code changes. In this case, we have indeed to separate cases:

When exactly-once is enabled and the state manager is closed dirty and the lock is not owned then the state manager should not be closed, the directory should not be wiped and the directory should not be unlocked.

When exactly-once is disabled and the lock is not owned then the state manager should not be closed and the directory should not be unlocked.

So, I think what you did is fine, although would not verify that stateManager.baseDir() is never called but that Utils.delete() is never called. However, you need to run that test once with:

StateManagerUtil.closeStateManager( logger, "logPrefix:", false, true, stateManager, stateDirectory, TaskType.ACTIVE);

and once with

StateManagerUtil.closeStateManager( logger, "logPrefix:", true, false, stateManager, stateDirectory, TaskType.ACTIVE);

streams/src/test/java/org/apache/kafka/streams/processor/internals/StateManagerUtilTest.java

clolov · 2022-12-14T17:54:22Z

Hello @cadonna and thanks for the review! I will aim to address the comments tomorrow.

cadonna

@clolov Thanks for the update and as always for your patience!

LGTM!

I have just minor comment! I will leave it up to you, if you want to change it. Just let me know so that I can merge.

cadonna · 2023-01-19T08:35:18Z

streams/src/test/java/org/apache/kafka/streams/processor/internals/StateManagerUtilTest.java

+        inOrder.verify(stateManager, never()).close();
+        inOrder.verify(stateManager, never()).baseDir();
+        inOrder.verify(stateDirectory, never()).unlock(taskId);


I do not know how much it makes sense to verify the call order of methods that should not be called.

This is a fair point. I used inOrder because the original test wanted to be strict. This being said it wanted to be strict on the actual calls rather than the calls which weren't made. Does

@Test public void shouldNotCloseStateManagerIfUnableToLockTaskDirectory() { final InOrder inOrder = inOrder(stateManager, stateDirectory); when(stateManager.taskId()).thenReturn(taskId); when(stateDirectory.lock(taskId)).thenReturn(false); StateManagerUtil.closeStateManager( logger, "logPrefix:", true, false, stateManager, stateDirectory, TaskType.ACTIVE); inOrder.verify(stateManager).taskId(); <- ENSURE ORDER inOrder.verify(stateDirectory).lock(taskId); <- ENSURE ORDER verify(stateManager, never()).close(); verify(stateManager, never()).baseDir(); verify(stateDirectory, never()).unlock(taskId); verifyNoMoreInteractions(stateManager, stateDirectory); } @Test public void shouldNotWipeStateStoresIfUnableToLockTaskDirectory() { final InOrder inOrder = inOrder(stateManager, stateDirectory); when(stateManager.taskId()).thenReturn(taskId); when(stateDirectory.lock(taskId)).thenReturn(false); StateManagerUtil.closeStateManager( logger, "logPrefix:", false, true, stateManager, stateDirectory, TaskType.ACTIVE); inOrder.verify(stateManager).taskId(); <- ENSURE ORDER inOrder.verify(stateDirectory).lock(taskId); <- ENSURE ORDER verify(stateManager, never()).close(); verify(stateManager, never()).baseDir(); verify(stateDirectory, never()).unlock(taskId); verifyNoMoreInteractions(stateManager, stateDirectory); }

look better to you?

Looks great!

Okay, I pushed the latest rebased version!

cadonna · 2023-01-19T08:36:07Z

streams/src/test/java/org/apache/kafka/streams/processor/internals/StateManagerUtilTest.java

+        inOrder.verify(stateManager, never()).close();
+        inOrder.verify(stateManager, never()).baseDir();
+        inOrder.verify(stateDirectory, never()).unlock(taskId);


See my comment above about call order.

Co-authored-by: Bruno Cadonna <cadonna@apache.org>

cadonna · 2023-01-19T17:43:26Z

Build failures are unrelated:

Build / JDK 11 and Scala 2.13 / org.apache.kafka.clients.consumer.KafkaConsumerTest.testReturnRecordsDuringRebalance()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.integration.BlockingConnectorTest.testBlockInSinkTaskStart
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.integration.ConnectWorkerIntegrationTest.testBrokerCoordinator
Build / JDK 17 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()
Build / JDK 17 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()

clolov · 2023-01-20T10:10:46Z

Thank you very much for the review and merge!

…ests (apache#12821) Batch 1 of the tests detailed in https://issues.apache.org/jira/browse/KAFKA-14132 which use PowerMock/EasyMock and need to be moved to Mockito. Reviewer: Bruno Cadonna <cadonna@apache.org>

clolov changed the title ~~Replace PowerMock and EasyMock with Mockito in streams tests~~ KAFKA-14132: Replace PowerMock and EasyMock with Mockito in streams tests Nov 4, 2022

clolov commented Nov 4, 2022

View reviewed changes

streams/src/test/java/org/apache/kafka/streams/processor/internals/RepartitionTopicsTest.java Show resolved Hide resolved

clolov commented Nov 4, 2022

View reviewed changes

streams/src/test/java/org/apache/kafka/streams/processor/internals/RepartitionTopicsTest.java Show resolved Hide resolved

clolov commented Nov 4, 2022

View reviewed changes

streams/src/test/java/org/apache/kafka/streams/processor/internals/RepartitionTopicsTest.java Show resolved Hide resolved

clolov commented Nov 4, 2022

View reviewed changes

streams/src/test/java/org/apache/kafka/streams/processor/internals/StateManagerUtilTest.java Show resolved Hide resolved

clolov commented Nov 4, 2022

View reviewed changes

cadonna added the streams label Nov 14, 2022

cadonna reviewed Nov 15, 2022

View reviewed changes

clolov force-pushed the KAFKA-14133-PART6 branch from e648e8a to 7fdbf82 Compare November 16, 2022 15:40

clolov force-pushed the KAFKA-14133-PART6 branch from 7fdbf82 to 09ea80f Compare December 1, 2022 15:07

cadonna reviewed Dec 12, 2022

View reviewed changes

clolov force-pushed the KAFKA-14133-PART6 branch from 99cc9ae to 687730d Compare December 16, 2022 08:43

mjsax added the tests Test fixes (including flaky tests) label Dec 28, 2022

cadonna approved these changes Jan 19, 2023

View reviewed changes

clolov and others added 6 commits January 19, 2023 10:49

Replace PowerMock and EasyMock with Mockito in streams tests

5233cb1

Address review comments

6866aa8

Fix checkstyle problems

6b8718e

Apply suggestions from code review

cf82876

Co-authored-by: Bruno Cadonna <cadonna@apache.org>

Return a test

b2dbf44

Address comments about ordering

5183cbe

clolov force-pushed the KAFKA-14133-PART6 branch from 687730d to 5183cbe Compare January 19, 2023 14:02

cadonna merged commit e235e1a into apache:trunk Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-14132: Replace PowerMock and EasyMock with Mockito in streams tests #12821

KAFKA-14132: Replace PowerMock and EasyMock with Mockito in streams tests #12821

clolov commented Nov 4, 2022

clolov Nov 4, 2022

cadonna Nov 14, 2022

clolov Nov 16, 2022

clolov Nov 16, 2022 •

edited

cadonna Dec 12, 2022

clolov Dec 15, 2022

clolov commented Nov 4, 2022

cadonna left a comment

cadonna Nov 14, 2022

clolov commented Nov 15, 2022

divijvaidya commented Dec 1, 2022

cadonna left a comment

cadonna Dec 12, 2022

clolov commented Dec 14, 2022

cadonna left a comment

cadonna Jan 19, 2023

clolov Jan 19, 2023

cadonna Jan 19, 2023

clolov Jan 19, 2023

cadonna Jan 19, 2023

cadonna commented Jan 19, 2023

clolov commented Jan 20, 2023

KAFKA-14132: Replace PowerMock and EasyMock with Mockito in streams tests #12821

KAFKA-14132: Replace PowerMock and EasyMock with Mockito in streams tests #12821

Conversation

clolov commented Nov 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clolov Nov 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clolov commented Nov 4, 2022

cadonna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clolov commented Nov 15, 2022

divijvaidya commented Dec 1, 2022

cadonna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clolov commented Dec 14, 2022

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

cadonna commented Jan 19, 2023

clolov commented Jan 20, 2023

clolov Nov 16, 2022 •

edited