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 2020/12/24 07:14:02 UTC
[GitHub] [kafka] g1geordie opened a new pull request #9785: KAFKA-10887 Migrate log4j-appender module to JUnit 5
g1geordie opened a new pull request #9785:
URL: https://github.com/apache/kafka/pull/9785
Migrate log4j-appender module to JUnit 5
----------------------------------------------------------------
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] g1geordie commented on a change in pull request #9785: KAFKA-10887 Migrate log4j-appender module to JUnit 5
Posted by GitBox <gi...@apache.org>.
g1geordie commented on a change in pull request #9785:
URL: https://github.com/apache/kafka/pull/9785#discussion_r554590943
##########
File path: build.gradle
##########
@@ -1782,8 +1782,8 @@ project(':log4j-appender') {
compile libs.slf4jlog4j
testCompile project(':clients').sourceSets.test.output
- testCompile libs.junitJupiterApi
- testCompile libs.junitVintageEngine
+ testCompile libs.junitJupiter
+ testCompile libs.hamcrest
Review comment:
Thank you .
You are so strong that you can track a lot of issues simultaneously !
----------------------------------------------------------------
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 #9785: KAFKA-10887 Migrate log4j-appender module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9785:
URL: https://github.com/apache/kafka/pull/9785#discussion_r548481521
##########
File path: build.gradle
##########
@@ -1782,8 +1782,8 @@ project(':log4j-appender') {
compile libs.slf4jlog4j
testCompile project(':clients').sourceSets.test.output
- testCompile libs.junitJupiterApi
- testCompile libs.junitVintageEngine
+ testCompile libs.junitJupiter
+ testCompile libs.hamcrest
Review comment:
@ijuma Should we remove hamcrest? Removing it causes a bigger patch. The benefit of removing it is that our test assert get more consistent.
----------------------------------------------------------------
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 #9785: KAFKA-10887 Migrate log4j-appender module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9785:
URL: https://github.com/apache/kafka/pull/9785#discussion_r554520668
##########
File path: build.gradle
##########
@@ -1782,8 +1782,8 @@ project(':log4j-appender') {
compile libs.slf4jlog4j
testCompile project(':clients').sourceSets.test.output
- testCompile libs.junitJupiterApi
- testCompile libs.junitVintageEngine
+ testCompile libs.junitJupiter
+ testCompile libs.hamcrest
Review comment:
see https://github.com/apache/kafka/pull/9520#issuecomment-757426244 so keeping ```libs.hamcrest``` is fine.
----------------------------------------------------------------
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 merged pull request #9785: KAFKA-10887 Migrate log4j-appender module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #9785:
URL: https://github.com/apache/kafka/pull/9785
----------------------------------------------------------------
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] g1geordie commented on pull request #9785: KAFKA-10887 Migrate log4j-appender module to JUnit 5
Posted by GitBox <gi...@apache.org>.
g1geordie commented on pull request #9785:
URL: https://github.com/apache/kafka/pull/9785#issuecomment-750829564
@chia7712 please help me take a look
----------------------------------------------------------------
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] g1geordie commented on a change in pull request #9785: KAFKA-10887 Migrate log4j-appender module to JUnit 5
Posted by GitBox <gi...@apache.org>.
g1geordie commented on a change in pull request #9785:
URL: https://github.com/apache/kafka/pull/9785#discussion_r554593025
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -130,55 +116,51 @@ public void testLog4jAppends() {
for (int i = 1; i <= 5; ++i) {
logger.error(getMessage(i));
}
-
- Assert.assertEquals(
- 5, (getMockKafkaLog4jAppender()).getHistory().size());
+ assertEquals(getMockKafkaLog4jAppender().getHistory().size(), 5);
}
- @Test(expected = RuntimeException.class)
- public void testLog4jAppendsWithSyncSendAndSimulateProducerFailShouldThrowException() {
+ @Test
+ public void testSyncSendAndSimulateProducerFailShouldThrowException() {
Properties props = getLog4jConfig(true);
props.put("log4j.appender.KAFKA.IgnoreExceptions", "false");
PropertyConfigurator.configure(props);
MockKafkaLog4jAppender mockKafkaLog4jAppender = getMockKafkaLog4jAppender();
replaceProducerWithMocked(mockKafkaLog4jAppender, false);
- logger.error(getMessage(0));
+ assertThrows(RuntimeException.class, () -> logger.error(getMessage(0)));
}
@Test
- public void testLog4jAppendsWithSyncSendWithoutIgnoringExceptionsShouldNotThrowException() {
+ public void testSyncSendWithoutIgnoringExceptionsShouldNotThrowException() {
Properties props = getLog4jConfig(true);
props.put("log4j.appender.KAFKA.IgnoreExceptions", "false");
PropertyConfigurator.configure(props);
MockKafkaLog4jAppender mockKafkaLog4jAppender = getMockKafkaLog4jAppender();
replaceProducerWithMocked(mockKafkaLog4jAppender, true);
- logger.error(getMessage(0));
+ assertDoesNotThrow(() -> logger.error(getMessage(0)));
Review comment:
As you think . I just point out where we do the check .
I will file a MINOR to change that . Thank you
----------------------------------------------------------------
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 #9785: KAFKA-10887 Migrate log4j-appender module to JUnit 5
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9785:
URL: https://github.com/apache/kafka/pull/9785#discussion_r554589317
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -130,55 +116,51 @@ public void testLog4jAppends() {
for (int i = 1; i <= 5; ++i) {
logger.error(getMessage(i));
}
-
- Assert.assertEquals(
- 5, (getMockKafkaLog4jAppender()).getHistory().size());
+ assertEquals(getMockKafkaLog4jAppender().getHistory().size(), 5);
}
- @Test(expected = RuntimeException.class)
- public void testLog4jAppendsWithSyncSendAndSimulateProducerFailShouldThrowException() {
+ @Test
+ public void testSyncSendAndSimulateProducerFailShouldThrowException() {
Properties props = getLog4jConfig(true);
props.put("log4j.appender.KAFKA.IgnoreExceptions", "false");
PropertyConfigurator.configure(props);
MockKafkaLog4jAppender mockKafkaLog4jAppender = getMockKafkaLog4jAppender();
replaceProducerWithMocked(mockKafkaLog4jAppender, false);
- logger.error(getMessage(0));
+ assertThrows(RuntimeException.class, () -> logger.error(getMessage(0)));
}
@Test
- public void testLog4jAppendsWithSyncSendWithoutIgnoringExceptionsShouldNotThrowException() {
+ public void testSyncSendWithoutIgnoringExceptionsShouldNotThrowException() {
Properties props = getLog4jConfig(true);
props.put("log4j.appender.KAFKA.IgnoreExceptions", "false");
PropertyConfigurator.configure(props);
MockKafkaLog4jAppender mockKafkaLog4jAppender = getMockKafkaLog4jAppender();
replaceProducerWithMocked(mockKafkaLog4jAppender, true);
- logger.error(getMessage(0));
+ assertDoesNotThrow(() -> logger.error(getMessage(0)));
Review comment:
I considered it is a kind of highlight. However, it is unnecessary in this test case as it is the final line. @g1geordie WDYT? Should we file a PR to remove this unnecessary noise?
----------------------------------------------------------------
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 #9785: KAFKA-10887 Migrate log4j-appender module to JUnit 5
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9785:
URL: https://github.com/apache/kafka/pull/9785#discussion_r554586805
##########
File path: log4j-appender/src/test/java/org/apache/kafka/log4jappender/KafkaLog4jAppenderTest.java
##########
@@ -130,55 +116,51 @@ public void testLog4jAppends() {
for (int i = 1; i <= 5; ++i) {
logger.error(getMessage(i));
}
-
- Assert.assertEquals(
- 5, (getMockKafkaLog4jAppender()).getHistory().size());
+ assertEquals(getMockKafkaLog4jAppender().getHistory().size(), 5);
}
- @Test(expected = RuntimeException.class)
- public void testLog4jAppendsWithSyncSendAndSimulateProducerFailShouldThrowException() {
+ @Test
+ public void testSyncSendAndSimulateProducerFailShouldThrowException() {
Properties props = getLog4jConfig(true);
props.put("log4j.appender.KAFKA.IgnoreExceptions", "false");
PropertyConfigurator.configure(props);
MockKafkaLog4jAppender mockKafkaLog4jAppender = getMockKafkaLog4jAppender();
replaceProducerWithMocked(mockKafkaLog4jAppender, false);
- logger.error(getMessage(0));
+ assertThrows(RuntimeException.class, () -> logger.error(getMessage(0)));
}
@Test
- public void testLog4jAppendsWithSyncSendWithoutIgnoringExceptionsShouldNotThrowException() {
+ public void testSyncSendWithoutIgnoringExceptionsShouldNotThrowException() {
Properties props = getLog4jConfig(true);
props.put("log4j.appender.KAFKA.IgnoreExceptions", "false");
PropertyConfigurator.configure(props);
MockKafkaLog4jAppender mockKafkaLog4jAppender = getMockKafkaLog4jAppender();
replaceProducerWithMocked(mockKafkaLog4jAppender, true);
- logger.error(getMessage(0));
+ assertDoesNotThrow(() -> logger.error(getMessage(0)));
Review comment:
What is the reason for having `assertDoesNotThrow` here and below? The test will fail if an exception is thrown, so seems like unnecessary noise.
----------------------------------------------------------------
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