You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/06/09 11:13:56 UTC
[GitHub] [kafka] dengziming opened a new pull request #10852: MINOR: Replace easymock with mockito in log4j-appender
dengziming opened a new pull request #10852:
URL: https://github.com/apache/kafka/pull/10852
*More detailed description of your change*
As the title
*Summary of testing strategy (including rationale)*
QA
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10852: MINOR: Replace easymock with mockito in log4j-appender
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10852:
URL: https://github.com/apache/kafka/pull/10852#issuecomment-871199867
Unrelated test failures:
> Build / JDK 16 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic()
> Build / JDK 11 and Scala 2.13 / org.apache.kafka.streams.integration.StoreQueryIntegrationTest.shouldQueryOnlyActivePartitionStoresByDefault
> Build / JDK 11 and Scala 2.13 / org.apache.kafka.streams.integration.TaskMetadataIntegrationTest.shouldReportCorrectEndOffsetInformation
> Build / JDK 15 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopics()
> Build / JDK 16 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopics()
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10852: MINOR: Replace easymock with mockito in log4j-appender
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10852:
URL: https://github.com/apache/kafka/pull/10852#discussion_r649169159
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -158,18 +160,18 @@ public void testRealProducerConfigWithSyncSendAndNotIgnoringExceptionsShouldThro
assertThrows(RuntimeException.class, () -> logger.error(getMessage(0)));
}
+ @SuppressWarnings("unchecked")
Review comment:
Why do we need this?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10852: MINOR: Replace easymock with mockito in log4j-appender
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10852:
URL: https://github.com/apache/kafka/pull/10852#discussion_r650510053
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -159,17 +161,18 @@ public void testRealProducerConfigWithSyncSendAndNotIgnoringExceptionsShouldThro
}
private void replaceProducerWithMocked(MockKafkaLog4jAppender mockKafkaLog4jAppender, boolean success) {
- MockProducer<byte[], byte[]> producer = EasyMock.niceMock(MockProducer.class);
- Future<RecordMetadata> futureMock = EasyMock.niceMock(Future.class);
+ @SuppressWarnings("unchecked")
+ MockProducer<byte[], byte[]> producer = mock(MockProducer.class);
+ @SuppressWarnings("unchecked")
+ Future<RecordMetadata> futureMock = mock(Future.class);
try {
if (!success)
- EasyMock.expect(futureMock.get())
- .andThrow(new ExecutionException("simulated timeout", new TimeoutException()));
+ when(futureMock.get())
+ .thenThrow(new ExecutionException("simulated timeout", new TimeoutException()));
} catch (InterruptedException | ExecutionException e) {
Review comment:
This catch is a bit weird to me. Could you create a true `CompletableFuture` carrying the exception `new TimeoutException()` instead of mocking object? For example:
```java
CompletableFuture<RecordMetadata> future = new CompletableFuture<>();
if (success) future.complete(new RecordMetadata(new TopicPartition("tp", 0), 0, 0, 0, 0, 0));
else future.completeExceptionally(new TimeoutException());
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #10852: MINOR: Replace easymock with mockito in log4j-appender
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #10852:
URL: https://github.com/apache/kafka/pull/10852#discussion_r649629620
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -158,18 +160,18 @@ public void testRealProducerConfigWithSyncSendAndNotIgnoringExceptionsShouldThro
assertThrows(RuntimeException.class, () -> logger.error(getMessage(0)));
}
+ @SuppressWarnings("unchecked")
Review comment:
Because Mockito.mock() returns `MockProducer` but we need `MockProducer<byte[], byte[]>`, this is similar to https://github.com/apache/kafka/blob/5936a249bda171851cfa669da4276d23230eab6e/raft/src/test/java/org/apache/kafka/raft/internals/MemoryBatchReaderTest.java#L46
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -158,18 +160,18 @@ public void testRealProducerConfigWithSyncSendAndNotIgnoringExceptionsShouldThro
assertThrows(RuntimeException.class, () -> logger.error(getMessage(0)));
}
+ @SuppressWarnings("unchecked")
Review comment:
Because Mockito.mock() returns `MockProducer` but we need `MockProducer<byte[], byte[]>`, this is similar to https://github.com/apache/kafka/blob/5936a249bda171851cfa669da4276d23230eab6e/raft/src/test/java/org/apache/kafka/raft/internals/MemoryBatchReaderTest.java#L45
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -158,18 +160,18 @@ public void testRealProducerConfigWithSyncSendAndNotIgnoringExceptionsShouldThro
assertThrows(RuntimeException.class, () -> logger.error(getMessage(0)));
}
+ @SuppressWarnings("unchecked")
Review comment:
We have 2 variables here that need to be suppressed, `futureMock` and `producer`, so it seems better to suppress them at the method level, WDYT?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10852: MINOR: Replace easymock with mockito in log4j-appender
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10852:
URL: https://github.com/apache/kafka/pull/10852#discussion_r649169530
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -158,18 +160,18 @@ public void testRealProducerConfigWithSyncSendAndNotIgnoringExceptionsShouldThro
assertThrows(RuntimeException.class, () -> logger.error(getMessage(0)));
}
+ @SuppressWarnings("unchecked")
private void replaceProducerWithMocked(MockKafkaLog4jAppender mockKafkaLog4jAppender, boolean success) {
- MockProducer<byte[], byte[]> producer = EasyMock.niceMock(MockProducer.class);
- Future<RecordMetadata> futureMock = EasyMock.niceMock(Future.class);
+ MockProducer<byte[], byte[]> producer = mock(MockProducer.class);
+ Future<RecordMetadata> futureMock = mock(Future.class);
try {
if (!success)
- EasyMock.expect(futureMock.get())
- .andThrow(new ExecutionException("simulated timeout", new TimeoutException()));
+ Mockito.when(futureMock.get())
Review comment:
Please use static imports to make this more readable.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10852: MINOR: Replace easymock with mockito in log4j-appender
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10852:
URL: https://github.com/apache/kafka/pull/10852#discussion_r650040685
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -158,18 +160,18 @@ public void testRealProducerConfigWithSyncSendAndNotIgnoringExceptionsShouldThro
assertThrows(RuntimeException.class, () -> logger.error(getMessage(0)));
}
+ @SuppressWarnings("unchecked")
Review comment:
Can we have the suppression at the variable level like it is in the example you mentioned?
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -158,18 +160,18 @@ public void testRealProducerConfigWithSyncSendAndNotIgnoringExceptionsShouldThro
assertThrows(RuntimeException.class, () -> logger.error(getMessage(0)));
}
+ @SuppressWarnings("unchecked")
Review comment:
No, it's still best to do it at the most specific place in my opinion.
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -159,17 +161,18 @@ public void testRealProducerConfigWithSyncSendAndNotIgnoringExceptionsShouldThro
}
private void replaceProducerWithMocked(MockKafkaLog4jAppender mockKafkaLog4jAppender, boolean success) {
- MockProducer<byte[], byte[]> producer = EasyMock.niceMock(MockProducer.class);
- Future<RecordMetadata> futureMock = EasyMock.niceMock(Future.class);
+ @SuppressWarnings("unchecked")
+ MockProducer<byte[], byte[]> producer = mock(MockProducer.class);
+ @SuppressWarnings("unchecked")
+ Future<RecordMetadata> futureMock = mock(Future.class);
try {
if (!success)
- EasyMock.expect(futureMock.get())
- .andThrow(new ExecutionException("simulated timeout", new TimeoutException()));
+ when(futureMock.get())
+ .thenThrow(new ExecutionException("simulated timeout", new TimeoutException()));
} catch (InterruptedException | ExecutionException e) {
Review comment:
That's a good point.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma merged pull request #10852: MINOR: Replace easymock with mockito in log4j-appender
Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #10852:
URL: https://github.com/apache/kafka/pull/10852
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #10852: MINOR: Replace easymock with mockito in log4j-appender
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #10852:
URL: https://github.com/apache/kafka/pull/10852#discussion_r650319455
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -158,18 +160,18 @@ public void testRealProducerConfigWithSyncSendAndNotIgnoringExceptionsShouldThro
assertThrows(RuntimeException.class, () -> logger.error(getMessage(0)));
}
+ @SuppressWarnings("unchecked")
Review comment:
Thank you, Done!
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -159,17 +161,18 @@ public void testRealProducerConfigWithSyncSendAndNotIgnoringExceptionsShouldThro
}
private void replaceProducerWithMocked(MockKafkaLog4jAppender mockKafkaLog4jAppender, boolean success) {
- MockProducer<byte[], byte[]> producer = EasyMock.niceMock(MockProducer.class);
- Future<RecordMetadata> futureMock = EasyMock.niceMock(Future.class);
+ @SuppressWarnings("unchecked")
+ MockProducer<byte[], byte[]> producer = mock(MockProducer.class);
+ @SuppressWarnings("unchecked")
+ Future<RecordMetadata> futureMock = mock(Future.class);
try {
if (!success)
- EasyMock.expect(futureMock.get())
- .andThrow(new ExecutionException("simulated timeout", new TimeoutException()));
+ when(futureMock.get())
+ .thenThrow(new ExecutionException("simulated timeout", new TimeoutException()));
} catch (InterruptedException | ExecutionException e) {
Review comment:
Thank you for your reminder @chia7712 , Done!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org