Junit5 Migration Part 1 - #1576

[Desislav-Petrov]

This PR sets the groundwork for the migration by introducing the junit5 dependency and migrates the auto-generated test templates. The other modules and any clean up work will be finalised once we do the complete migration.

Dependency management changes

Junit5 comes with two engines - jupiter and legacy. Jupiter is the new Junit5 engine whereas the vintage engine provides junit4 backwards compatibility.

Due to the multi-stage migration approach, we need to have both of these for now. All modules migrated to junit5 will depend on jupiter. All modules to be migrated will depend on vintage. Once all modules are migrated, we will drop the vintage engine dependency as it will no longer be needed.

Because of this legacy and non-legacy dependency management we needed to add a few dependency exclusions for the maven-analyse plugin as otherwise it generates a failure. These will be cleaned up once the migration is concluded as a part of the last task on the issue.

Junit template file explanation
The migration involved making changes to a significant number of files. Most files had to have their imports adjusted. What I have done to simplify future migrations is that I've taken out the main imports into a new junit template file that's imported where needed.

Additional test override to some templates
A few templates needed an @ Override for an iterator test. This was necessary because the original test had a bug that only became visible after the exception expectation of the test was adjusted to match junit5. It was a very neat bug I have to say but all sorted now.

[Desislav-Petrov]

@donraab would appreciate an eye on this - thx

[motlin]

It's a bit hard to review due to the size. I'm looking for the new overrides you mentioned.

I know it's a pain to split up but I'd recommend separating a few changes.

• Switch Assert to a static import so that we don't need to add Assertions in so many places
• Add the new overrides or bug fix
• add the new dependencies and switch the imports

[Desislav-Petrov]

@motlin I appreciate it's a bit of a pain to review but i'm not sure we can split this further in any meaningful way - moving to static asserts and switching imports would end up as 2 PRs touching 150 files each. Similar net effect but it would require a lot of extra time that's why I've bundled them together. Next PRs will be significantly smaller.

The test fix touches about 20 template files, i'm happy to flag all of them if that's gonna help. These are the only places where something changes in the code apart from imports/assertions breaking changes.

[Desislav-Petrov]

@motlin that's an example of the Override that was necessary because the implementation that's inherited from the parent doesn't work in all subclasses. The inherited one tries to add new elements to immutable collections.

It was passing before as it was expecting NoSuchElementException but this exception was coming from the method's super.testMethod() call that was failing, luckily with the correct exception, but nothing after the super.testMethod() call was actually executed.

This became apparent only after moving to the new junit5 assertion style.

[motlin]

We recently landed a few PRs with the goal of paving the way toward JUnit 5, like this one #1555. In it, you can see that we got rid of some usages of @Test(expected = ....class) in templates. I thought we had got rid of all of them, but I see that we still haven't.

I'm having a tough time answering my own questions here because the diff is so big that GitHub collapses all the diffs. Is the test in the superclass changed o not use expected? I'd recommend making that refactoring in its own PR similar to the linked PR and then rebasing this on top.

[motlin]

Suggested change

*.iml

[motlin]

Suggested change

	<groupId>org.junit.vintage</groupId>
	<artifactId>junit-vintage-engine</artifactId>
	<groupId>org.junit.vintage</groupId>
	<artifactId>junit-vintage-engine</artifactId>
	<scope>test</scope>

[motlin]

On second thought, the engine is not a dependency of the code, it's a runtime dependency for the surefire plugin. I'd recommend configuring it there, like I've done here in the Liftwizard project.

[motlin]

This is an unrelated change and should be reverted

[motlin]

We recently landed a few PRs with the goal of paving the way toward JUnit 5, like this one #1555. In it, you can see that we got rid of some usages of @Test(expected = ....class) in templates. I thought we had got rid of all of them, but I see that we still haven't.

I'm having a tough time answering my own questions here because the diff is so big that GitHub collapses all the diffs. Is the test in the superclass changed o not use expected? I'd recommend making that refactoring in its own PR similar to the linked PR and then rebasing this on top.

[donraab]

@donraab would appreciate an eye on this - thx

Hi @Desislav-Petrov, I took a quick look at a few files and read @motlin comments which I gave thumbs up to as I agree with his points. The only way this change can happen in any timely manner is in small chunks (less than 50 files per PR would be ideal). I think the idea of using static imports for Assert initially make sense to make it easier to switch to Assertions without changing every line. This can be done in lots of small chunks, which will then make it easier to review the switch to Assertions which would be just changing the static import from Assert to Assertions if I am understanding things correctly.

I won't have time for the next few months to review any very large PRs. If I see small ones that I can get through without a lot time required, I will try to help @motlin move things forward with you. The easiest to review will be purely mechanical and focus on only one kind of change. Thanks!

[Desislav-Petrov]

@motlin so what would be the plan for this one - do we rebase on top of current master or do we want to move all imports (maybe in chunks..) into static imports first?

[motlin]

I landed a similar change for non-primitive collections recently: #1583

This was easy to implement (though not easy for @donraab to review!) because I was able to use Openrewrite. For primitive collections it's more difficult since we either have to make the similar changes by hand or use Openrewrite but sync the changes back to the templates.

The reason I like Openrewrite here is that it's thorough. https://github.com/eclipse/eclipse-collections/pull/1578/files is a similar refactoring, but only for assertEquals and assertThrows, not assertTrue, assertSame, etc.

From a high level though I think the very next step on the path to JUnit 5 is to get rid of the use of expected. We still have lots of usages.

❯ mvn clean
❯ rg '@test(expected' | wc -l
869