[GitHub] [accumulo] ctubbsii commented on a change in pull request #2472: Convert try-catch to assertThrows

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

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



##########
File path: core/src/test/java/org/apache/accumulo/core/clientImpl/TableOperationsHelperTest.java
##########
@@ -327,23 +327,15 @@ public void testAttachIterator() throws Exception {
             "table.iterator.minc.otherName.opt.key=value",
             "table.iterator.scan.otherName=20,some.classname",});
 
-    try {
-      t.attachIterator("table", setting);
-      fail();
-    } catch (AccumuloException e) {
-      // expected, ignore
-    }
+    IteratorSetting finalSetting = setting; // effectively final var needed for lambda below

Review comment:
       Could probably remove the explanation by just being explicit about the final, when you encounter these. For example:
   
   ```java
       final var finalSetting = setting;
   ```
   
   However, a better idea is to use new variables. I looked at the lines above, and it looks like there's no good reason they need to reuse the variable name. It just as easily could have had IteratorSetting setting1, setting2, etc., or more specific names if there are any that make sense.

##########
File path: core/src/test/java/org/apache/accumulo/core/clientImpl/TabletLocatorImplTest.java
##########
@@ -1281,12 +1281,8 @@ public void testAccumulo1248() {
     setLocation(tservers, "tserver2", MTE, ke1, "L1", "I1");
     setLocation(tservers, "tserver2", MTE, ke1, "L2", "I2");
 
-    try {
-      metaCache.locateTablet(context, new Text("a"), false, false);
-      fail();
-    } catch (Exception e) {
-
-    }
+    assertThrows(Exception.class,
+        () -> metaCache.locateTablet(context, new Text("a"), false, false));

Review comment:
       It'd be great if this code was more specific about which exception it was expecting.

##########
File path: core/src/test/java/org/apache/accumulo/core/iterators/system/SourceSwitchingIteratorTest.java
##########
@@ -286,10 +286,8 @@ public void testSetInterrupt() throws Exception {
 
     flag.set(true);
 
-    try {
-      ssi.seek(new Range("r1"), new ArrayList<>(), false);
-      fail("expected to see IterationInterruptedException");
-    } catch (IterationInterruptedException iie) {}
+    assertThrows(IterationInterruptedException.class,
+        () -> ssi.seek(new Range("r1"), new ArrayList<>(), false));

Review comment:
       Presumably, these are failing on the `seek` method, and not on `new Range`. But the code isn't clear. It could be written like:
   
   ```suggestion
       var range = new Range("r1");
       var newList = new ArrayList<?>();
       assertThrows(IterationInterruptedException.class, () -> ssi.seek(range, newList, false));
   ```
   
   In general, it's best to assert the throws on the narrow-est method possible. The old code is sub-par for this reason as well, so it's not a problem you introduced. But, it would be a good opportunity to clean these up.

##########
File path: core/src/test/java/org/apache/accumulo/core/iterators/system/DeletingIteratorTest.java
##########
@@ -244,13 +245,12 @@ public void testFail() throws IOException {
     SortedKeyValueIterator<Key,Value> it =
         DeletingIterator.wrap(new SortedMapIterator(tm), false, Behavior.FAIL);
     it.seek(new Range(), EMPTY_COL_FAMS, false);
-    try {
+    assertThrows(IllegalStateException.class, () -> {
       while (it.hasTop()) {
         it.getTopKey();
         it.next();
       }
-      fail();
-    } catch (IllegalStateException e) {}
+    });

Review comment:
       This could probably be improved if it were to only assertThrows on the specific method that is being called. It's not clear here if it is expected to be throw on `hasTop`, `getTopKey`, or `next`. It's also not clear if it's supposed to be thrown on the first pass through the loop, or a subsequent one.




-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org