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

Use JUnit 5 instead of JUnit 4 #473

Merged

Conversation

obfischer
Copy link
Contributor

@obfischer obfischer commented Jul 18, 2021

Updated the dependencies to use JUnit 5 instead of JUnit 4. The usage of the JUnit Vintage plattform ensures, that the existing test can be executed without any changes.

Fixed #472

@obfischer obfischer changed the title Fixes #472 Use JUnit 5 instead of JUnit 4 Use JUnit 5 instead of JUnit 4 Jul 18, 2021
@obfischer obfischer force-pushed the improvement/472-update-to-junit-5 branch from 31d4e3c to 25040fd Compare July 18, 2021 11:25
pom.xml Outdated Show resolved Hide resolved
@olamy
Copy link
Member

olamy commented Jul 19, 2021

what is the point if you do not migrate code as well?

Version 64 of the parent POM will provide the dependency information
for JUnit 5. Therefore the entry in the dependency management section
can be removed if version 64 will be released.
@obfischer obfischer force-pushed the improvement/472-update-to-junit-5 branch from 25040fd to da37339 Compare July 19, 2021 10:12
@obfischer
Copy link
Contributor Author

what is the point if you do migrate code as well?

I will do it in the one of the next commits. I prefer to do smaller contributions as I can not guarantee when I will be able to continue my work. I think even having the possibility to use JUni 5 will help new contributors. WDYT?

@bmarwell
Copy link
Contributor

what is the point if you do migrate code as well?

I will do it in the one of the next commits. I prefer to do smaller contributions as I can not guarantee when I will be able to continue my work. I think even having the possibility to use JUni 5 will help new contributors. WDYT?

Sounds good to me! @olamy I wouldn't be too strict here. Besides, I really think this is a valid and really, really good point.

@bmarwell bmarwell requested a review from olamy July 19, 2021 18:59
@olamy
Copy link
Member

olamy commented Jul 21, 2021

@bmarwell @obfischer I just wonder if we add this dependencies with still old junit dependencies available, contributors may start contributing code with junit5.
How surefire works when you mix junit4 with junit5?

@obfischer
Copy link
Contributor Author

@olamy It depends on the version of Surefire. The one currently used runs them via JUnit 5. JUnit 5 itself provides the support via a dedicated test engine to run also JUni 4 tests.

I checked this and Surefire runs the same number of tests after the change as before it.

@bmarwell
Copy link
Contributor

Yeah, the Junit5 vintage engine is a drop in replacement. Just make sure we do not downgrade surefire. 😉 I've been using this setup for ages.

@olamy
Copy link
Member

olamy commented Jul 23, 2021

I will not oppose to this change but well frankly I just feel it weird (even no sense) to make a change to add a dependency which is not used... that's my point.

@bmarwell
Copy link
Contributor

Which one is unused? It is replacing junit4. If you remove it, no test will run.

@obfischer
Copy link
Contributor Author

obfischer commented Jul 23, 2021

Why it is not used? It runs the tests, allows users to use write tests in a modern way and modernizes the project, as every dependency update it does. It it elementes this moment "WTF, JUnit 4" 😨

@olamy
Copy link
Member

olamy commented Jul 23, 2021

As already I mean what is the point of this if you are not replacing org.junit.* by org.junit.jupiter.api.*
change the code as well otherwise what is the point to only change a dependency...
again feel free to merge I just don't see the point if you do not change any code...

@bmarwell
Copy link
Contributor

I just don't see the point if you do not change any code...

Well, I do. Future tests can be written with junit-jupiter.
Previously you said it is an unused dependency, which is something totally different!

The vintage engine dependency contains org.junit.* classes. That is the whole point: No need to rewrite unit tests!

I am going to merge it now.

@bmarwell bmarwell merged commit c47291d into mojohaus:master Jul 23, 2021
@obfischer
Copy link
Contributor Author

@olamy I understand your point and as I wrote in one of my comments, I plan to migrate all currently existing test cases. But I decided to split them up in small, but IMHO useful parts, as I can't give any guarantee when I will be able to do it. I don't wan't simply to add to another project a new stale issue, hanging around for some time, which causes only load on the reviewers and maintainers. I hope I was able to illustrate my point. ;-)

@bmarwell
Copy link
Contributor

Works for me! :) Quick question: Is there any hurry to convert tests? Maybe we could keep the existing ones for a while unless they need conversions for particular reasons ("don't touch working code")?

@obfischer
Copy link
Contributor Author

I am not in a hurry, but I would like to convert the tests, so that new users will automatically start to write tests using JUnit 5.

@obfischer obfischer deleted the improvement/472-update-to-junit-5 branch July 23, 2021 07:46
@bmarwell
Copy link
Contributor

That is not too important: New users would need to create a PR which we could review. ;-)

@stefanseifert stefanseifert added the dependencies Pull requests that update a dependency file label Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from JUnit 4 to JUnit 5
4 participants