Skip to content
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

Merged
merged 8 commits into from
Oct 10, 2023

Conversation

mureinik
Copy link
Contributor

@mureinik mureinik commented Oct 8, 2023

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:

  1. Gradle dependencies:

    1. All the dependencies under org.junit.jupiter were consolidated to use the single artifact org.junit.jupiter:junit-jupiter.
    2. The junit:junit dependency was removed in favor of org.junit.jupiter:junit-jupiter mentioned in 1.i.
    3. The Mockito dependencies org.mockito:mockito-core and org.mockito:mockito-inline duplications were cleaned up, and each module was left only with the relevant dependency.
    4. The org.mockito:mockito-junit-jupiter dependency was added to provide the integration with Mockito.
    5. The org.powermock:powermock-module-junit4 dependency was removed.
    6. The org.powermock:powermock-api-mockito2 dependency was removed.
    7. 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.ix.
    8. The "org.apache.rocketmq:rocketmq-test dependency was never really used, and was removed
  2. Annotations

    1. org.junit.jupiter.api.BeforeEach was used as a drop-in replacement for org.junit.Before.
    2. org.junit.jupiter.api.BeforeAll was used as a drop-in replacement for org.junit.BeforeClass.
    3. org.junit.jupiter.api.AfterEach was used as a drop-in replacement for org.junit.After.
    4. org.junit.jupiter.api.AfterAll was used as a drop-in replacement for org.junit.AfterClass.
    5. org.junit.jupiter.api.AfterEach was used as a drop-in replacement for org.junit.After.
    6. org.junit.jupiter.api.Disabled was used as a drop-in replacement for org.junit.Ignore.
    7. 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.i. below.
    8. 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.
      1. 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.
      2. org.mockito.junit.jupiter.MockitoExtension was used to replace usages of org.powermock.modules.junit4.PowerMockRunner and its associated annotations.
    9. 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

    1. 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.
    2. 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.
    3. 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.
    4. 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.
    5. org.junit.jupiter.api.Assertions' assertThrows was used to assert an expected exception is thrown instead of using org.junit.Test with the expected annotation.
  4. Mocking

    1. Usages of org.powermock.api.mockito.PowerMockito were deemed to be unnescary, and replaced with straight-forward usages of org.mockito.Mockito.
    2. Usages of org.powermock.reflect.Whitebox were cleaned, and the org.mockito.InjectMocks was used instead in order to inject mocks to the object under test.

Documentation

  • Does this pull request introduce a new feature? no

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.
Copy link
Contributor

@github-actions github-actions bot left a 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
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #4475 (f7381a7) into master (c8b7916) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head f7381a7 differs from pull request most recent head 0a327cf. Consider uploading reports for the commit 0a327cf to get more accurate results

@@             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

Copy link
Member

@mxsm mxsm left a 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

@Alonexc
Copy link
Contributor

Alonexc commented Oct 9, 2023

Need to create an issue for this pr and link to this pr.

Copy link
Member

@Pil0tXia Pil0tXia left a 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.

@Pil0tXia
Copy link
Member

Pil0tXia commented Oct 9, 2023

image

Please update dependency in tools/dependency-check/known-dependencies.txt.

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.
@mureinik
Copy link
Contributor Author

mureinik commented Oct 9, 2023

image

Please update dependency in tools/dependency-check/known-dependencies.txt.

@Pil0tXia once the explicit dependency on junit:junit was removed, the runtimeClasspath that this check depends on picked up all sorts of transitive dependencies that TBH shouldn't have been there in the first place (i.e., they were either completely redundant or should have been test dependencies).
I've pushed an updated version of this PR, that I think fixes the issue.

@Pil0tXia / @mxsm does the PR need to be re-approved in order for the CI to run again?

@mureinik mureinik changed the title Upgrade testing framework to JUnit 5 Jupiter [ISSUE #4478] Upgrade testing framework to JUnit 5 Jupiter Oct 9, 2023
@mureinik
Copy link
Contributor Author

mureinik commented Oct 9, 2023

LGTM. Please create a corresponding issue and standardize your PR title and Fixes.

@Pil0tXia done.

@pandaapo pandaapo changed the title [ISSUE #4478] Upgrade testing framework to JUnit 5 Jupiter [ISSUE #4478] Upgrade JUnit to JUnit Jupiter Oct 10, 2023
Copy link
Member

@mxsm mxsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mxsm mxsm merged commit 7508251 into apache:master Oct 10, 2023
6 checks passed
@mureinik mureinik deleted the junit-jupiter branch October 10, 2023 10:46
@pandaapo pandaapo added this to the 1.10 milestone Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Upgrade JUnit to JUnit Jupiter
6 participants