[GitHub] [kafka] ableegoldman commented on a change in pull request #11214: KAFKA-12994 Migrate JoinWindowsTest and SessionWindowsTest to new API

[GitBox]

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



##########
File path: 
streams/src/test/java/org/apache/kafka/streams/kstream/JoinWindowsTest.java
##########
@@ -101,102 +93,100 @@ public void startTimeShouldNotBeAfterEnd() {
         }
     }
 
-    @Test
-    public void untilShouldSetGraceDuration() {
-        final JoinWindows windowSpec = JoinWindows.of(ofMillis(ANY_SIZE));
-        final long windowSize = windowSpec.size();
-        assertEquals(windowSize, 
windowSpec.grace(ofMillis(windowSize)).gracePeriodMs());
-    }
-
     @Test
     public void gracePeriodShouldEnforceBoundaries() {
-        JoinWindows.of(ofMillis(3L)).grace(ofMillis(0L));
+        JoinWindows.ofTimeDifferenceAndGrace(ofMillis(3L), ofMillis(0L));
 
         try {
-            JoinWindows.of(ofMillis(3L)).grace(ofMillis(-1L));
+            JoinWindows.ofTimeDifferenceAndGrace(ofMillis(3L), ofMillis(-1L));
             fail("should not accept negatives");
         } catch (final IllegalArgumentException e) {
             //expected
         }
     }
 
     @Test
-    public void oldAPIShouldSetDefaultGracePeriod() {

Review comment:
       Ditto here, we should retain some version of this test and any others 
that are specifically intending to test the behavior of the old API (until the 
deprecation period has elapsed and we can remove it)

##########
File path: 
streams/src/test/java/org/apache/kafka/streams/kstream/SessionWindowsTest.java
##########
@@ -18,104 +18,91 @@
 
 import org.junit.Test;
 
-import java.time.Duration;
-
 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.assertThrows;
 import static org.junit.Assert.fail;
 
-@SuppressWarnings("deprecation")
 public class SessionWindowsTest {
 
+    private static final long ANY_SIZE = 123L;
+    private static final long ANY_OTHER_SIZE = 456L; // should be larger than 
anySize
+    private static final long ANY_GRACE = 1024L;
+
     @Test
     public void shouldSetWindowGap() {
         final long anyGap = 42L;
-        final long anyGrace = 1024L;
 
-        assertEquals(anyGap, 
SessionWindows.with(ofMillis(anyGap)).inactivityGap());
         assertEquals(anyGap, 
SessionWindows.ofInactivityGapWithNoGrace(ofMillis(anyGap)).inactivityGap());
-        assertEquals(anyGap, 
SessionWindows.ofInactivityGapAndGrace(ofMillis(anyGap), 
ofMillis(anyGrace)).inactivityGap());
-    }
-
-    @Test
-    public void shouldSetWindowGraceTime() {
-        final long anyRetentionTime = 42L;
-        assertEquals(anyRetentionTime, 
SessionWindows.with(ofMillis(1)).grace(ofMillis(anyRetentionTime)).gracePeriodMs());
+        assertEquals(anyGap, 
SessionWindows.ofInactivityGapAndGrace(ofMillis(anyGap), 
ofMillis(ANY_GRACE)).inactivityGap());
     }
 
     @Test
     public void gracePeriodShouldEnforceBoundaries() {
-        SessionWindows.with(ofMillis(3L)).grace(ofMillis(0));
+        SessionWindows.ofInactivityGapAndGrace(ofMillis(3L), ofMillis(0));
 
         try {
-            SessionWindows.with(ofMillis(3L)).grace(ofMillis(-1L));
+            SessionWindows.ofInactivityGapAndGrace(ofMillis(3L), 
ofMillis(-1L));
             fail("should not accept negatives");
         } catch (final IllegalArgumentException e) {
             //expected
         }
     }
 
     @Test
-    public void oldAPIShouldSetDefaultGracePeriod() {

Review comment:
       Same here -- I'll stop commenting on each case, just give it a pass 
yourself to look out for any tests that should be kept to make sure we don't 
somehow introduce a regression in the deprecated APIs 

##########
File path: 
streams/src/test/java/org/apache/kafka/streams/kstream/JoinWindowsTest.java
##########
@@ -101,102 +93,100 @@ public void startTimeShouldNotBeAfterEnd() {
         }
     }
 
-    @Test
-    public void untilShouldSetGraceDuration() {

Review comment:
       We should avoid outright removing tests that cover the old deprecated 
APIs until we actually get to remove those methods. Instead we can just scope 
the deprecation warning suppression to the individual tests that cover the 
behavior of the old APIs like here, and a few places below




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