[GitHub] ctubbsii commented on a change in pull request #761: Fix warnings and jar sealing issues with ITs

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

ctubbsii commented on a change in pull request #761: Fix warnings and jar sealing issues with ITs
URL: https://github.com/apache/accumulo/pull/761#discussion_r233144094
 
 

 ##########
 File path: core/src/test/java/org/apache/accumulo/core/security/crypto/CryptoTest.java
 ##########
 @@ -339,13 +344,14 @@ public void testAESKeyUtilsLoadKekFromUriInvalidUri() {
     exception.expect(CryptoException.class);
     SecretKeySpec fileKey = AESKeyUtils.loadKekFromUri(
         System.getProperty("user.dir") + "/target/CryptoTest-testkeyfile-doesnt-exist");
+    fail("creation of " + fileKey + " should fail");
   }
 
   @Test
   public void testAESKeyUtilsLoadKekFromEmptyFile() {
     exception.expect(CryptoException.class);
     SecretKeySpec fileKey = AESKeyUtils.loadKekFromUri(emptyKeyPath);
-
+    fail("creation of " + fileKey + " should fail");
 
 Review comment:
   I almost changed it that way, for that reason. However, I've found that with the dynamic checks for thrown exceptions using `ExpectedException.expect(Class)`, the test code is made more robust by explicitly having a `fail()` call following the line which is expected to throw. Otherwise, the test could be inadvertently modified to add additional lines of code, and the subsequent lines could throw the expected exception, instead of the intended line (especially if the expected exception is too generic, like `IOException`). Best practice is to call `expect`, then execute the code expected to throw, then call `fail`.
   
   Of course, following this pattern does not require keeping the assignment... but some spotbugs warnings occur on some APIs if you don't use/check the return value. Initially, I didn't care to check which was the case for this particular API, since it doesn't really matter here... the use in the `fail` message is trivial enough. However, I realized that there's a benefit to including it in the message... if the exception is not thrown... it'll be useful to see more information about what actually happened in the `fail` message. In any case, fixing this warning was secondary to the main issue, which was to fix the build.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services