Re: [PR] KAFKA-14133: Migrate storeMetadata mock in StoreChangelogReaderTest to Mockito [kafka]

2024-01-04 Thread via GitHub


clolov commented on PR #15116:
URL: https://github.com/apache/kafka/pull/15116#issuecomment-1877054383

   Okay @lucasbru, I will look into it


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] KAFKA-14133: Migrate storeMetadata mock in StoreChangelogReaderTest to Mockito [kafka]

2024-01-04 Thread via GitHub


lucasbru commented on PR #15116:
URL: https://github.com/apache/kafka/pull/15116#issuecomment-1877052454

   @clolov I do believe that the flaky test @divijvaidya mentioned started with 
another mockito migration PR on 4th of december, #13932. Maybe you could give 
it a look even if it's not related to this PR. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] KAFKA-14133: Migrate storeMetadata mock in StoreChangelogReaderTest to Mockito [kafka]

2024-01-04 Thread via GitHub


divijvaidya merged PR #15116:
URL: https://github.com/apache/kafka/pull/15116


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] KAFKA-14133: Migrate storeMetadata mock in StoreChangelogReaderTest to Mockito [kafka]

2024-01-04 Thread via GitHub


divijvaidya commented on PR #15116:
URL: https://github.com/apache/kafka/pull/15116#issuecomment-1877045252

   My bad! the test failure I notes above is known to be flaky as per 
https://ge.apache.org/scans/tests?search.relativeStartTime=P28D&search.rootProjectNames=kafka&search.tags=trunk&search.timeZoneId=Europe%2FBerlin&tests.container=org.apache.kafka.streams.processor.internals.StreamThreadTest&tests.test=shouldNotEnforceRebalanceWhenCurrentlyRebalancing%5B0%5D
 and is not related to this PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] KAFKA-14133: Migrate storeMetadata mock in StoreChangelogReaderTest to Mockito [kafka]

2024-01-04 Thread via GitHub


divijvaidya commented on PR #15116:
URL: https://github.com/apache/kafka/pull/15116#issuecomment-1876978251

   @clolov we have test failures related to this PR. Can you please fix them: 
https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-15116/1/testReport/org.apache.kafka.streams.processor.internals/StreamThreadTest/Build___JDK_11_and_Scala_2_13___shouldNotEnforceRebalanceWhenCurrentlyRebalancing_0_/
 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] KAFKA-14133: Migrate storeMetadata mock in StoreChangelogReaderTest to Mockito [kafka]

2024-01-03 Thread via GitHub


clolov commented on code in PR #15116:
URL: https://github.com/apache/kafka/pull/15116#discussion_r1440615357


##
streams/src/test/java/org/apache/kafka/streams/processor/internals/StoreChangelogReaderTest.java:
##
@@ -453,17 +456,17 @@ private void shouldPollWithRightTimeout(final boolean 
stateUpdaterEnabled) {
 @Test
 public void shouldPollWithRightTimeoutWithStateUpdaterDefault() {
 setupStateManagerMock();
+setupStoreMetadata();
 final Properties properties = new Properties();
 shouldPollWithRightTimeout(properties);
 }
 
 private void shouldPollWithRightTimeout(final Properties properties) {
 final TaskId taskId = new TaskId(0, 0);
 
-
EasyMock.expect(storeMetadata.offset()).andReturn(null).andReturn(9L).anyTimes();
-EasyMock.expect(storeMetadata.endOffset()).andReturn(10L).anyTimes();

Review Comment:
   Ditto



##
streams/src/test/java/org/apache/kafka/streams/processor/internals/StoreChangelogReaderTest.java:
##
@@ -257,15 +255,16 @@ public void 
shouldSupportUnregisterChangelogBeforeInitialization() {
 @Test
 public void shouldSupportUnregisterChangelogBeforeCompletion() {
 setupStateManagerMock();
+setupStoreMetadata();
 final Map mockTasks = mock(Map.class);
 
EasyMock.expect(mockTasks.get(null)).andReturn(mock(Task.class)).anyTimes();
 
EasyMock.expect(mockTasks.containsKey(null)).andReturn(true).anyTimes();
-EasyMock.expect(storeMetadata.offset()).andReturn(9L).anyTimes();
-EasyMock.expect(storeMetadata.endOffset()).andReturn(10L).anyTimes();
+when(storeMetadata.offset()).thenReturn(9L);
 if (type == STANDBY) {
+when(storeMetadata.endOffset()).thenReturn(10L);

Review Comment:
   Only used on the STANDBY path



##
streams/src/test/java/org/apache/kafka/streams/processor/internals/StoreChangelogReaderTest.java:
##
@@ -821,11 +831,11 @@ public Map 
committed(final Set mockTasks = mock(Map.class);
 
EasyMock.expect(mockTasks.get(null)).andReturn(mock(Task.class)).anyTimes();
-EasyMock.expect(storeMetadata.offset()).andReturn(9L).anyTimes();

Review Comment:
   Reported as unused by Mockito



##
streams/src/test/java/org/apache/kafka/streams/processor/internals/StoreChangelogReaderTest.java:
##
@@ -363,12 +363,12 @@ public void 
shouldSupportUnregisterChangelogAfterCompletion() {
 @Test
 public void shouldInitializeChangelogAndCheckForCompletion() {
 setupStateManagerMock();
+setupStoreMetadata();
 final Map mockTasks = mock(Map.class);
 
EasyMock.expect(mockTasks.get(null)).andReturn(mock(Task.class)).anyTimes();
 
EasyMock.expect(mockTasks.containsKey(null)).andReturn(true).anyTimes();
-EasyMock.expect(storeMetadata.offset()).andReturn(9L).anyTimes();
-EasyMock.expect(storeMetadata.endOffset()).andReturn(10L).anyTimes();

Review Comment:
   Unused according to Mockito



##
streams/src/test/java/org/apache/kafka/streams/processor/internals/StoreChangelogReaderTest.java:
##
@@ -862,10 +873,9 @@ public void 
shouldRequestCommittedOffsetsAndHandleTimeoutException() {
 EasyMock.expectLastCall();
 
 when(stateManager.changelogAsSource(tp)).thenReturn(true);
-EasyMock.expect(storeMetadata.offset()).andReturn(5L).anyTimes();
-EasyMock.expect(storeMetadata.endOffset()).andReturn(10L).anyTimes();

Review Comment:
   Ditto



##
streams/src/test/java/org/apache/kafka/streams/processor/internals/StoreChangelogReaderTest.java:
##
@@ -924,13 +934,13 @@ public synchronized ListConsumerGroupOffsetsResult 
listConsumerGroupOffsets(fina
 @Test
 public void shouldThrowIfCommittedOffsetsFail() {
 setupStateManagerMock();
+when(storeMetadata.changelogPartition()).thenReturn(tp);
 
 final TaskId taskId = new TaskId(0, 0);
 
 when(stateManager.taskId()).thenReturn(taskId);
 when(stateManager.changelogAsSource(tp)).thenReturn(true);
-EasyMock.expect(storeMetadata.offset()).andReturn(10L).anyTimes();

Review Comment:
   Ditto



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org