-
Notifications
You must be signed in to change notification settings - Fork 642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ISSUE #4478] Upgrade JUnit to JUnit Jupiter #4475
Conversation
Adding empty arguments adds no value, and will just interfere with future efforts to migrate to JUnit Jupiter.
PowerMockito is known to be slow and cumbersome compared to Mockito. In most cases, removing it was a straight forward change of removing the PowerMockito annotations and use the MockitoJUnitRunner. In the single non-trivial case, WebHookProcessorTest, PowerMockito's WhiteBox was removed and replaced with Mockito's @Injectmocks annotation.
mockito-inline is intended to replace mockito-core in cerain situations (e.g., mocking static or final classes), and there's no need to have them both as dependencies in the same module. This patch cleans up these duplications and leaves a "slimmer" dependency tree with just the required mockito dependency in each module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to the Apache EventMesh community!!
This is your first PR in our project. We're very excited to have you onboard contributing. Your contributions are greatly appreciated!
Please make sure that the changes are covered by tests.
We will be here shortly.
Let us know if you need any help!
Want to get closer to the community?
WeChat Assistant | WeChat Public Account | Slack |
---|---|---|
Join Slack Chat |
Mailing Lists:
Name | Description | Subscribe | Unsubscribe | Archive |
---|---|---|---|---|
Users | User support and questions mailing list | Subscribe | Unsubscribe | Mail Archives |
Development | Development related discussions | Subscribe | Unsubscribe | Mail Archives |
Commits | All commits to repositories | Subscribe | Unsubscribe | Mail Archives |
Issues | Issues or PRs comments and reviews | Subscribe | Unsubscribe | Mail Archives |
Codecov Report
@@ Coverage Diff @@
## master #4475 +/- ##
============================================
- Coverage 15.58% 15.57% -0.01%
+ Complexity 1475 1474 -1
============================================
Files 698 698
Lines 28205 28205
Branches 2633 2633
============================================
- Hits 4396 4394 -2
- Misses 23361 23362 +1
- Partials 448 449 +1 see 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mureinik Please fix CI
Need to create an issue for this pr and link to this pr. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please create a corresponding issue and standardize your PR title and Fixes
.
In its current form, the project has some JUnit 4 and some JUnit 5 (Jupiter) tests. This patch aims to align all the tests to a single modern framework, JUnit Jupiter, in order to make future development easier. As this patch is already pretty large as-is, it attempts to be non-opinionated and simply replace JUnit 4 calls with the closed JUnit Jupiter equivalents. Subsequent work may want to change some tests to take further advantage of JUnit Jupiter's features. This patch includes the following changes: 1. Gradle dependencies: a. All the dependencies under org.junit.jupiter were consolidated to use the single artifact org.junit.jupiter:junit-jupiter. b. The junit:junit dependency was removed in favor of org.junit.jupiter:junit-jupiter as mentioned in 1.a.. c. The org.mockito:mockito-junit-jupiter dependency was added to provide the integration with Mockito. d. The com.github.stefanbirkner:system-rules dependency was removed in favor of org.junit-pioneer:junit-pioneer that was used to provide the same functionality as mentioned in 2.i. 2. Annotations a. org.junit.jupiter.api.BeforeEach was used as a drop-in replacement for org.junit.Before. b. org.junit.jupiter.api.BeforeAll was used as a drop-in replacement for org.junit.BeforeClass. c. org.junit.jupiter.api.AfterEach was used as a drop-in replacement for org.junit.After. d. org.junit.jupiter.api.AfterAll was used as a drop-in replacement for org.junit.AfterClass. e. org.junit.jupiter.api.AfterEach was used as a drop-in replacement for org.junit.After. f. org.junit.jupiter.api.Disabled was used as a drop-in replacement for org.junit.Ignore. g. org.junit.jupiter.api.Test was used as a replacement for org.junit.Test, although with some caveats: 1. For the simple case with no arguments, org.junit.jupiter.api.Test was used as a drop-in replacement for org.junit.Test. 2. For the case where org.junit.Test was used with a timeout argument, a combination of org.junit.jupiter.api.Test and org.junit.jupiter.api.Timeout was used. 3. For the case where org.junit.Test was used with an expected argument, org.junit.jupiter.api.Test was used, but the assertion on the exception begin thrown is done explicitly in the test's code, see 3.e. below. h. org.junit.jupiter.api.extension.ExtendWith was used as a replacement for org.junit.runner.RunWith with an extension corresponding to the runner being replaced. a. org.mockito.junit.jupiter.MockitoExtension was used to provide the same functionality as org.mockito.junit.MockitoJUnitRunner. Since the extension is stricter than the runner, in some cases org.mockito.junit.jupiter.MockitoSettings was used to explicitly make the mocking more lenient. i. org.junitpioneer.jupiter.SetEnvironmentVariable was used in order to set environment variables in the tests instead of explicitly calling org.junit.contrib.java.lang.system.EnvironmentVariables in the test's body. As an added bonus, using this annotation also cleans up the changes to the environment variables when the test is over and prevents it from inadvertently effecting other tests. 3. Assertions a. org.junit.jupiter.api.Assertions was used as a drop-in replacement for org.junit.Assert for the case where the assertion was performed without a message. b. org.junit.jupiter.api.Assertions was used instead of org.junit.Assert in the cases where an assertion method was used with a message, but the argument were permuted to fit the new method's signature. c. org.junit.jupiter.api.Assertions does not have an equivalent of org.junit.Assert's assertThat method, but luckily it was only used in a few places, and always used the org.hamcrest.CoreMatchers.is matcher. These assertions were rewritten as straight-forward assertEquals assertions. c. org.junit.jupiter.api.Assertions does not have an equivalent of org.junit.Assert's assertThat method, but luckily it was only used in a few places, and always used the org.hamcrest.CoreMatchers.is matcher. These assertions were rewritten as straight-forward assertEquals assertions. d. org.junit.jupiter.api.Assertions does not have an equivalent of org.hamcrest.MatcherAssert's assertThat method, but luckily it was only used in a few places, and always used the org.hamcrest.CoreMatchers.is matcher. These assertions were rewritten as straight-forward assertEquals assertions. e. org.junit.jupiter.api.Assertions' assertThrows was used to assert an expected exception is throws instead of using org.junit.Test with the expected annotation.
b0969d7
to
0a327cf
Compare
@Pil0tXia once the explicit dependency on @Pil0tXia / @mxsm does the PR need to be re-approved in order for the CI to run again? |
@Pil0tXia done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #4478
Motivation
In its current form, the project has some JUnit 4 and some JUnit 5 (Jupiter) tests. This PR aims to align all the tests to a single modern framework, JUnit Jupiter, in order to make future development easier.
Modifications
As this PR is already pretty large as-is, it attempts to be non-opinionated and simply replace JUnit 4 calls with the closed JUnit Jupiter equivalents. Subsequent work may want to change some tests to take further advantage of JUnit Jupiter's features.
This PR includes the following changes:
Gradle dependencies:
org.junit.jupiter
were consolidated to use the single artifactorg.junit.jupiter:junit-jupiter
.junit:junit
dependency was removed in favor oforg.junit.jupiter:junit-jupiter
mentioned in 1.i.org.mockito:mockito-core
andorg.mockito:mockito-inline
duplications were cleaned up, and each module was left only with the relevant dependency.org.mockito:mockito-junit-jupiter
dependency was added to provide the integration with Mockito.org.powermock:powermock-module-junit4
dependency was removed.org.powermock:powermock-api-mockito2
dependency was removed.com.github.stefanbirkner:system-rules
dependency was removed in favor oforg.junit-pioneer:junit-pioneer
that was used to provide the same functionality as mentioned in 2.ix."org.apache.rocketmq:rocketmq-test
dependency was never really used, and was removedAnnotations
org.junit.jupiter.api.BeforeEach
was used as a drop-in replacement fororg.junit.Before
.org.junit.jupiter.api.BeforeAll
was used as a drop-in replacement fororg.junit.BeforeClass
.org.junit.jupiter.api.AfterEach
was used as a drop-in replacement fororg.junit.After
.org.junit.jupiter.api.AfterAll
was used as a drop-in replacement fororg.junit.AfterClass
.org.junit.jupiter.api.AfterEach
was used as a drop-in replacement fororg.junit.After
.org.junit.jupiter.api.Disabled
was used as a drop-in replacement fororg.junit.Ignore
.org.junit.jupiter.api.Test
was used as a replacement fororg.junit.Test
, although with some caveats:org.junit.jupiter.api.Test
was used as a drop-in replacement fororg.junit.Test
.org.junit.Test
was used with atimeout
argument, a combination oforg.junit.jupiter.api.Test
andorg.junit.jupiter.api.Timeout
was used.org.junit.Test
was used with anexpected
argument,org.junit.jupiter.api.Test
was used, but the assertion on the exception begin thrown is done explicitly in the test's code, see 3.i. below.org.junit.jupiter.api.extension.ExtendWith
was used as a replacement fororg.junit.runner.RunWith
with an extension corresponding to the runner being replaced.org.mockito.junit.jupiter.MockitoExtension
was used to provide the same functionality asorg.mockito.junit.MockitoJUnitRunner
. Since the extension is stricter than the runner, in some casesorg.mockito.junit.jupiter.MockitoSettings
was used to explicitly make the mocking more lenient.org.mockito.junit.jupiter.MockitoExtension
was used to replace usages oforg.powermock.modules.junit4.PowerMockRunner
and its associated annotations.org.junitpioneer.jupiter.SetEnvironmentVariable
was used in order to set environment variables in the tests instead ofexplicitly calling
org.junit.contrib.java.lang.system.EnvironmentVariables
in the test's body. As an added bonus, using this annotation also cleans up the changes to the environment variables when the test is over and prevents it from inadvertently effecting other tests.Assertions
org.junit.jupiter.api.Assertions
was used as a drop-in replacement fororg.junit.Assert
for the case where the assertion was performed without a message.org.junit.jupiter.api.Assertions
was used instead oforg.junit.Assert
in the cases where an assertion method was used with a message, but the argument were permuted to fit the new method's signature.org.junit.jupiter.api.Assertions
does not have an equivalent oforg.junit.Asser
t'sassertThat
method, but luckily it was only used in a few places, and always used theorg.hamcrest.CoreMatchers.is
matcher. These assertions were rewritten as straight-forwardassertEquals
assertions.org.junit.jupiter.api.Assertions
does not have an equivalent oforg.hamcrest.MatcherAssert
'sassertThat
method, but luckily it was only used in a few places, and always used theorg.hamcrest.CoreMatchers.is
matcher. These assertions were rewritten as straight-forwardassertEquals
assertions.org.junit.jupiter.api.Assertions
'assertThrows
was used to assert an expected exception is thrown instead of usingorg.junit.Test
with theexpected
annotation.Mocking
org.powermock.api.mockito.PowerMockito
were deemed to be unnescary, and replaced with straight-forward usages oforg.mockito.Mockito
.org.powermock.reflect.Whitebox
were cleaned, and theorg.mockito.InjectMocks
was used instead in order to inject mocks to the object under test.Documentation