Skip to content

Migrate tests to JUnit5#14922

Merged
CrazyHZM merged 2 commits intoapache:3.3from
strangelookingnerd:migrate_to_junit5
Nov 23, 2024
Merged

Migrate tests to JUnit5#14922
CrazyHZM merged 2 commits intoapache:3.3from
strangelookingnerd:migrate_to_junit5

Conversation

@strangelookingnerd
Copy link
Contributor

Most of the tests already use JUnit5. This PR will migrate the missing tests and remove the junit-jupiter-vintage as well as junit dependencies.

Migration includes:

  • Using JUnit5 annotations
  • Using Assertions
  • Remove public modifiers from test classes and methods
  • Clean up assertions (switching expected and actual, simplyifing assertions...)
  • Trivial code cleanup

I validated my changes using ./build -t.

Checklist

  • Make sure there is a GitHub_issue field for the change.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Make sure gitHub actions can pass. Why the workflow is failing and how to fix it?
@wcy666103
Copy link
Contributor

LGTM

@wcy666103
Copy link
Contributor

Test locally first.

image

@strangelookingnerd
Copy link
Contributor Author

strangelookingnerd commented Nov 21, 2024

Test locally first.

Seems like I missed that one, sorry for that. However to me it seems like this test should have failed without my changes as well.

I noticed that there is an exact copy of the failing test which has the assertion that is causing the issue here is commented out

Comparing
dubbo-spring-boot/src/test/java/org/apache/dubbo/spring/boot/env/DubboDefaultPropertiesEnvironmentPostProcessorTest.java

dubbo-spring-boot-autoconfigure/src/test/java/org/apache/dubbo/spring/boot/env/DubboDefaultPropertiesEnvironmentPostProcessorTest


Edit:

I just confirmed that the test that is failing was simply not executed before my changes. Digging a little deeper I found #12861 which suggests that the property that is checked here caused some kind of issue. I therefore propose to give the test the same treatment as in dubbo-spring-boot and comment the assertion out.

Copy link
Member

@CrazyHZM CrazyHZM left a comment

Choose a reason for hiding this comment

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

LGTM

@CrazyHZM CrazyHZM merged commit 9e249cf into apache:3.3 Nov 23, 2024
@strangelookingnerd strangelookingnerd deleted the migrate_to_junit5 branch November 23, 2024 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants