-
Notifications
You must be signed in to change notification settings - Fork 477
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
5634 new unit tests #5643
5634 new unit tests #5643
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.
Overall good, one minor request! Thanks for adding coverage!
import org.junit.runner.RunWith; | ||
|
||
@RunWith(Theories.class) | ||
public class MyDataUtilTest { |
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.
I hadn't seen JUnit theories before. They look really powerful and I like how you've used them here. They are experimental though and it looks like JUnit 5 is steering away from this sort of functionality junit-team/junit5#1422 (comment)
I'm ok with accepting this test. Can you leave a comment in the code about this experimentally for us to refer to when we decide to migrate tests? Also maybe a line about Theories in general?
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 for the review. I added a comment about JUnit Theories in a new commit.
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.
Could this be replaced with the upstream-supported Dynamic Testing in JUnit5?
Promising blog article on that: https://blog.codefx.org/libraries/junit-5-dynamic-tests/
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.
@poikilotherm This does look really useful! I don't think I'll ask for it in this PR but if we want more dynamic tests this is the way to go.
Hi @nikzaugg - is this ready for Code Review? |
Hi @djbrooke |
Should a new issue be created since #5634 is already closed? |
#5643 is good and ready for QA |
@pdurbin I'm OK with this moving as a PR. Thanks @matthew-a-dunlap for the code review! |
This pull request contributes three new test classes with unit tests (as mentioned in #5634 ).
Related Issues
Pull Request Checklist