-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Port tests from JUnit 3 to JUnit 4 #2294
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.
This is great, thanks! And thanks too for your patience.
I have added I don't simplify the asserts cause, as you said, we switch to Truth.
No problems, I’m here to help and learn 😄 |
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 a lot for these changes! There might be one small oversight (see review comment), but besides that this pull request has really good quality; based on the Maven test output it appears you did not overlook a single test. It still runs the same amount of tests (or actually a few more since those are now properly reported as 'skipped').
Out of curiousity, did you adjust these test classes by hand? I had a look at OpenRewrite, but it appears they only have a recipe for migrating from JUnit 4 to JUnit 5.
assertEquals(7.936508173034305E-4, values[3]); | ||
assertEquals(0.0011904761904761906, values[4]); | ||
assertEquals(0.0, values[5]); | ||
assertEquals(6, values.length, 0); |
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.
This looks incorrect / unnecessary since the values here are int
so there is no need to use assertEquals(double, double, double)
:
assertEquals(6, values.length, 0); | |
assertEquals(6, values.length); |
In case you do want to change this, maybe the following should also be changed to use assertEquals(float, float, float)
instead of the double
variant by appendingF
to 0.00001
:
assertEquals(1F, json.getAsFloat(), 0.00001); |
(though this is not an issue you introduced)
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.
Ohh yes, good point. 😄
If we don’t change all the assertions to Truth, the next PR will be "simplify assertions". I’ll fix it in that PR.
No, I used some regex to adjust classes, methods and imports. Then I ran the tests and corrected the remaining errors. Here you can find some regex that i have used: link(If you want to use it, the regexs need some small adjustments. It's not just a copy-paste) |
* Port tests from JUnit 3 to JUnit 4 * Port tests from JUnit 3 to JUnit 4 * Add `@Test` above `@Ignore`
I have port tests from JUnit 3 to JUnit 4