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 2022/12/29 16:30:00 UTC
[GitHub] [commons-compress] theobisproject opened a new pull request, #346: Use JUnit 5 assertions in JUnit 5 tests instead of JUnit 4 classes
theobisproject opened a new pull request, #346:
URL: https://github.com/apache/commons-compress/pull/346
Only the `OsgiITest` still uses the JUnit 4 test annotation. Migrate all other tests to make use of the JUnit 5 api only.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory closed pull request #346: Use JUnit 5 assertions in JUnit 5 tests instead of JUnit 4 classes
Posted by GitBox <gi...@apache.org>.
garydgregory closed pull request #346: Use JUnit 5 assertions in JUnit 5 tests instead of JUnit 4 classes
URL: https://github.com/apache/commons-compress/pull/346
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] kinow commented on a diff in pull request #346: Use JUnit 5 assertions in JUnit 5 tests instead of JUnit 4 classes
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #346:
URL: https://github.com/apache/commons-compress/pull/346#discussion_r1059077582
##########
src/test/java/org/apache/commons/compress/archivers/tar/SparseFilesTest.java:
##########
@@ -119,9 +120,8 @@ private String getTarBinaryHelp() throws IOException {
}
@Test
+ @DisabledOnOs(OS.WINDOWS)
Review Comment:
:ok_man:
##########
src/test/java/org/apache/commons/compress/archivers/TarTestCase.java:
##########
@@ -400,7 +400,7 @@ public void testTarFileCOMPRESS178() throws Exception {
fail("Expected IOException");
} catch (final IOException e) {
final Throwable t = e.getCause();
- assertTrue("Expected cause = IllegalArgumentException", t instanceof IllegalArgumentException);
+ assertTrue(t instanceof IllegalArgumentException, "Expected cause = IllegalArgumentException");
Review Comment:
Maybe we should move to assertThrows here too?
##########
src/test/java/org/apache/commons/compress/compressors/bzip2/PythonTruncatedBzip2Test.java:
##########
@@ -105,7 +105,7 @@ public void testPartialReadTruncatedData() throws IOException {
buffer = ByteBuffer.allocate(1);
try {
bz2Channel.read(buffer);
- Assert.fail("The read should have thrown.");
+ fail("The read should have thrown.");
Review Comment:
assertThrows here too, I think.
##########
src/test/java/org/apache/commons/compress/compressors/snappy/FramedSnappyCompressorInputStreamTest.java:
##########
@@ -184,14 +184,11 @@ public void testUnskippableChunk() {
(byte) 0xff, 6, 0, 0, 's', 'N', 'a', 'P', 'p', 'Y',
2, 2, 0, 0, 1, 1
};
- try {
- final FramedSnappyCompressorInputStream in =
- new FramedSnappyCompressorInputStream(new ByteArrayInputStream(input));
- in.read();
- fail("expected an exception");
- in.close();
+ try (final FramedSnappyCompressorInputStream in =
+ new FramedSnappyCompressorInputStream(new ByteArrayInputStream(input))) {
+ IOException exception = assertThrows(IOException.class, () -> in.read());
Review Comment:
:+1: you've already replaced that `fail` by an `assertThrows` here. We should probably change it in the other places too.
##########
src/test/java/org/apache/commons/compress/utils/MultiReadOnlySeekableByteChannelTest.java:
##########
@@ -139,44 +143,44 @@ private void check(final byte[] expected, final SeekableByteChannel channel, fin
// If this isn't the last read() then we expect the buf
// ByteBuffer to be full (i.e. have no remaining)
if (resultBuffer.position() < expected.length) {
- Assert.assertEquals("readBufferSize " + readBufferSize, 0, remaining);
+ assertEquals(0, remaining, "readBufferSize " + readBufferSize);
}
if (bytesRead == -1) {
- Assert.assertEquals("readBufferSize " + readBufferSize, 0, buf.position());
+ assertEquals(0, buf.position(), "readBufferSize " + readBufferSize);
} else {
- Assert.assertEquals("readBufferSize " + readBufferSize, bytesRead, buf.position());
+ assertEquals(bytesRead, buf.position(), "readBufferSize " + readBufferSize);
}
}
resultBuffer.flip();
final byte[] arr = new byte[resultBuffer.remaining()];
resultBuffer.get(arr);
- Assert.assertArrayEquals("readBufferSize " + readBufferSize, expected, arr);
+ assertArrayEquals(expected, arr, "readBufferSize " + readBufferSize);
}
private void checkEmpty(final SeekableByteChannel channel) throws IOException {
final ByteBuffer buf = ByteBuffer.allocate(10);
- Assert.assertTrue(channel.isOpen());
- Assert.assertEquals(0, channel.size());
- Assert.assertEquals(0, channel.position());
- Assert.assertEquals(-1, channel.read(buf));
+ assertTrue(channel.isOpen());
+ assertEquals(0, channel.size());
+ assertEquals(0, channel.position());
+ assertEquals(-1, channel.read(buf));
channel.position(5);
- Assert.assertEquals(-1, channel.read(buf));
+ assertEquals(-1, channel.read(buf));
channel.close();
- Assert.assertFalse(channel.isOpen());
+ assertFalse(channel.isOpen());
try {
channel.read(buf);
- Assert.fail("expected a ClosedChannelException");
+ fail("expected a ClosedChannelException");
} catch (final ClosedChannelException expected) {
}
try {
channel.position(100);
- Assert.fail("expected a ClosedChannelException");
+ fail("expected a ClosedChannelException");
Review Comment:
assertThrows :point_up:
##########
src/test/java/org/apache/commons/compress/utils/FixedLengthBlockOutputStreamTest.java:
##########
@@ -208,8 +208,7 @@ public void testPartialWritingThrowsException() {
fail("Exception for partial write not thrown");
} catch (final IOException e) {
final String msg = e.getMessage();
- assertEquals("exception message",
- "Failed to write 512 bytes atomically. Only wrote 511", msg);
+ assertEquals("Failed to write 512 bytes atomically. Only wrote 511", msg, "exception message");
Review Comment:
I think this can be rewritten for an assertThrows here too
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #346: Use JUnit 5 assertions in JUnit 5 tests instead of JUnit 4 classes
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #346:
URL: https://github.com/apache/commons-compress/pull/346#discussion_r1059140981
##########
src/test/java/org/apache/commons/compress/archivers/tar/SparseFilesTest.java:
##########
@@ -119,9 +120,8 @@ private String getTarBinaryHelp() throws IOException {
}
@Test
+ @DisabledOnOs(OS.WINDOWS)
Review Comment:
Why would we disable a test that is passing?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] kinow commented on pull request #346: Use JUnit 5 assertions in JUnit 5 tests instead of JUnit 4 classes
Posted by GitBox <gi...@apache.org>.
kinow commented on PR #346:
URL: https://github.com/apache/commons-compress/pull/346#issuecomment-1368082743
Leaving unmerged for a while so others can review it too. We just need to squash and merge it if there's nothing wrong. Maybe a JIRA to credit in the changelog too, such a big task :+1: But optional. Thanks!
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] garydgregory commented on pull request #346: Use JUnit 5 assertions in JUnit 5 tests instead of JUnit 4 classes
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #346:
URL: https://github.com/apache/commons-compress/pull/346#issuecomment-1368447105
Patch applied locally to resolve issues. 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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on a diff in pull request #346: Use JUnit 5 assertions in JUnit 5 tests instead of JUnit 4 classes
Posted by GitBox <gi...@apache.org>.
theobisproject commented on code in PR #346:
URL: https://github.com/apache/commons-compress/pull/346#discussion_r1059408643
##########
src/test/java/org/apache/commons/compress/archivers/TarTestCase.java:
##########
@@ -400,7 +400,7 @@ public void testTarFileCOMPRESS178() throws Exception {
fail("Expected IOException");
} catch (final IOException e) {
final Throwable t = e.getCause();
- assertTrue("Expected cause = IllegalArgumentException", t instanceof IllegalArgumentException);
+ assertTrue(t instanceof IllegalArgumentException, "Expected cause = IllegalArgumentException");
Review Comment:
I've gone through all tests and replaced the try/catch/fail pattern with assertThrows
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] theobisproject commented on a diff in pull request #346: Use JUnit 5 assertions in JUnit 5 tests instead of JUnit 4 classes
Posted by GitBox <gi...@apache.org>.
theobisproject commented on code in PR #346:
URL: https://github.com/apache/commons-compress/pull/346#discussion_r1059409087
##########
src/test/java/org/apache/commons/compress/archivers/tar/SparseFilesTest.java:
##########
@@ -119,9 +120,8 @@ private String getTarBinaryHelp() throws IOException {
}
@Test
+ @DisabledOnOs(OS.WINDOWS)
Review Comment:
As @kinow said those tests where already disabled by the assume statements in the tests. Would be nice if you can double check that I didn't mess up the logic 😉
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] kinow commented on a diff in pull request #346: Use JUnit 5 assertions in JUnit 5 tests instead of JUnit 4 classes
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #346:
URL: https://github.com/apache/commons-compress/pull/346#discussion_r1059143127
##########
src/test/java/org/apache/commons/compress/archivers/tar/SparseFilesTest.java:
##########
@@ -119,9 +120,8 @@ private String getTarBinaryHelp() throws IOException {
}
@Test
+ @DisabledOnOs(OS.WINDOWS)
Review Comment:
It's disabled on Windows. The old test was already disabled on windows, and had this line removed (not easy to see as the annotation is added at the top, but this assumption is within the method body :+1):
```java
assumeFalse("This test should be ignored on Windows", isOnWindows);
```
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] kinow commented on a diff in pull request #346: Use JUnit 5 assertions in JUnit 5 tests instead of JUnit 4 classes
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #346:
URL: https://github.com/apache/commons-compress/pull/346#discussion_r1059143127
##########
src/test/java/org/apache/commons/compress/archivers/tar/SparseFilesTest.java:
##########
@@ -119,9 +120,8 @@ private String getTarBinaryHelp() throws IOException {
}
@Test
+ @DisabledOnOs(OS.WINDOWS)
Review Comment:
It's disabled on Windows. The old test had this line removed:
```java
assumeFalse("This test should be ignored on Windows", isOnWindows);
```
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] codecov-commenter commented on pull request #346: Use JUnit 5 assertions in JUnit 5 tests instead of JUnit 4 classes
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #346:
URL: https://github.com/apache/commons-compress/pull/346#issuecomment-1367451029
# [Codecov](https://codecov.io/gh/apache/commons-compress/pull/346?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#346](https://codecov.io/gh/apache/commons-compress/pull/346?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f1cccc4) into [master](https://codecov.io/gh/apache/commons-compress/commit/9045a2d1e687492f6f6e28e378a60f43f90a8e48?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9045a2d) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #346 +/- ##
=========================================
Coverage 80.36% 80.36%
Complexity 6653 6653
=========================================
Files 342 342
Lines 25198 25198
Branches 4081 4081
=========================================
Hits 20251 20251
Misses 3368 3368
Partials 1579 1579
```
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org