[GitHub] [kafka] cadonna commented on a change in pull request #9779: KAFKA-10767: Adding test cases for all, reverseAll and reverseRange for ThreadCache

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

cadonna commented on a change in pull request #9779:
URL: https://github.com/apache/kafka/pull/9779#discussion_r596741592



##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/ThreadCacheTest.java
##########
@@ -252,20 +262,43 @@ public void shouldGetSameKeyAsPeekNext() {
         assertEquals(iterator.peekNextKey(), iterator.next().key);
     }
 
+    @Test
+    public void shouldGetSameKeyAsPeekNextReverseRange() {
+        final ThreadCache cache = new ThreadCache(logContext, 10000L, new MockStreamsMetrics(new Metrics()));
+        final Bytes theByte = Bytes.wrap(new byte[]{1});
+        cache.put(namespace, theByte, dirtyEntry(theByte.get()));
+        final ThreadCache.MemoryLRUCacheBytesIterator iterator = cache.reverseRange(namespace, Bytes.wrap(new byte[]{0}), theByte);
+        assertEquals(iterator.peekNextKey(), iterator.next().key);

Review comment:
       nit: see my comment about `assertThat()` above. 

##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/ThreadCacheTest.java
##########
@@ -252,20 +262,43 @@ public void shouldGetSameKeyAsPeekNext() {
         assertEquals(iterator.peekNextKey(), iterator.next().key);
     }
 
+    @Test
+    public void shouldGetSameKeyAsPeekNextReverseRange() {
+        final ThreadCache cache = new ThreadCache(logContext, 10000L, new MockStreamsMetrics(new Metrics()));
+        final Bytes theByte = Bytes.wrap(new byte[]{1});
+        cache.put(namespace, theByte, dirtyEntry(theByte.get()));
+        final ThreadCache.MemoryLRUCacheBytesIterator iterator = cache.reverseRange(namespace, Bytes.wrap(new byte[]{0}), theByte);
+        assertEquals(iterator.peekNextKey(), iterator.next().key);
+    }
+
     @Test
     public void shouldThrowIfNoPeekNextKey() {
         final ThreadCache cache = new ThreadCache(logContext, 10000L, new MockStreamsMetrics(new Metrics()));
         final ThreadCache.MemoryLRUCacheBytesIterator iterator = cache.range(namespace, Bytes.wrap(new byte[]{0}), Bytes.wrap(new byte[]{1}));
         assertThrows(NoSuchElementException.class, iterator::peekNextKey);
     }
 
+    @Test
+    public void shouldThrowIfNoPeekNextKeyReverseRange() {
+        final ThreadCache cache = new ThreadCache(logContext, 10000L, new MockStreamsMetrics(new Metrics()));
+        final ThreadCache.MemoryLRUCacheBytesIterator iterator = cache.reverseRange(namespace, Bytes.wrap(new byte[]{0}), Bytes.wrap(new byte[]{1}));
+        assertThrows(NoSuchElementException.class, iterator::peekNextKey);
+    }
+

Review comment:
       nit: could you try to deduplicate code here and in the other unit tests? Here for example, you could have one method like this:
   
   ```
       private void shouldThrowIfNoPeekNextKey(final Supplier<MemoryLRUCacheBytesIterator> methodUnderTest) {
           final ThreadCache.MemoryLRUCacheBytesIterator iterator = methodUnderTest.get();
           assertThrows(NoSuchElementException.class, iterator::peekNextKey);
       }
   ```
   
   and then two public tests
   
   ```
       @Test
       public void shouldThrowIfNoPeekNextKeyRange() {
           final ThreadCache cache = new ThreadCache(logContext, 10000L, new MockStreamsMetrics(new Metrics()));
           shouldThrowIfNoPeekNextKey(() -> cache.range(namespace, Bytes.wrap(new byte[]{0}), Bytes.wrap(new byte[]{1})));
       }
   
       @Test
       public void shouldThrowIfNoPeekNextKeyReverseRange() {
           final ThreadCache cache = new ThreadCache(logContext, 10000L, new MockStreamsMetrics(new Metrics()));
           shouldThrowIfNoPeekNextKey(() -> cache.reverseRange(namespace, Bytes.wrap(new byte[]{0}), Bytes.wrap(new byte[]{1})));
       }
   ```
   
   Admittedly, in this specific case, we would not win much but for other unit tests in this test class it may be worth. Try and then decide if it is worth or not.  

##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/ThreadCacheTest.java
##########
@@ -243,6 +243,16 @@ public void shouldPeekNextKey() {
         assertEquals(theByte, iterator.peekNextKey());
     }
 
+    @Test
+    public void shouldPeekNextKeyReverseRange() {
+        final ThreadCache cache = new ThreadCache(logContext, 10000L, new MockStreamsMetrics(new Metrics()));
+        final Bytes theByte = Bytes.wrap(new byte[]{1});
+        cache.put(namespace, theByte, dirtyEntry(theByte.get()));
+        final ThreadCache.MemoryLRUCacheBytesIterator iterator = cache.reverseRange(namespace, Bytes.wrap(new byte[]{0}), theByte);
+        assertEquals(theByte, iterator.peekNextKey());
+        assertEquals(theByte, iterator.peekNextKey());

Review comment:
       nit: Could you please use `assertThat(iterator.peekNextKey(), is(the Byte))` here?




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