jmrozanec / cron-utils

Cron utils for parsing, validations and human readable descriptions as well as date/time interoperability.
http://cron-utils.com
Apache License 2.0
1.15k stars 262 forks source link

Standardize tests to use JUnit Jupiter #541

Closed mureinik closed 2 years ago

mureinik commented 2 years ago

The project currently uses a mixture of JUnit 4 and JUnit Jupiter which while it isn't a problem per-se is confusing at best and may cause sublte bugs at worst. This PR standardizes the entire project to use the newer JUnit Jupiter in order to make it easier to maintain.

It includes the following changes:

  1. Maven
    1. Replaced the org.junit.jupiter:junit-jupiter-api:5.8.1 dependency with the broader org.junit.jupiter:junit-jupiter:5.8.1 dependency. This dependency also includes the classes needed for writing parameterized tests in JUnit Jupiter, as described in 2.iii.
    2. . Removed the pl.pragmatists:JUnitParams:1.1.1 dependency which is no longer used. Note that the project does not contain an explicit dependency on org.junit:junit:4.12 with provides the JUnit 4 classes, but it was transitively available via this dependency.
    3. Added an explicit dependency on org.mockito:mockito-inline:3.9.0 in order to use Mockito's inline mock maker's capabilities.
    4. Removed the org.powermock:powermock-module-junit4:2.0.9 dependency which is no longer used, see 6.i and 6.ii.
    5. Removed the org.powermock:powermock-api-mockito2:2.0.9 dependency which is no longer used, see 6.i and 6.ii.
    6. Removed the org.mockito:mockito-junit-jupiter:3.9.0 dependency. The classes provided by this dependency were never used, and it was only useful for getting the indirect dependency org.mockito:mockito-all:3.9.0. With the explicit dependency described in 1.iii, this is no longer required.
  2. Annotations
    1. org.junit.jupiter.api.Test is used as a drop-in replacement for org.junit.Test in the simple case where it was used without any additional parameters.
    2. org.junit.jupiter.api.Test is used instead of org.junit.Test with the expected argument, although in this case the assertions needed to be changed too, see 5.iv.
    3. org.junit.jupiter.params.ParameterizedTest is used instead of org.junit.Test in the cases where the test was parameterized, although additional changes were required, see 4.i and 4.ii. In the simple case the parameters were strings an org.junit.jupiter.params.provider.ValueSource annotation was added, and in other cases an org,junit.Jupiter.params.provider.MethodSource was used.
    4. org.junit.jupiter.api.BeforeEach is used as drop-in replacement for org.junit.Before.
    5. org.junit.jupiter.api.Disabled is used as drop-in replacement for org.junit.Ignore.
    6. Usages of the org.powermock.core.classloader.annotations.PrepareForTest annotation were removed, see 6.i and 6.ii for details.
  3. Rules
    1. JUnit Jupiter does not support Rule annotations. The existing ExpectedException rules were removed and replaced with other mechanisms, see 5.iv.
  4. Runners
    1. The usage of org.junit.runner.Parameterized were removed and the tests were rewritten to use the ParameterizedTest annotation, see 2.iii.
    2. The usage of junitparams.JUnitParamsRunner was removed and the tests were rewritten to use the ParameterizedTest annotation, see 2.iii.
  5. Assertions
    1. org.junit.jupiter.api.Assertions' methods are used as drop-in replacements for the methods with the same name from org.junit.Assert in the simple case the method was called without the optional message argument.
    2. org.junit.jupiter.api.Assertions' methods are used as replacements for org.junit.Assert's methods with the same name also in the case where a message argument was used, but in this case the arguments needed to be permuted and the message argument moved from the being the first argument to being the last argument.
    3. org.junit.jupiter.api.Assertions' methods are used as drop-in replacements for the methods with the same name from junit.framework.Assert.
    4. org.junit.jupiter.api.Assertions' methods are used as drop-in replacements for the methods with the same name from junit.framework.TestCase.
    5. org.junit.jupiter.api.Assertions does not have an equivalent of org.junit.Assert's assertThat method. In the cases it was used, the assertion was rewritten to use assertTrue or assertEquals where possible.
    6. org.junit.jupiter.api.Assertions' assertThrows method was used to rewrite tests that used the org.junit.Test annotation with the expected argument (see 2.ii). As a side bonus, using this method allowed writing a stricter test where the exception must be thrown from the expected call instead of some arbitrary statement in the test. This, in turn, uncovered some dead code in those methods that could be removed.
    7. org.junit.jupiter.api.Assertions' assertThrows method was used to rewrite tests that used the ExpectedException Rule (see 3.iii).
  6. Mocking
    1. Usages of org.powermock.api.mockito.PowerMockito's mockStatic were replaced with equivalent calls to org.mockito.Mockito' mockStatic.
    2. Usages of org.powermock.api.mockito.PowerMockito's whenNew were replaced with equivalent calls to org.mockito.Mockito' mockConstruction.
    3. New tearDown methods annotated with org.junit.jupiter.api.AfterEach were added to close the MockedStatic and MockedConstruction instances created from the changes in 6.i and 6.ii.
  7. Code cleanups
    1. In order to make the work on this PR easier, imports of org.junit.Assert were replaced with static imports of the relevant methods, as per what seems to be the defacto standard for this project.
    2. Unused imports in test files were removed.
  8. Specific test fixes
    1. BetweenFieldValueGeneratorTest's testGenerateNextValue and testGeneratePreviousValue did not fail, but they were subtly wrong. In both tests, the loops have one iteration too many, meaning the last iteration of the loop will throw the expected NoSuchValueException, and the statement after the loop will never be executed.
    2. CompositeCronTest's weDoNotSupportCronsWithDifferentDefinitions didn't fail, but it is was subtly wrong. The statement parser2.parse("0 0 1 * *") throws an IllegalArgumentException (causing the test to pass), and by doing so the call to the CompositeCron's constructor which is what this method is supposed to test is never called.
    3. Issue499Test was never run - it used org.junit.Test, but the class is not public, so it's never picked up by the test runner. It was converted to use org.junit.jupiter.api.Test with assertThrows, and the dead code that tested the behavior before #499 was fixed was removed.

Individual commit messages contain additional details and context not described here.

jmrozanec commented 2 years ago

@mureinik thanks! We will review the changes and merge soon. Best!

jmrozanec commented 2 years ago

@mureinik, many thanks for the enhancements! 😄 🚀