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:
us...@infra.apache.org
With regards,
Apache Git Services
