Assertion library in Platform tests: Hamcrest or AssertJ?

[HeikoKlare]

During test stabilization work and migration to JUnit 4, I have replaced several "plain" assert statements with more comprehensible assertions based on Hamcrest matchers. In order to finally migrate tests to JUnit 5, I would like to proceed with that, as particularly the plain assertEquals is bad in terms of comprehensibility (unclear which parameter is expected and which is actual) and in terms of migration to JUnit 5 (the parameters have been swapped in the JUnit 5 assertions signatures requiring adaptations of every assertEquals statement).

While using Hamcrest matchers are a valid way for improving assertions, there is also AssertJ as another (maybe eben more) popular library. I chose Hamcrest over AssertJ for rather simple reasons: I was used to it and the dependency was already added to the projects I touched first. Since I propose to replace assertEquals statements in general, a lot more assertions will be affected and I would like to give the opportunity for discussion on whether AssertJ (or even another library) would be preferred for assertions across all Platform projects.

The benefit I see in AssertJ is its fluent API providing better syntax and eased combination of assertions. I also read that AssertJ may sometimes provide better error messages. On the other hand, I see benefits in Hamcrest when it comes to custom assertions that can be implemented in complex matchers. This could, for example, be used for all the resource existence assertions in Platform resource tests, which currently rely on simple assertTrue and do not provide very helpful debug output.

If you have an opinion on that, I would be glad if you can share it here.

[vogella]

I personally think it is fine to have both used and think the effort to migrate to only one library is to high. My personal favourite is AssertJ for it's fluent API and IIRC Hamcrest is only very slowly released (this might have changed).

[HeikoKlare]

You seem to be right that Hamcrest is released quite slowly. I was not aware of that.
With respect to migration effort: I currently see only a single usage of AssertJ in Platform projects, so if we stick to Hamcrest there will actually be nothing to migrate.

[akurtakov]

From releng POV - if one of them is already included in the dependency chain it should be preferred (this has been the case for hamcrest but not anymore with JUnit 5, although as JUnit 4 is here to stay hamcrest is technically a required dependency).

This is just to give an extra view angle which is quite often not considered early enough and creates extra work in the long term to keep things working. "Less is more" (tm)

[HeikoKlare]

That's a good point and also an argument for (trying to) stick to one of the libraries rather than allowing both of them to be used.
In addition, if a migration from one library to another would become necessary at some point in time, only having one of them currently used may ease that process because of less ambiguities making pattern-matching based migration more complex.

[laeubi]

In general it should not matter (also from relng POV), but the eclipse test are a bit "special" and PDE support sometimes not very optimal so I think the best would be to try AspectJ AssertJ out for a single test and see what happens.

I already have opened this what most probably will require AspectJ AssertJ at least for PDE for advanced use cases:

• First class OSGi Testing Support eclipse-pde/eclipse.pde#877

[merks]

Note that this discussion is asking asking about assertj not aspectj.

[laeubi]

Fixed :-)

[Bananeweizen]

I've been contributing to many projects in different areas (outside the Eclipse stack). And I would summarize it as:

• Many developers don't care about assertion libraries, and just use the 2 handful of assertTrue/False/NotNull etc. that they have with JUnit.
• People that actually care typically choose JUnit Jupiter together with AssertJ.

The choice of AssertJ over hamcrest is quite simple: You don't have to learn anything. With hamcrest you have to know how to nest some matcher methods, which of them exist and how they are named. I can't remember that. With AssertJ, your IDE code completion will always tell you which exact assertions are available when you type assertThat(someObject).. If it's a collection, then you will see contains/containsExactly/containsInAnyOrder/contains... and so on, if it's an integer, then you will see lessThan/greaterThan/... And writing multiple assertions becomes enjoyable. The following is valid AssertJ:

assertThat(new File("foo.text"))
    .isRegularFile()
    .isNotEmpty()
    .content()
         .contains("someText")
         .doesntContain("someOtherText")
         .hasLineCount(42);

Also the API really takes the extra mile all the time. You will find assertThat(foo).isEmpty() implemented on any type that has a matching semantic, no matter whether the type itself has an actual "isEmpty()" method. Or if you are asserting numbers, you can of course write the typical assertThat(number).isEqualTo(0), but you can also shorten it to assertThat(number).isZero(). In the long run, your assertions become way shorter and more readable at the same time.

For a migration, AssertJ provides some regular expression based conversion directly in the documentation. But I would rather suggest to use OpenRewrite, which is a "refactoring engine for anything". I've been contributing lots of fixes there over the last year, and I meanwhile use it in production for code migrations on "unknown" projects at work quite successfully. https://docs.openrewrite.org/recipes/java/testing/assertj/assertj is the composite refactoring recipe for converting from JUnit to AssertJ, which you can run from a maven command line without adding it to your POM.

If you want to see how AssertJ tests look like and "feel", then start reading the documentation at this anchor: https://assertj.github.io/doc/#use-code-completion

Google Trend agrees: https://trends.google.de/trends/explore?date=today%205-y&q=assertj,hamcrest

[merks]

Thank you for the helpful and insightful response!

[HeikoKlare]

Thank you, @Bananeweizen. Like Ed said, a very helpful comment!
I find the code-completion argument because of AssertJ's fluent API very strong as it relieves you from knowing or searching for appropriate matchers like in Hamcrest. Only with custom assertions for specific types that are not provided via AssertJ's Assertions class, you need to know where to find the according assertThat implementation to import, but that's the same for Hamcrest matchers.

I will also have a look at the OpenRewrite receipes for AssertJ migration. I would not suggest a general/automated migration as many existing assertions I have seen should be rethought/redesigned and at least I have the interest to check and potentially improve an assertion once I touch it. But using OpenRewrite might make sense as a first, rather "mechanic" step when improving specific assertions, as I would like to do for the ambiguous assertEquals statements.

[laeubi]

But I would rather suggest to use OpenRewrite, which is a "refactoring engine for anything".

Did you get it working for eclipse repositories? I tried this once but it was not successful:

• Incompatibility with Maven Polyglot leads to ParseError openrewrite/rewrite-maven-plugin#649 (comment)

I got it working partly but then my recipe did not get applied and I found it hard to get any output why:

• https://github.com/eclipse-platform/eclipse.platform.ui/blob/master/bundles/org.eclipse.ui.workbench/META-INF/rewrite/platformui_sethelp.yml

If you like you can also contact me per mail so we maybe can chat about the topic separately.

[HeikoKlare]

Thank you all for the great input!

I found the arguments pro AssertJ convincing and gave it a try by changing the Hamcrest assertions in the Platform repository to AssertJ assertions. In many cases, they became much more comprehensible and it was easier to combine different kinds of assertions on the same object in comparison to Hamcrest.

I also found that mixing up both libraries is not a good idea: I found broken assertions because both libraries were used in the same compilation unit and matchers are passed to AssertJ statements, which makes the assertion ineffective: eclipse-platform/eclipse.platform#1075

You can find the migration for the Platform repository I experimented with in eclipse-platform/eclipse.platform#1076.

[laeubi]

It would be great if JDT can warn on "ineffective" assertions (even though it is of course framework dependent and can't be done in a generic way).

[HeikoKlare]

Yes, that would be great. But I am not sure whether that would be possible without becoming very case-specific. For example, the risk I see with AssertJ is that if you define a pure boolean assertion, you need to write assertThat(condition).isTrue(), where it is likely that you forget the isTrue(). In particular because, e.g., in Hamcrest assertThat(condition) is a proper assertion.
Concerning the example I found with mixing up Hamcrest and AssertJ is even worse. You can write something like the following, which "sounds" valid, but is complete nonsense:

Object object = ...;
assertThat(not(object)).isNotNull();

But even if it's possible, it would not be that easy to determine when an assertion is "effective". Only checking whether the result of assertThat() is further processed is insufficient, as, for example, assertThat(condition).as(message) would still be ineffective.

I see a similiarity to returned status objects for which you want to enforce further processing (such as IStatus). I know that Sonar rules exist for that, but the case is slightly simpler as you only need to check whether a specific return type is processed.

[Bananeweizen]

Sonarqube can detect incomplete assertions of AssertJ and several other libraries. I have that running at work and it surely does the job. I believe the methods are hardcoded in that check. A generic check would be possible if more people used the annotation javax.annotation.CheckReturnValue to mark that a return value is mandatory to be evaluated further. Spotbugs has such a check but the AssertJ methods are not annotated that way, if I got it right.

[HeikoKlare]

Thank you all for your helpful responses! There were some very interesting arguments in particular concerning benefits of AssertJ and concerning the releng POV on the topic. Based on this discussion I've proceeded towards JUnit 5 migration using AssertJ, as I see clear benefits over Hamcrest and once JUnit 3/4 are not required anymore, the dependency to Hamcrest will not be necessary anymore.