Replace Hamcrest assertions with AssertJ assertions

[HeikoKlare]

Comprehensive assertions can be defined with different libraries, of which Hamcrest and AssertJ are two of the most popular ones. While Platform tests currently only use Hamcrest at some places, AssertJ has several advantages including:

• Easier to apply and learn due to fluent API rather than matcher providing comprehensive code completion support
• Higher expressiveness by easily chaining multiple assertions on the same object
• Better comprehensibility by chaining instead of nesting calls and avoiding the necessity to start an assertThat statement with a custom message if desired

This change migrates the existing usages of Hamcrest to AssertJ (except for one custom Matcher implementation). Doing that it also makes use of String formatting rather than String concatenation with a potential slight performance improvement.

See also the discussion eclipse-platform/.github#177

[github-actions]

Test Results

   594 files  ±0     594 suites  ±0   42m 19s ⏱️ - 4m 19s
 3 864 tests ±0   3 842 ✅  - 1   22 💤 +1  0 ❌ ±0 
12 198 runs  ±0  12 037 ✅  - 3  161 💤 +3  0 ❌ ±0 

Results for commit ae41a9e. ± Comparison against base commit 4ebb172.

This pull request skips 1 test.

org.eclipse.debug.tests.ui.LaunchConfigurationTabGroupViewerTest ‑ testOnlyDefaultTabInOtherConfigIsActivated

♻️ This comment has been updated with latest results.

[HeikoKlare]

This pull request skips 1 test.

org.eclipse.debug.tests.ui.LaunchConfigurationTabGroupViewerTest ‑ testOnlyDefaultTabInOtherConfigIsActivated

This PR corrects a faulty assertion in LaunchConfigurationTabGroupViewerTest#testOnlyDefaultTabInOtherConfigIsActivated(), which requires it's temporary disablement as the test fails with the correct assertion, see #1075.

[Bananeweizen]

Just one more recommendation: It's perfectly fine to convert existing tests with error message towards AssertJ as(errorMessage) calls. However, that is not the best way. as("something") just changes the description of the unit under test, but the original error message remains.
assertThat(foo()).as("very long error message").isTrue() will still produce "Expecting 'very long error message' to be true, but it was false". It just changes the name or description. assertThat(foo).withFailMessage(()->"my custom message").isTrue() would really replace the complete message. Now you might be wondering why does the "as" method exist in the first place? Take this example:

assertThat(familyMember.get(0)).as("father").has...
assertThat(familyMember.get(1)).as("mother").has...

A failure in that test will be written "Expecting 'father' to have..."

And let me repeat: Your conversion of the existing tests is fine, no need to change it there. It just allows even more nice error messages in new tests.

[Bananeweizen]

Suggested change

	assertThat(longRunningProjects).as(
	"all build jobs have started in time although infinitely running builds with conflicting rules exist")
	.anySatisfy(longRunningProject -> assertThat(TimerBuilder.getStartedProjectBuilds())
	.doesNotContain(longRunningProject));
	assertThat(longRunningProjects)
	assertThat(TimerBuilder.getStartedProjectBuilds())
	.withFailMessage("all build jobs have started in time although infinitely running builds with conflicting rules exist")
	.doesNotContainAnyOf(longRunningProjects);

[HeikoKlare]

Thank you for your feedback, @Bananeweizen!
I also thought some time about the as(message) calls, in particular because some of the existing failure messages do not really make sense as a description for the assertion. Many of them would totally make sense when passed to withFailureMessage(message), but then the failure report will only contain that message and nothing about the expected/actual value. In many cases it can be helpful to have more detailed information about the actual and expected values. However, for pure boolean assertions it totally makes sense to simply use withFailureMessage(), which is why I will update the PR according.

Regarding your concrete proposal for one assertion: the proposed assertion is semantically different from the original one. The original one checks that "not all long running project builds have started". The proposed on checks that "none of the long running project builds have started". This assertion looked better with Hamcrest, where you can combine matchers to: assertThat(TimerBuilder.getStartedProjectBuilds(), not(containsInAnyOrder(longRunningProjects))). Unfortunately, I did not find an according assertion in AssertJ, which is why I made this strange construct of assertions realizing "there is a long running project build that has not started". If you know a better way to express that, I'd be happy if you can let me know :-)

[HeikoKlare]

I have now also replaced several pure boolean assertions with matches assertions that improve the output in case of a failure, in particular because they also output the subject under test. This makes it far easier to provide a helpful error message with minimal custom text.
The marked code snipped shows as example for that.

[Bananeweizen]

That's an interesting technique that I will remember, thanks for that hint. Regarding earlier discussions: You are completely right about as() and withFailureMessage() sometimes not fitting well, therefore in practice it's sometimes more ugly than the nice theory I painted before. Personally I try to use as() as much as possible, if the assertion itself has helpful text (like isNonEmptyDirectory), but switch to withFailMessage, if the default error message is to generic (like isTrue). Since withFailMessage can take format strings and objects, that's also okay performance wise. However, I'll probably use more of your matches(...) version in the future, as that probably simplifies custom error messages a bit.

[HeikoKlare]

As a side note: a method reference used for matching can even be negated easily with Predication.not(), which, when statically imported, can be used like this: assertThat(file).matches(not(IFile::exists), "does not exist").

[Bananeweizen]

👍🏻 LGTM

[HeikoKlare]

Thanks for your review and all your comments, @Bananeweizen! Feels like this PR was worth the effort as it improves some validations even semantically, and particularly because I I've learned quite some things about validations / AssertJ.