Assertj-core: [WithAssertions] ListAssert is broken due to generics fluff

[cykl]

Summary

775 fixed some discrepancies between WithAssertions and Assertions (shipped in 3.6.0). Unfortunately, I'm afraid that I got some generics wrong leading unwanted regression 😞

Example

public class RemoveMe implements WithAssertions {

  @Test
  public void fixme() {
    List<String> l = new ArrayList<>();
    assertThat(l).contains("foo");
  }
}

Is broken since 3.6.0.

Error:(213, 18) java: no suitable method found for contains(java.lang.String)
    method org.assertj.core.api.AbstractIterableAssert.contains(capture#3 of ? extends java.lang.String...) is not applicable
      (varargs mismatch; java.lang.String cannot be converted to capture#3 of ? extends java.lang.String)
    method org.assertj.core.api.ListAssert.contains(capture#3 of ? extends java.lang.String...) is not applicable
      (varargs mismatch; java.lang.String cannot be converted to capture#3 of ? extends java.lang.String)

This is related to the addition of the following method:

default <T> ListAssert<? extends T> assertThat(final List<? extends T> actual)

I think that return type should be ListAssert<T>, or ListAssert<? super T>, rather than ListAssert<? extends T>.

Do you want me to open a PR to fix it? Should we review the API to spot such mistakes?

Java 8 specific ?

• YES : create Pull Request from the master branch

[joel-costigliola]

God damned generics ! Good catch @cykl.

I think WithAssertions should be consistent with Assertions (sans jeux de mots :smile_cat: ) and use

public <T> ListAssert<T> assertThat(List<? extends T> actual);

I'm not sure what would be the advantage of returning ListAssert<? super T>.

It would be good to see if there are other discrepancies like that and if there is an elegant way of catching them.

[filiphr]

Isn't it possible to use the tests that are used to make sure that there are no differences between Assertions and BddAssertions. I am not sure if we added WithAssertions when we added those tests.

[joel-costigliola]

I thought we had one but I don't see anything in Assertions_sync_assertThat_with_BDD_and_Soft_variants_Test, we should add a method there and rename the test class as Assertions_entry_points_variants_are_in_sync_Test or something like it.

[cykl]

I added AssertionsTest#with_assertions_should_have_the_same_methods_as_in_assertions as part of #775. But AssertionsTest is now gone and I was not able to spot this test case in Assertions_sync_*.

However, a such test did exist and did not catch my mistake. I have not yet read the code but usingElementComparator(IGNORING_DECLARING_CLASS_AND_RETURN_TYPE) might explain why...

[joel-costigliola]

Did I screwed up this ? likely so (facepalm).

Anyway let's give it a try with an ElementComparator not ignoring return type (obviously).

[cykl]

I just patched BDDAssertions to introduce the same error than in WithAssertions. Assertions_sync_assertThat_with_BDD_and_Soft_variants_Test#standard_assertions_and_bdd_assertions_should_have_the_same_assertions_methods still passes (and it relies on IGNORING_DECLARING_CLASS_AND_METHOD_NAME)

It has been a long time since I last fiddled with generics and reflection, but IIRC getReturnType does not care about generics. getGenericsReturnType might be more useful.

[joel-costigliola]

Good idea, Guava's TypeToken might also be helpful here.

[filiphr]

You are right getReturnType does not care about generics. I tried with getGenericsReturnType, and for some reason ObjectAssert<T> is not equal to ObjectAssert<T>, the reason for this is that it takes the method declaration into consideration when testing the T into consideration.

I haven't used TypeToken it might help. Maybe we need to do the check a bit differently.

Just for info in some places we are ignoring the check of the return type because the SoftAssertions and the normal Assertions have different return types on some places. Like the SoftAssertionListAssert vs the ListAssert, due to the final methods with @SafeVarargs.

[cykl]

I came to the same conclusion when I gave it a try last week.

I just played with Guava's TypeResolver and it might be what we need. It allows to substitute TypeVariable by a Class allowing to compare ParametrizedType.

https://gist.github.com/cykl/0daeb8c1f17c6eae5feacbebf50d4b8d#file-typeresolvertest-java-L63

[filiphr]

You are right that might actually work.

I tried it out in the tests (only for the return type, we need to do it for the parameters as well) and there is a problem when you have something like: AbstractListAssert<?, List<? extends ELEMENT>, ELEMENT, ObjectAssert<ELEMENT>> it cannot resolve List<? extends ELEMENT>, to String. We might need to do some loop there.

[yoelb]

It explains my IDE warning. Thanks for catching @cykl !

[cykl]

@yoelb You better not thanks me for catching a regression I introduced ;)

@filiphr How about https://gist.github.com/cykl/0daeb8c1f17c6eae5feacbebf50d4b8d#file-typeresolvertest2-java ? I haven't tried to plug it into assertj yet.

[filiphr]

@cykl almost there 😄. I plugged it into AssertJ and found some issues. I took the liberty to fork your gist and do some updates. My gist changes are here. You can copy them to your gist.

And there are differences between some returns like Java6AbstractStandardSoftAssertions#L225 and Assertions#L2338.

I would propose that we do this integration with 2.x first so we can align the currently not aligned methods and then do it for WithAssertions.

I have also been thinking about another possible approach with the help of Annotation Processing. We can theoretically define the rules in one class and have the annotation processor create compile time errors if they don't match. I don't know whether the other AssertJ projects have also normal, BDD and Soft assertions, but we could theoretically build the processor so that all the other projects can use it.

[cykl]

My gist changes are here. You can copy them to your gist.

I have been playing with a standalone class because it was easier & quicker to prototype. Maybe it's time to create a branch and work on it. Feel free to take or delete all my code.

I would propose that we do this integration with 2.x first so we can align the currently not aligned methods and then do it for WithAssertions.

My plate being quite full, I don't know when I will have time to do the real work. If someone want to step in, he would be welcome.

I have also been thinking about another possible approach with the help of Annotation Processing. We can theoretically define the rules in one class and have the annotation processor create compile time errors if they don't match.

It might work. Dunno if it's easier / safer / better. If we go this way, couldn't API classes be automatically generated from a reference class ?

[filiphr]

I've created PR #1033 for the 2.x branch. Once we get that one merged, I can make one for the master and hopefully we'll have a consistent API across the entire framework 😄

[cykl]

@filiphr Many thanks for taking over the task.

I haven't seen any obvious oversight in the patch. I cannot test it over our projects because they all rely on 3.x.

[joel-costigliola]

@filiphr I have integrated the #1033 PR into 2.x and merged 2.x into master so that this issue can be solved.

[filiphr]

@joel-costigliola the issue is not completely resolved. I need to prepare a PR for the master branch. There are still sync tests missing for WithAssertions. I'll prepare a PR for that as well.

[filiphr]

@joel-costigliola the WithAssertions should have the exact same methods as the Assertions, right? not just assertThat methods, but also the other ones as well.

[joel-costigliola]

Good point, it think it should indeed provide all methods.

[filiphr]

@joel-costigliola I just created a PR that will fix this issue and additionally adds the missing methods from Assertions to WithAssertions there were quite a few 😄

[joel-costigliola]

Fixed thanks to @filiphr !
@cykl moreover WithAssertions now have all utility method of Assertions (not only assertThat methods)

[filiphr]

Fixed thanks to @filiphr !

And @cykl as well 😉.

[cykl]

Thanks @filiphr & @joel-costigliola! 🥇