Replace JUnit 3 with JUnit 5#344
Conversation
Codecov Report
@@ 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 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| try { | ||
| checkArchiveContent(ais, expected); | ||
| } catch (final AssertionFailedError e) { | ||
| } catch (final Exception e) { |
There was a problem hiding this comment.
-1: Porting to JUnit 5 means using Assertions.assertThrows(), Assertions.doesNotThrow(), and so on, not whatever this is.
There was a problem hiding this 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.
|
Hi @theobisproject |
|
@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, ...)? |
|
Keep it focused on the one task please: Junit 5 😉 |
|
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. |
| import org.junit.jupiter.params.provider.MethodSource; | ||
|
|
||
| public class ArchiveTest extends TestCase { | ||
| class ArchiveTest { |
There was a problem hiding this 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).
|
|
||
| public void testAlternativeConstructor() throws IOException, URISyntaxException, Pack200Exception { | ||
| @Test | ||
| void testAlternativeConstructor() throws IOException, URISyntaxException, Pack200Exception { |
|
|
||
| public void testAnnotations() throws IOException, Pack200Exception, | ||
| @Test | ||
| void testAnnotations() throws IOException, Pack200Exception, |
|
|
||
| public void testAnnotations2() throws IOException, Pack200Exception, | ||
| @Test | ||
| void testAnnotations2() throws IOException, Pack200Exception, |
There was a problem hiding this comment.
And so on, so I'll stop here.
Most tests are replaced with simple JUnit 5 tests. A small number of tests were migrated to parameterized tests.
- Replace try-catch-assert with assertThrows/assertDoesNotThrow - Migrate more tests to use the parameterized test execution
theobisproject
force-pushed
the
replace-junit3
branch
from
ad1bf22 to
c202c30
Compare
|
I have removed the visibility change of the classes and methods from the commits. |
3 participants
Most tests are replaced with simple JUnit 5 tests. A small number of tests were migrated to parameterized tests.