[GitHub] [kafka] ableegoldman commented on a change in pull request #11215: KAFKA-12994: Migrate TimeWindowsTest to new API

[GitBox]

ableegoldman commented on a change in pull request #11215:
URL: https://github.com/apache/kafka/pull/11215#discussion_r692546162



##########
File path: 
streams/src/test/java/org/apache/kafka/streams/kstream/TimeWindowsTest.java
##########
@@ -95,27 +91,19 @@ public void advanceIntervalMustNotBeLargerThanWindowSize() {
 
     @Test
     public void gracePeriodShouldEnforceBoundaries() {
-        TimeWindows.of(ofMillis(3L)).grace(ofMillis(0L));
+        TimeWindows.ofSizeAndGrace(ofMillis(3L), ofMillis(0L));
 
         try {
-            TimeWindows.of(ofMillis(3L)).grace(ofMillis(-1L));
+            TimeWindows.ofSizeAndGrace(ofMillis(3L), ofMillis(-1L));
             fail("should not accept negatives");
         } catch (final IllegalArgumentException e) {
             //expected
         }
     }
 
-    @Test
-    public void oldAPIShouldSetDefaultGracePeriod() {
-        assertEquals(Duration.ofDays(1).toMillis(), 
DEPRECATED_DEFAULT_24_HR_GRACE_PERIOD);
-        assertEquals(DEPRECATED_DEFAULT_24_HR_GRACE_PERIOD - 3L, 
TimeWindows.of(ofMillis(3L)).gracePeriodMs());
-        assertEquals(0L, 
TimeWindows.of(ofMillis(DEPRECATED_DEFAULT_24_HR_GRACE_PERIOD)).gracePeriodMs());
-        assertEquals(0L, 
TimeWindows.of(ofMillis(DEPRECATED_DEFAULT_24_HR_GRACE_PERIOD + 
1L)).gracePeriodMs());
-    }
-

Review comment:
       Same here: we should leave this test here until we remove the deprecated 
API. (and just suppress the warnings for only this test method)

##########
File path: 
streams/src/test/java/org/apache/kafka/streams/kstream/TimeWindowsTest.java
##########
@@ -19,50 +19,46 @@
 import org.apache.kafka.streams.kstream.internals.TimeWindow;
 import org.junit.Test;
 
-import java.time.Duration;
 import java.util.Map;
 
 import static java.time.Duration.ofMillis;
 import static org.apache.kafka.streams.EqualityCheck.verifyEquality;
 import static org.apache.kafka.streams.EqualityCheck.verifyInEquality;
-import static 
org.apache.kafka.streams.kstream.Windows.DEPRECATED_DEFAULT_24_HR_GRACE_PERIOD;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.fail;
 
-@SuppressWarnings("deprecation")
 public class TimeWindowsTest {
 
     private static final long ANY_SIZE = 123L;
     private static final long ANY_GRACE = 1024L;
 
     @Test
     public void shouldSetWindowSize() {
-        assertEquals(ANY_SIZE, TimeWindows.of(ofMillis(ANY_SIZE)).sizeMs);

Review comment:
       Let's leave this one in here, as long as we still support a deprecated 
API we should continue to test that it works. We just shouldn't use deprecated 
APIs to test other functionality unrelated to the API itself (eg that an 
exception is thrown for window size of 0).
   
   Then we can restrict the scope of the deprecation warning to just this one 
test, rather than the entire class




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