You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/12/06 07:54:28 UTC
[GitHub] [commons-crypto] arturobernalg opened a new pull request #114: [CRYPTO-151] - Migrate to Junit 5
arturobernalg opened a new pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114
Migrate to Junit 5
https://issues.apache.org/jira/browse/CRYPTO-151
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on a change in pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#discussion_r537046441
##########
File path: src/test/java/org/apache/commons/crypto/stream/PositionedCryptoInputStreamTest.java
##########
@@ -247,7 +250,7 @@ private void testSeekFailed(final String cipherClass, final int position, final
getCipher(cipherClass), bufferSize);
try {
in.seek(position);
- Assert.fail("Excepted exception for cannot seek to negative offset.");
+ fail("Excepted exception for cannot seek to negative offset.");
} catch (final IllegalArgumentException iae) {
Review comment:
Hi @arturobernalg
Thank you for your PR.
If we go to JUnit 5, then let's remove this pattern with `assertThrows()`
WDYT?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-743858956
@garydgregory
That's true. I was just saying that those message are unnecessary IMO. But I get your point, and you're right. The propose of the PR it's migrate to Junit 5, not improve the test or the message.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls commented on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35670437)
Coverage decreased (-0.5%) to 82.635% when pulling **667614f90ff46844e5dddf8b9b2f29dc73eaeb5d on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-751490927
Done
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35670437)
Coverage decreased (-0.5%) to 82.635% when pulling **667614f90ff46844e5dddf8b9b2f29dc73eaeb5d on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35956974)
Coverage decreased (-0.6%) to 82.036% when pulling **2ed4f9b3f4276274b6e5a2f8e13ec3398fe3b5f9 on arturobernalg:feature/CRYPTO-151** into **2ed0f2bdc537fafba18f27e26422f3d24aae7716 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35666252)
Coverage decreased (-0.5%) to 82.635% when pulling **c5342c60028912215b181f91098f4baabb1d56e7 on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on a change in pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#discussion_r548542504
##########
File path: src/test/java/org/apache/commons/crypto/cipher/AbstractCipherTest.java
##########
@@ -251,13 +249,13 @@ private void byteArrayTest(final String transformation, final byte[] key, final
final int n = enc.doFinal(input, 0, input.length, temp, 0);
final byte[] cipherText = new byte[n];
System.arraycopy(temp, 0, cipherText, 0, n);
- Assert.assertArrayEquals("byte array encryption error.", output, cipherText);
+ assertArrayEquals( output, cipherText, "byte array encryption error.");
temp = new byte[cipherText.length + blockSize];
final int m = dec.doFinal(cipherText, 0, cipherText.length, temp, 0);
final byte[] plainText = new byte[m];
System.arraycopy(temp, 0, plainText, 0, m);
- Assert.assertArrayEquals("byte array decryption error.", input, plainText);
+ assertArrayEquals( input, plainText,"byte array decryption error.");
Review comment:
Done
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-750802626
HI @garydgregory
Any progress here?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35672010)
Coverage remained the same at 83.179% when pulling **786bf78fda7de88087aef50d1e8fe0d54d6cf57d on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35942598)
Coverage increased (+0.5%) to 83.179% when pulling **8eedf711fa01376e98741d3675da9f1740c06229 on arturobernalg:feature/CRYPTO-151** into **91650c2770aea4ee09c39bdc7ae81234b79912b0 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35666230)
Coverage decreased (-0.5%) to 82.635% when pulling **c5342c60028912215b181f91098f4baabb1d56e7 on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on a change in pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#discussion_r548542582
##########
File path: src/test/java/org/apache/commons/crypto/cipher/AbstractCipherTest.java
##########
@@ -251,13 +249,13 @@ private void byteArrayTest(final String transformation, final byte[] key, final
final int n = enc.doFinal(input, 0, input.length, temp, 0);
final byte[] cipherText = new byte[n];
System.arraycopy(temp, 0, cipherText, 0, n);
- Assert.assertArrayEquals("byte array encryption error.", output, cipherText);
+ assertArrayEquals( output, cipherText, "byte array encryption error.");
Review comment:
Done
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35666230)
Coverage decreased (-0.5%) to 82.635% when pulling **c5342c60028912215b181f91098f4baabb1d56e7 on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35936285)
Coverage decreased (-0.5%) to 82.635% when pulling **92121d09200ab08a1b27fa15a84b347da549aab1 on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35670460)
Coverage decreased (-0.5%) to 82.635% when pulling **786bf78fda7de88087aef50d1e8fe0d54d6cf57d on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on a change in pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#discussion_r548771093
##########
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,tran);
Review comment:
Fix formatting.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on a change in pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#discussion_r548771173
##########
File path: src/test/java/org/apache/commons/crypto/cipher/AbstractCipherTest.java
##########
@@ -251,13 +249,13 @@ private void byteArrayTest(final String transformation, final byte[] key, final
final int n = enc.doFinal(input, 0, input.length, temp, 0);
final byte[] cipherText = new byte[n];
System.arraycopy(temp, 0, cipherText, 0, n);
- Assert.assertArrayEquals("byte array encryption error.", output, cipherText);
+ assertArrayEquals(output, cipherText, "byte array encryption error.");
temp = new byte[cipherText.length + blockSize];
final int m = dec.doFinal(cipherText, 0, cipherText.length, temp, 0);
final byte[] plainText = new byte[m];
System.arraycopy(temp, 0, plainText, 0, m);
- Assert.assertArrayEquals("byte array decryption error.", input, plainText);
+ assertArrayEquals(input, plainText,"byte array decryption error.");
Review comment:
Fix formatting.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35942548)
Coverage remained the same at 83.179% when pulling **ad7061571128573c943d48ad043f2659318fcdf5 on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-751190861
HI @garydgregory
I think they should be fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on a change in pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#discussion_r541744144
##########
File path: src/test/java/org/apache/commons/crypto/cipher/AbstractCipherTest.java
##########
@@ -251,13 +247,13 @@ private void byteArrayTest(final String transformation, final byte[] key, final
final int n = enc.doFinal(input, 0, input.length, temp, 0);
final byte[] cipherText = new byte[n];
System.arraycopy(temp, 0, cipherText, 0, n);
- Assert.assertArrayEquals("byte array encryption error.", output, cipherText);
+ assertArrayEquals( output, cipherText);
Review comment:
You lost the assertion message and the formatting should match the rest of the code.
##########
File path: src/test/java/org/apache/commons/crypto/cipher/AbstractCipherTest.java
##########
@@ -220,9 +218,7 @@ private void byteBufferTest(final String transformation, final byte[] key, final
final byte[] b = new byte[output.remaining()];
output.get(b);
final byte[] c = new byte[encResult.remaining()];
- encResult.get(c);
- Assert.fail("AES failed encryption - expected " + new String(DatatypeConverter.printHexBinary(b))
- + " got " + new String(DatatypeConverter.printHexBinary(c)));
+ assertThrows(IllegalArgumentException.class, () -> encResult.get(c));
Review comment:
Please preserve the assertion message, it will be handy when the test fails.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-751490390
@arturobernalg
One last thing: Please rebase on master to pick up the default Maven goal for Travis CI builds. This will make sure PRs don't break anything WRT Javadoc and so on.
TY!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-743781742
> Hi @garydgregory
> As you can imaginate I don't think validating the exception message will helps with the functional testing. But in any case, i made the revert and adapt to JUnit 5.
> I've check of all the new change
@arturobernalg
Maybe so, but this is a large PR that is _supposed_ to only _port_ the tests from JUnit 4 to 5, so it is best IMO to _not_ change _what_ is being tested or _how_ it is being tested.
As you can imagine, it's a slog to go through a large PR ;-)
If you want to improve the tests beyond this port, then sure go ahead, but I don't see a case for removing something successfully tested for now.
Gary
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35670460)
Coverage decreased (-0.5%) to 82.635% when pulling **786bf78fda7de88087aef50d1e8fe0d54d6cf57d on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35670437)
Coverage decreased (-0.5%) to 82.635% when pulling **667614f90ff46844e5dddf8b9b2f29dc73eaeb5d on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35670460)
Coverage decreased (-0.5%) to 82.635% when pulling **786bf78fda7de88087aef50d1e8fe0d54d6cf57d on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35666252)
Coverage decreased (-0.5%) to 82.635% when pulling **c5342c60028912215b181f91098f4baabb1d56e7 on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
arturobernalg edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-743749201
Hi @garydgregory
As you can imaginate I don't think validating the exception message will helps with the functional testing. But in any case, i made the revert and adapt to JUnit 5.
I've check of all the new change
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35787033)
Coverage remained the same at 83.179% when pulling **517960f2eeaaa5e909a113290ad4f63696e1b38e on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35936299)
Coverage remained the same at 83.179% when pulling **92121d09200ab08a1b27fa15a84b347da549aab1 on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on a change in pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#discussion_r542031287
##########
File path: src/test/java/org/apache/commons/crypto/cipher/AbstractCipherTest.java
##########
@@ -251,13 +249,13 @@ private void byteArrayTest(final String transformation, final byte[] key, final
final int n = enc.doFinal(input, 0, input.length, temp, 0);
final byte[] cipherText = new byte[n];
System.arraycopy(temp, 0, cipherText, 0, n);
- Assert.assertArrayEquals("byte array encryption error.", output, cipherText);
+ assertArrayEquals( output, cipherText, "byte array encryption error.");
temp = new byte[cipherText.length + blockSize];
final int m = dec.doFinal(cipherText, 0, cipherText.length, temp, 0);
final byte[] plainText = new byte[m];
System.arraycopy(temp, 0, plainText, 0, m);
- Assert.assertArrayEquals("byte array decryption error.", input, plainText);
+ assertArrayEquals( input, plainText,"byte array decryption error.");
Review comment:
Fix formatting.
##########
File path: src/test/java/org/apache/commons/crypto/cipher/AbstractCipherTest.java
##########
@@ -251,13 +249,13 @@ private void byteArrayTest(final String transformation, final byte[] key, final
final int n = enc.doFinal(input, 0, input.length, temp, 0);
final byte[] cipherText = new byte[n];
System.arraycopy(temp, 0, cipherText, 0, n);
- Assert.assertArrayEquals("byte array encryption error.", output, cipherText);
+ assertArrayEquals( output, cipherText, "byte array encryption error.");
Review comment:
Fix formatting.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35662023)
Coverage increased (+0.6%) to 83.179% when pulling **2008b13b138e4ce7ba8c1ebb0afe9a288ee22597 on arturobernalg:feature/CRYPTO-151** into **3c9c78d6185402108ac7ffb2b77d85e555eba5c1 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-743749201
Hi @garydgregory
As you can imaginate I don't think validating the exception message helps functional testing. But any case i made the revert and adapt to JUnit 5.
I've check of all the new change
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-749002653
HI @garydgregory
I've made all changes
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on a change in pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#discussion_r541778301
##########
File path: src/test/java/org/apache/commons/crypto/cipher/AbstractCipherTest.java
##########
@@ -220,9 +218,7 @@ private void byteBufferTest(final String transformation, final byte[] key, final
final byte[] b = new byte[output.remaining()];
output.get(b);
final byte[] c = new byte[encResult.remaining()];
- encResult.get(c);
- Assert.fail("AES failed encryption - expected " + new String(DatatypeConverter.printHexBinary(b))
- + " got " + new String(DatatypeConverter.printHexBinary(c)));
+ assertThrows(IllegalArgumentException.class, () -> encResult.get(c));
Review comment:
Done
##########
File path: src/test/java/org/apache/commons/crypto/cipher/AbstractCipherTest.java
##########
@@ -251,13 +247,13 @@ private void byteArrayTest(final String transformation, final byte[] key, final
final int n = enc.doFinal(input, 0, input.length, temp, 0);
final byte[] cipherText = new byte[n];
System.arraycopy(temp, 0, cipherText, 0, n);
- Assert.assertArrayEquals("byte array encryption error.", output, cipherText);
+ assertArrayEquals( output, cipherText);
Review comment:
Done
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on a change in pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [commons-crypto] arturobernalg commented on a change in pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#discussion_r537060004
##########
File path: src/test/java/org/apache/commons/crypto/stream/PositionedCryptoInputStreamTest.java
##########
@@ -247,7 +250,7 @@ private void testSeekFailed(final String cipherClass, final int position, final
getCipher(cipherClass), bufferSize);
try {
in.seek(position);
- Assert.fail("Excepted exception for cannot seek to negative offset.");
+ fail("Excepted exception for cannot seek to negative offset.");
} catch (final IllegalArgumentException iae) {
Review comment:
Hi @garydgregory
Agree with the approach. Changed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35666230)
Coverage decreased (-0.5%) to 82.635% when pulling **c5342c60028912215b181f91098f4baabb1d56e7 on arturobernalg:feature/CRYPTO-151** into **faa474d2f5cc878e64a41339b764881c79c60cb3 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory commented on a change in pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#discussion_r537046441
##########
File path: src/test/java/org/apache/commons/crypto/stream/PositionedCryptoInputStreamTest.java
##########
@@ -247,7 +250,7 @@ private void testSeekFailed(final String cipherClass, final int position, final
getCipher(cipherClass), bufferSize);
try {
in.seek(position);
- Assert.fail("Excepted exception for cannot seek to negative offset.");
+ fail("Excepted exception for cannot seek to negative offset.");
} catch (final IllegalArgumentException iae) {
Review comment:
Hi @arturobernalg
Thank you for your PR.
If we go to JUnit 5, then let's replace this `try-catch` pattern with `assertThrows()`
WDYT?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] coveralls edited a comment on pull request #114: [CRYPTO-151] - Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114#issuecomment-739533664
[](https://coveralls.io/builds/35662023)
Coverage increased (+0.03%) to 82.635% when pulling **2008b13b138e4ce7ba8c1ebb0afe9a288ee22597 on arturobernalg:feature/CRYPTO-151** into **3c9c78d6185402108ac7ffb2b77d85e555eba5c1 on apache:master**.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [commons-crypto] garydgregory merged pull request #114: [CRYPTO-151] Migrate to Junit 5
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #114:
URL: https://github.com/apache/commons-crypto/pull/114
----------------------------------------------------------------
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:
users@infra.apache.org