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/26 13:43:49 UTC
[GitHub] [commons-compress] theobisproject opened a new pull request, #344: Replace JUnit 3 with JUnit 5
theobisproject opened a new pull request, #344:
URL: https://github.com/apache/commons-compress/pull/344
Most tests are replaced with simple JUnit 5 tests. A small number of tests were migrated to parameterized tests.
--
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 pull request #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
theobisproject commented on PR #344:
URL: https://github.com/apache/commons-compress/pull/344#issuecomment-1365219087
@garydgregory I will go through the classes and do some more migrations. Is there anything else you would like to have changed (like using try-with-resources, ...)?
--
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 #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #344:
URL: https://github.com/apache/commons-compress/pull/344#discussion_r1057246093
##########
src/test/java/org/apache/commons/compress/archivers/LongPathTest.java:
##########
@@ -142,7 +140,7 @@ public void testArchive(final File file) throws Exception {
}
try {
checkArchiveContent(ais, expected);
- } catch (final AssertionFailedError e) {
+ } catch (final Exception e) {
Review Comment:
-1: Porting to JUnit 5 means using `Assertions.assertThrows()`, `Assertions.doesNotThrow()`, and so on, not whatever this is.
--
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 pull request #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
theobisproject commented on PR #344:
URL: https://github.com/apache/commons-compress/pull/344#issuecomment-1365946949
I have removed the visibility change of the classes and methods from the commits.
--
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 #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #344:
URL: https://github.com/apache/commons-compress/pull/344#discussion_r1057339000
##########
src/test/java/org/apache/commons/compress/harmony/pack200/tests/ArchiveTest.java:
##########
@@ -25,20 +28,27 @@
import java.io.InputStreamReader;
import java.io.OutputStream;
import java.net.URISyntaxException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
import java.util.Enumeration;
import java.util.jar.JarEntry;
import java.util.jar.JarFile;
import java.util.jar.JarInputStream;
import java.util.jar.JarOutputStream;
+import java.util.stream.Stream;
import org.apache.commons.compress.harmony.pack200.Archive;
import org.apache.commons.compress.harmony.pack200.Pack200Exception;
import org.apache.commons.compress.harmony.pack200.PackingOptions;
import org.apache.commons.compress.harmony.unpack200.Segment;
-import junit.framework.TestCase;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
-public class ArchiveTest extends TestCase {
+class ArchiveTest {
Review Comment:
There is no need to change the visibility of elements, it just overloads the PR with noise for no gain. Yes, I know that JUnit 5 like this style, but that is not the style we use in most of not all of Conmons components. It just make the PR harder and longer to review. What matters is the use of the Junit API, not the style IMO. For my money, the fact that a test is public tells me it's important, not some internal gadget, so let's not change access unless required (which should not happen).
--
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 #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #344:
URL: https://github.com/apache/commons-compress/pull/344#issuecomment-1365207025
Hi @theobisproject
Thank you for pitching in!
No matter which way you slice it, it's going to be a large PR. The only way I can think of making it smaller is by doing it one package at a time. Whichever way you go, I'd rather see it all done the "JUnit 5 way" rather than something in between.
--
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 merged pull request #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
garydgregory merged PR #344:
URL: https://github.com/apache/commons-compress/pull/344
--
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 #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
theobisproject commented on code in PR #344:
URL: https://github.com/apache/commons-compress/pull/344#discussion_r1057249368
##########
src/test/java/org/apache/commons/compress/archivers/LongPathTest.java:
##########
@@ -142,7 +140,7 @@ public void testArchive(final File file) throws Exception {
}
try {
checkArchiveContent(ais, expected);
- } catch (final AssertionFailedError e) {
+ } catch (final Exception e) {
Review Comment:
I'm with you on this one (and the one in the LongSymLinkTest). Just did not want to do too much in one go.
If I understand the test correctly we can get rid of the try-catch block completely since we do not want the check to throw an exception.
--
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 #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #344:
URL: https://github.com/apache/commons-compress/pull/344#discussion_r1057339000
##########
src/test/java/org/apache/commons/compress/harmony/pack200/tests/ArchiveTest.java:
##########
@@ -25,20 +28,27 @@
import java.io.InputStreamReader;
import java.io.OutputStream;
import java.net.URISyntaxException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
import java.util.Enumeration;
import java.util.jar.JarEntry;
import java.util.jar.JarFile;
import java.util.jar.JarInputStream;
import java.util.jar.JarOutputStream;
+import java.util.stream.Stream;
import org.apache.commons.compress.harmony.pack200.Archive;
import org.apache.commons.compress.harmony.pack200.Pack200Exception;
import org.apache.commons.compress.harmony.pack200.PackingOptions;
import org.apache.commons.compress.harmony.unpack200.Segment;
-import junit.framework.TestCase;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
-public class ArchiveTest extends TestCase {
+class ArchiveTest {
Review Comment:
There is no need to change the visibility of elements, it just overloads the PR with noise for no gain. Yes, I know that JUnit 5 like this style, but that is not the style we use in most if not all of Conmons components. It just make the PR harder and longer to review. What matters is the use of the Junit API, not the style IMO. For my money, the fact that a test is public tells me it's important, not some internal gadget, so let's not change access unless required (which should not happen).
--
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 #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #344:
URL: https://github.com/apache/commons-compress/pull/344#discussion_r1057246093
##########
src/test/java/org/apache/commons/compress/archivers/LongPathTest.java:
##########
@@ -142,7 +140,7 @@ public void testArchive(final File file) throws Exception {
}
try {
checkArchiveContent(ais, expected);
- } catch (final AssertionFailedError e) {
+ } catch (final Exception e) {
Review Comment:
-1: Porting to JUnit 5 means using `Assert.assertThrows()`, not whatever this is.
--
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 pull request #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
theobisproject commented on PR #344:
URL: https://github.com/apache/commons-compress/pull/344#issuecomment-1365283815
I have replaced all the try-catch-assert patterns in the tests I migrated. Also more tests do now use the parameterized tests for execution.
--
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 #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #344:
URL: https://github.com/apache/commons-compress/pull/344#issuecomment-1365225577
Keep it focused on the one task please: Junit 5 😉
--
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 #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #344:
URL: https://github.com/apache/commons-compress/pull/344#issuecomment-1365200978
# [Codecov](https://codecov.io/gh/apache/commons-compress/pull/344?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 [#344](https://codecov.io/gh/apache/commons-compress/pull/344?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0bcbf32) into [master](https://codecov.io/gh/apache/commons-compress/commit/f6dadd24b4b20f46541110b0146ce8413430e873?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f6dadd2) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #344 +/- ##
=========================================
Coverage 80.33% 80.33%
Complexity 6653 6653
=========================================
Files 342 342
Lines 25232 25232
Branches 4085 4085
=========================================
Hits 20271 20271
Misses 3382 3382
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
[GitHub] [commons-compress] garydgregory commented on a diff in pull request #344: Replace JUnit 3 with JUnit 5
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #344:
URL: https://github.com/apache/commons-compress/pull/344#discussion_r1057679052
##########
src/test/java/org/apache/commons/compress/harmony/pack200/tests/ArchiveTest.java:
##########
@@ -111,7 +121,8 @@ public void testAlternativeConstructor() throws IOException, URISyntaxException,
out.close();
}
- public void testAnnotations() throws IOException, Pack200Exception,
+ @Test
+ void testAnnotations() throws IOException, Pack200Exception,
Review Comment:
See above.
##########
src/test/java/org/apache/commons/compress/harmony/pack200/tests/ArchiveTest.java:
##########
@@ -95,11 +104,12 @@ private void compareJarEntries(final JarFile jarFile, final JarFile jarFile2) {
String name = entry.getName();
JarEntry entry2 = jarFile2.getJarEntry(name);
- assertNotNull("Missing Entry: " + name, entry2);
+ assertNotNull(entry2, "Missing Entry: " + name);
}
}
- public void testAlternativeConstructor() throws IOException, URISyntaxException, Pack200Exception {
+ @Test
+ void testAlternativeConstructor() throws IOException, URISyntaxException, Pack200Exception {
Review Comment:
See above.
##########
src/test/java/org/apache/commons/compress/harmony/pack200/tests/ArchiveTest.java:
##########
@@ -140,7 +151,8 @@ public void testAnnotations() throws IOException, Pack200Exception,
compareFiles(jarFile, jarFile2);
}
- public void testAnnotations2() throws IOException, Pack200Exception,
+ @Test
+ void testAnnotations2() throws IOException, Pack200Exception,
Review Comment:
And so on, so I'll stop 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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org