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:
[email protected]
