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 from JUnit 4 to JUnit 5 #11839

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

Nezisi
Copy link

@Nezisi Nezisi commented Jun 3, 2023

This is a PR that converts the JUnit 4 + Hamcrest + fest-assert Java code to JUnit 5.

I tried to minimize the changes to the bare minimum as good as I could, thus:

I'm not entirely sure what the best approach regarding dependencies is: I'd guess it's best to add JUnit 4 dependencies only to the play-test project?

@ihostage
Copy link
Member

ihostage commented Jun 7, 2023

Hi, @Nezisi! 👋
Firstly, thank you a lot for your work 👍
Secondly, my opinion is that as replacement assertion library Hamcrest we should using AssertJ instead of pure JUnit5 API.

@Nezisi
Copy link
Author

Nezisi commented Jun 7, 2023

@ihostage
Hello :) Thanks for your work, too!

Wouldn't mind porting later to AssertJ - though I strongly suggest doing it in a separate PR.

A lot of the calls of Hamcrest / fest-assert are similar to those in AssertJ, but might have different meaning / behaviour.

Getting a common ground by first migrating to JUnit 5, then doing the migration to AssertJ should be easier / less confusing.

Haven't used AssertJ so far, but there is a script available from AssertJ doing a lot of the "grunt work":
https://github.com/assertj/assertj/blob/main/assertj-core/src/main/scripts/convert-junit5-assertions-to-assertj.sh

Just to clarify: The only reason I removed Hamcrest was because of its dormant state ( https://groups.google.com/g/hamcrest-dev/c/lYcF3lz8JDQ ) and fest-assert being dead.

@Nezisi Nezisi changed the title Draft: Migrate from JUnit 4 to JUnit 5 Migrate from JUnit 4 to JUnit 5 Jun 9, 2023
@Nezisi
Copy link
Author

Nezisi commented Jun 9, 2023

Last commit splits the play-test package into 3 separate projects:

  • play-test-project itself ⇒ Scala Code + Non JUnit-specific classes like TestBrowser / TestServer / Helpers
  • play-test-junit4 ⇒ JUnit 4 Extensions + Tests, separate package breaks backwards compatibility, but eases dependency handling / maintenance
  • play-test-junit5 ⇒ Added the most simplistic JUnit 5 extensions possible, separate package for the same reason as JUnit4

I tried to set up the projects in build.sbt, but I think I need to add “somewhere” the java formatter, the code in the JUnit 5 tests is definitely wrongly formatted. Edit: lazy val userProjects needed to be extended, fixed.

I just cannot find out where the java formatter configuration is sobs

Regarding the JUnit5 extensions: I was first thinking about adding more complex extensions, for example an annotation-based approach like @PlayApplication (similar to what Micronaut does)... But this complexity would mean an additional burden regarding maintenance, as Play & the Jupiter Interface plugin would need to follow more closely the JUnit 5 upstream regarding breaking changes / version compatibility.

Given that the resources are already stretched thin, it's better to keep it as simple as possible.

Imho the PR is now good to go. We just need to resurrect the jupiter-interface, but I've already got an PR open on that side.

@Nezisi
Copy link
Author

Nezisi commented Jul 14, 2023

I've migrated the documentation, sorry it took so long.

There is one part in the documentation validation that I cannot wrap my head around, sadly.

The validation plugin seems to exclude explicitly everything that does not belong to either java / scala package structure.

But this link exists and works:
https://www.playframework.com/documentation/2.8.x/api/java/play/test/package-summary.html

Where does the magic happen that the play-test package gets moved to java/play/test... Sorry if I'm missing something obvious here. :(

Edit: It was a problem related to SBT. I corrupted the cache, which led to a lot of confusion. Nuking all target directories after I noticed the CI did what I actually expected to happen, everything was fine.

@Nezisi
Copy link
Author

Nezisi commented Jul 15, 2023

At long last, this should cover all the necessary grunt work to be on JUnit 5.

There is a lot of room for improvement, for example the suggestion of @ihostage to use AssertJ, additional usage of extensions / improvement for the extensions, removal of deprecations inside the test, ...

But that should come – in my opinion – after merging this PR. ;) Given the massive size, it's better to get this merged before it turns into an endless “yet another improvement” limbo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

None yet

4 participants