[GitHub] [kafka] jasonyanwenl commented on a change in pull request #11241: KAFKA-13032: add NPE checker for KeyValueMapper

[GitBox <gi...@apache.org>]

jasonyanwenl commented on a change in pull request #11241:
URL: https://github.com/apache/kafka/pull/11241#discussion_r695002209



##########
File path: streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamFlatMapTest.java
##########
@@ -86,4 +88,10 @@ public void testFlatMap() {
             assertEquals(expected[i], supplier.theCapturedProcessor().processed().get(i));
         }
     }
+
+    @Test
+    public void testKeyValueMapperResultNotNull() {
+        final KStreamFlatMap<String, Integer, String, Integer> supplier = new KStreamFlatMap<>((key, value) -> null);
+        assertThrows(NullPointerException.class, () -> supplier.get().process(new Record<>("K", 0, 0L)));

Review comment:
       Thanks for your review! I found we mixed the using of two packages: `org.junit.Assert` and `org.junit.jupiter.api.Assertions`.  The method `asssertThrows` in those two packages have different api:
   
   For `org.junit.Assert.assertThrows`, the [api](https://junit.org/junit4/javadoc/4.13/org/junit/Assert.html#assertThrows(java.lang.String,%20java.lang.Class,%20org.junit.function.ThrowingRunnable)) is:
   
   ```java
   public static <T extends Throwable> T assertThrows(String message,
                                                      Class<T> expectedThrowable,
                                                      ThrowingRunnable runnable)
   ```
   
   For `org.junit.jupiter.api.Assertions.assertThrows`, the [api](https://junit.org/junit5/docs/5.0.1/api/org/junit/jupiter/api/Assertions.html#assertThrows-java.lang.Class-org.junit.jupiter.api.function.Executable-java.lang.String-) is:
   ```java
   public static <T extends Throwable> T assertThrows(Class<T> expectedType,
                                                      Executable executable,
                                                      String message)
   ```
   
   I searched our Kafka code base and found there is [another place](https://github.com/apache/kafka/blob/9565a529e08d7aa36beac02c8e6115bcc87d2dc7/streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorStateManagerTest.java#L521) using the package `org.junit.Assert.assertThrows` to assert exception message so I follow that to use the same apii as well here.
   
   Please let me know if this is not proper.




-- 
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