-
Notifications
You must be signed in to change notification settings - Fork 426
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
RATIS-1970. Add junit 5 dependencies in ratis-common. #993
Conversation
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.
Thanks @nandakumar131 for the patch, LGTM.
@nandakumar131 , thanks a lot for working on JUnit5 migration! For the dependencies, let's add them when they are needed (i.e. add them with the tests). We minimize the dependencies because Ratis is a library. We should make it easier to be consumed. For JUnit5 migration, we may start with ratis-common since it only has a few tests. It can show how the migration works. |
Thanks for the review @adoroszlai & @szetszwo. @szetszwo, sure. will do. FYI: The scope of the junit dependencies are of scope |
@szetszwo, I have been working on limiting the changes just to I will try to limit the changes on each PR to make the review process easier. This PR will only update the parent Test-case changes will happen in follow-up PRs. |
@nandakumar131 , I understand that the test dependencies are not transitive. That's a good point! It is usually hard to remove dependencies. So I really oppose adding unused dependencies. Moreover, we have to handle it in case of CVEs. Please include some change to use the dependencies in this pull requests. |
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.
+1 the change looks good.
What changes were proposed in this pull request?
Junit 5 dependencies are added to the appropriate pom files.
What is the link to the Apache JIRA
RATIS-1970
How was this patch tested?
Existing unit tests.