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

Migrate tests to JUnit5 #14922

Merged
merged 2 commits into from
Nov 23, 2024
Merged

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

@strangelookingnerd strangelookingnerd force-pushed the migrate_to_junit5 branch 2 times, most recently from e7de4d4 to 757dd4b Compare November 21, 2024 11:36
@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
18 checks passed
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants