[GitHub] [accumulo] ctubbsii commented on a change in pull request #2177: Adds tests to LogFileKeyTest

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

ctubbsii commented on a change in pull request #2177:
URL: https://github.com/apache/accumulo/pull/2177#discussion_r657201788



##########
File path: server/tserver/src/test/java/org/apache/accumulo/tserver/log/LogFileKeyTest.java
##########
@@ -98,4 +103,48 @@ public void testSortOrder() {
     }
 
   }
+
+  private void testToFromKey(int tabletId, long seq) throws IOException {
+    var logFileKey = nk(LogEvents.DEFINE_TABLET, tabletId, seq);
+    logFileKey.tablet = new KeyExtent(TableId.of("5"), new Text("b"), null);
+    Key k = logFileKey.toKey();
+
+    var converted = LogFileKey.fromKey(k);

Review comment:
       The use of `var` here obscures the types, which are not otherwise obvious. This doesn't need to be changed, but it's probably not the best use of `var`.

##########
File path: server/tserver/src/test/java/org/apache/accumulo/tserver/log/LogFileKeyTest.java
##########
@@ -98,4 +103,48 @@ public void testSortOrder() {
     }
 
   }
+
+  private void testToFromKey(int tabletId, long seq) throws IOException {
+    var logFileKey = nk(LogEvents.DEFINE_TABLET, tabletId, seq);
+    logFileKey.tablet = new KeyExtent(TableId.of("5"), new Text("b"), null);
+    Key k = logFileKey.toKey();
+
+    var converted = LogFileKey.fromKey(k);
+    assertEquals("Failed to convert tabletId = " + tabletId + " seq = " + seq, logFileKey,
+        converted);
+  }
+
+  @Test
+  public void convertToKeyLargeTabletId() throws IOException {
+    // test continuous range of numbers to make sure all cases of signed bytes are covered
+    for (int i = -1_000; i < 1_000_000; i++) {
+      testToFromKey(i, 1);
+    }
+  }
+
+  @Test
+  public void convertToKeyLargeSeq() throws IOException {
+    // use numbers large enough to cover all bytes used
+    testToFromKey(1, 8); // 1 byte
+    testToFromKey(1, 1_000L); // 2 bytes
+    testToFromKey(1, 1_222_333L); // 3 bytes
+    testToFromKey(1, 100_000_000L); // 4 bytes
+    testToFromKey(1, 111_222_333_444L); // 5 bytes
+    testToFromKey(1, 1_000_222_333_777L); // 6 bytes
+    testToFromKey(1, 5_222_000_222_333_777L); // 7 bytes
+    testToFromKey(1, 500_222_333_444_555_666L); // 8 bytes
+
+    // test with more tabletId bytes
+    testToFromKey(-1000, 500_222_333_444_555_666L);
+    testToFromKey(10_000_000, 500_222_333_444_555_666L);
+    testToFromKey(Integer.MAX_VALUE, Long.MAX_VALUE);
+    testToFromKey(Integer.MIN_VALUE, Long.MAX_VALUE);
+    testToFromKey(Integer.MIN_VALUE, 0);
+    testToFromKey(Integer.MAX_VALUE, 0);
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void testNegativeSeq() throws IOException {
+    testToFromKey(1, -1);
+  }

Review comment:
       Consider using `assertThrows` in future. You can do useful things, like return the thrown exception for additional verification, as in:
   
   ```java
     var e = assertThrows(IllegalArgumentException.class, () -> testToFromKey(1, -1));
     assertEquals("expected exception message", e.getMessage());
     assertTrue(e.getCause() instanceof SomeCauseExceptionType);
   ```
   
   It also gives you more control over checking which specific line throws the exception, rather than just checking that it's thrown at *some* point in the method's execution.




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