garydgregory commented on a change in pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#discussion_r541320735
##########
File path:
src/test/java/org/apache/commons/crypto/cipher/AbstractCipherTest.java
##########
@@ -58,11 +62,11 @@
0x08 };
private CryptoCipher enc, dec;
- @Before
+ @BeforeEach
public void setup() {
init();
- assertNotNull("cipherClass", cipherClass);
- assertNotNull("transformations", transformations);
+ assertNotNull( cipherClass, "cipherClass");
Review comment:
Fix formatting for this kind of change throughout.
##########
File path:
src/test/java/org/apache/commons/crypto/cipher/AbstractCipherTest.java
##########
@@ -123,7 +127,7 @@ public void cryptoTest() throws Exception {
for (final String tran : transformations) {
/** uses the small data set in {@link TestData} */
cipherTests = TestData.getTestData(tran);
- assertNotNull(tran, cipherTests);
+ assertNotNull( cipherTests);
Review comment:
Why are we loosing the failure message here?
##########
File path: src/test/java/org/apache/commons/crypto/cipher/OpenSslCipherTest.java
##########
@@ -18,174 +18,163 @@
package org.apache.commons.crypto.cipher;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
+
import java.nio.ByteBuffer;
import java.security.InvalidAlgorithmParameterException;
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import java.util.Properties;
+import java.util.concurrent.TimeUnit;
import javax.crypto.NoSuchPaddingException;
import javax.crypto.ShortBufferException;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.SecretKeySpec;
import javax.crypto.spec.GCMParameterSpec;
-import org.junit.Assert;
-import org.junit.Assume;
-import org.junit.Test;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
+
public class OpenSslCipherTest extends AbstractCipherTest {
@Override
public void init() {
- Assume.assumeTrue(OpenSsl.getLoadingFailureReason() == null);
+ assumeTrue(OpenSsl.getLoadingFailureReason() == null);
transformations = new String[] {
"AES/CBC/NoPadding",
"AES/CBC/PKCS5Padding",
"AES/CTR/NoPadding"};
cipherClass = OPENSSL_CIPHER_CLASSNAME;
}
- @Test(expected = NoSuchPaddingException.class, timeout = 120000)
- public void testInvalidPadding() throws Exception {
- Assume.assumeTrue(OpenSsl.getLoadingFailureReason() == null);
- OpenSsl.getInstance("AES/CTR/NoPadding2");
+ @Test
+ @Timeout(value = 120000, unit = TimeUnit.MILLISECONDS)
+ public void testInvalidPadding() {
+ assumeTrue(OpenSsl.getLoadingFailureReason() == null);
+ assertThrows(NoSuchPaddingException.class,
+ () -> OpenSsl.getInstance("AES/CTR/NoPadding2"));
}
- @Test(expected = NoSuchAlgorithmException.class, timeout = 120000)
- public void testInvalidMode() throws Exception {
- Assume.assumeTrue(OpenSsl.getLoadingFailureReason() == null);
- OpenSsl.getInstance("AES/CTR2/NoPadding");
+ @Test
+ @Timeout(value = 120000, unit = TimeUnit.MILLISECONDS)
+ public void testInvalidMode() {
+ assumeTrue(OpenSsl.getLoadingFailureReason() == null);
+ assertThrows(NoSuchAlgorithmException.class,
+ () -> OpenSsl.getInstance("AES/CTR2/NoPadding"));
}
- @Test(timeout = 120000)
+ @Test
+ @Timeout(value = 120000, unit = TimeUnit.MILLISECONDS)
public void testUpdateArguments() throws Exception {
- Assume.assumeTrue(OpenSsl.getLoadingFailureReason() == null);
+ assumeTrue(OpenSsl.getLoadingFailureReason() == null);
final OpenSsl cipher = OpenSsl
.getInstance("AES/CTR/NoPadding");
- Assert.assertNotNull(cipher);
+ assertNotNull(cipher);
cipher.init(OpenSsl.ENCRYPT_MODE, KEY, new IvParameterSpec(IV));
// Require direct buffers
ByteBuffer input = ByteBuffer.allocate(1024);
ByteBuffer output = ByteBuffer.allocate(1024);
- try {
- cipher.update(input, output);
- Assert.fail("Should have failed to accept non-direct buffers.");
- } catch (final IllegalArgumentException e) {
- Assert.assertTrue(e.getMessage().contains(
- "Direct buffers are required"));
- }
+ final ByteBuffer finalInput = input;
+ final ByteBuffer finalOutput = output;
+ assertThrows(IllegalArgumentException.class, () ->
cipher.update(finalInput, finalOutput));
// Output buffer length should be sufficient to store output data
input = ByteBuffer.allocateDirect(1024);
output = ByteBuffer.allocateDirect(1000);
- try {
- cipher.update(input, output);
- Assert.fail("Failed to check for output buffer size.");
- } catch (final ShortBufferException e) {
- Assert.assertTrue(e.getMessage().contains(
- "Output buffer is not sufficient"));
- }
+ final ByteBuffer finalInput1 = input;
+ final ByteBuffer finalOutput1 = output;
+ assertThrows(ShortBufferException.class, () ->
cipher.update(finalInput1, finalOutput1));
}
- @Test(timeout = 120000)
+ @Test
+ @Timeout(value = 120000, unit = TimeUnit.MILLISECONDS)
public void testDoFinalArguments() throws Exception {
- Assume.assumeTrue(OpenSsl.getLoadingFailureReason() == null);
+ assumeTrue(OpenSsl.getLoadingFailureReason() == null);
final OpenSsl cipher = OpenSsl
.getInstance("AES/CTR/NoPadding");
- Assert.assertNotNull(cipher);
+ assertNotNull(cipher);
cipher.init(OpenSsl.ENCRYPT_MODE, KEY, new IvParameterSpec(IV));
// Require direct buffer
final ByteBuffer input = ByteBuffer.allocate(1024);
final ByteBuffer output = ByteBuffer.allocate(1024);
- try {
- cipher.doFinal(input, output);
- Assert.fail("Should have failed to accept non-direct buffers.");
- } catch (final IllegalArgumentException e) {
- Assert.assertTrue(e.getMessage().contains(
- "Direct buffer is required"));
- }
+ assertThrows(IllegalArgumentException.class, () ->
cipher.doFinal(input, output));
}
@Override
- @Test(expected = InvalidKeyException.class, timeout = 120000)
+ @Test
+ @Timeout(value = 120000, unit = TimeUnit.MILLISECONDS)
public void testInvalidKey() throws Exception {
- Assume.assumeTrue(OpenSsl.getLoadingFailureReason() == null);
+ assumeTrue(OpenSsl.getLoadingFailureReason() == null);
final OpenSsl cipher = OpenSsl
.getInstance("AES/CTR/NoPadding");
- Assert.assertNotNull(cipher);
+ assertNotNull(cipher);
final byte[] invalidKey = { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06,
0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x11 };
- cipher.init(OpenSsl.ENCRYPT_MODE, invalidKey, new IvParameterSpec(IV));
+
+
Review comment:
Remove extra line.
##########
File path:
src/test/java/org/apache/commons/crypto/random/OsCryptoRandomTest.java
##########
@@ -48,19 +47,6 @@ public void testInvalidRandom() {
props.setProperty(CryptoRandomFactory.CLASSES_KEY,
OsCryptoRandom.class.getName());
// Invalid device
props.setProperty(CryptoRandomFactory.DEVICE_FILE_PATH_KEY, "");
- try {
- CryptoRandomFactory.getCryptoRandom(props);
- fail("Expected GeneralSecurityException");
- } catch (final GeneralSecurityException e) {
- Throwable cause;
- cause = e.getCause();
- Assert.assertEquals(IllegalArgumentException.class,
cause.getClass());
- cause = cause.getCause();
- Assert.assertEquals(InvocationTargetException.class,
cause.getClass());
- cause = cause.getCause();
- Assert.assertEquals(IllegalArgumentException.class,
cause.getClass());
- cause = cause.getCause();
- Assert.assertEquals(FileNotFoundException.class,
cause.getClass());
- }
+ assertThrows(GeneralSecurityException.class, () ->
CryptoRandomFactory.getCryptoRandom(props));
Review comment:
You've changed the test here a lot, and lost a lot. I did not write this
code so I can't say exactly why the test was written this way but I would not
just blow away all of this.
##########
File path:
src/test/java/org/apache/commons/crypto/cipher/AbstractCipherTest.java
##########
@@ -300,12 +296,11 @@ private void byteArrayTest(final String transformation,
final byte[] key, final
plainPos += dec.doFinal(cipherText, offset,
cipherPos % bufferLen, realPlainText, plainPos);
// verify
- Assert.assertEquals("random byte array length
changes after transformation", dataLen, plainPos);
+ assertEquals(dataLen, plainPos,"random byte
array length changes after transformation");
Review comment:
Fix formatting.
##########
File path:
src/test/java/org/apache/commons/crypto/stream/CtrCryptoStreamTest.java
##########
@@ -176,14 +162,8 @@ protected void doDecryptTest(final String cipherClass,
final boolean withChannel
final byte[] expectedData = new byte[dataLen];
buf.get(readData);
System.arraycopy(data, 0, expectedData, 0, dataLen);
- Assert.assertArrayEquals(readData, expectedData);
-
- try {
- in.decryptBuffer(buf);
- Assert.fail("Expected IOException.");
- } catch (final IOException ex) {
- Assert.assertEquals(ex.getCause().getClass(),
ShortBufferException.class);
- }
+ assertArrayEquals(readData, expectedData);
+ assertThrows(IOException.class, () -> in.decryptBuffer(buf));
Review comment:
Again, you've lost some of the testing. In this case, the test checks
for ShortBufferException.
I'm going to pause my review of this PR until you revisit all changes to
make sure you keep/restore ALL the test assertions.
##########
File path:
src/test/java/org/apache/commons/crypto/stream/AbstractCipherStreamTest.java
##########
@@ -814,8 +769,7 @@ private void doReadWriteTestForReadableByteChannel(final
int count,
int expected;
do {
expected = originalIn.read();
- Assert.assertEquals("Decrypted stream read by byte does not match",
- expected, in.read());
+ assertEquals(expected, in.read());
Review comment:
You've lost the message in this and above changes, make sure you keep
those.
----------------------------------------------------------------
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]
