Code does not compile after AssertionsForClassTypes import is replaced by Assertions

[barbulescu]

What version of OpenRewrite are you using?

I am using 3.0.0

• OpenRewrite v6.0.0
• Maven/Gradle plugin v1.2.3
• rewrite-module v3.0.0

How are you running OpenRewrite?

not relevant

What is the smallest, simplest way to reproduce the problem?

import java.util.Map;
import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

class Test {
  void test() {
    assertThat(Map.of("a", 1).entrySet()).hasNoNullFieldsOrProperties();
  }
}

What did you expect to see?

import java.util.Map;
import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

class Test {
  void test() {
    assertThat(Map.of("a", 1).entrySet()).hasNoNullFieldsOrProperties();
  }
}

What did you see instead?

import java.util.Map;
import static org.assertj.core.api.Assertions.assertThat;

class Test {
  void test() {
    assertThat(Map.of("a", 1).entrySet()).hasNoNullFieldsOrProperties();
  }
}

Code does not compile anymore after this change.

What is the full stack trace of any errors you encountered?

Are you interested in contributing a fix to OpenRewrite?

yes

[timtebeek]

Thanks for the report! Curious case; that'l likely stem from these lines that have been in there since 2021:

rewrite-testing-frameworks/src/main/resources/META-INF/rewrite/assertj.yml

Lines 61 to 63 in fc9d8b4

	- org.openrewrite.java.ChangeMethodTargetToStatic:
	methodPattern: "org.assertj.core.api.AssertionsForClassTypes assertThat(..)"
	fullyQualifiedTargetTypeName: "org.assertj.core.api.Assertions"

The hasNoNullFieldsOrProperties seems to be defined on org.assertj.core.api.AbstractObjectAssert, which I'd imagine is returned from both AssertionsForClassTypes.assertThat and Assertions.assertThat.

Is there any other way to make it work with Assertions.assertThat?

[scordio]

I think the root cause is a combination of how the client code uses AssertJ classes and the expectation behind that, with how AssertionsForClassTypes is probably redundant and misleading when placed next to Assertions.

AssertionsForInterfaceTypes and AssertionsForClassTypes were created to aid cases where the compiler struggles to select the appropriate assertThat from Assertions (some background at assertj/assertj#365 and assertj/assertj#473). Specifically, AssertionsForInterfaceTypes is meant to be used whenever assertions for a specific interface are of interest, while AssertionsForClassTypes should be used for assertions of concrete types.

To better describe what happens with the example in this ticket, assuming the usage of AssertJ 3.27.3:

AssertionsForClassTypes.assertThat(Map.of("a", 1).entrySet()) // returns ObjectAssert 
  .hasNoNullFieldsOrProperties(); // passes, but what is the expected semantic for Set<Entry<K, V>> ?

AssertionsForInterfaceTypes.assertThat(Map.of("a", 1).entrySet()) // returns AbstractCollectionAssert 
  .hasNoNullFieldsOrProperties(); // does not compile

Assertions.assertThat(Map.of("a", 1).entrySet()) // returns AbstractCollectionAssert 
  .hasNoNullFieldsOrProperties(); // does not compile

Given that Map::entrySet returns a Set (i.e., an interface), using AssertionsForClassTypes.assertThat leads to getting object assertions instead of collection assertions. Assertions.assertThatObject would be the way to get an equivalent behavior to AssertionsForClassTypes.assertThat when a Set instance is provided:

Assertions.assertThatObject(Map.of("a", 1).entrySet()) // returns ObjectAssert 
  .hasNoNullFieldsOrProperties(); // passes, but what is the expected semantic?

I'm not saying that such a replacement should be suggested by Open Rewrite; I still believe the initial code has a semantic flaw, therefore it isn't something I would expect OpenRewrite to address.

Nevertheless, we'll discuss within the team whether there is anything we can improve in AssertJ. Ideally, there shouldn't be so many options when importing assertThat 🙃

[timtebeek]

Thanks for the detailed explanation! It sounds like perhaps the original test case above is perhaps not the best use of assertions, brought to light by the recipe change here.

To avoid confusion perhaps something like .assertThatClass or .assertThatInterface could help show their intended usage even when statically imported, and avoids the common pattern we see where folks inadvertently import .assertThat from elsewhere. We all know IDEs are eager to suggest the first one that compiles. :) Happy to help of course with recipes if such a change would be coming down the line.

[scordio]

To avoid confusion perhaps something like .assertThatClass or .assertThatInterface could help show their intended usage even when statically imported

We already introduced variants like assertThatCharSequence, assertThatComparable, and assertThatCollection to Assertions.

My gut feeling (not yet discussed with the team) would be to deprecate both AssertionsForInterfaceTypes and AssertionsForClassTypes for removal, while adding the missing assertThatX variants to Assertions. Let's see if we can make this less misleading in AssertJ 4!

[rob-valor]

We also just faced an issue with the same class. When running org.openrewrite.java.testing.assertj.Assertj on a class that has both org.assertj.core.api.Assertions.assertThatExceptionOfType(...).isThrownBy(()->...) and org.junit.jupiter.api.Assertions.assertThrows(...,()->...) (both using imports) the recipe replaces the junit on with org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType ending up with

import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType;

Maybe the AssertJ recipes should also rewrite the AssertionsFor...Types classes to the Assertions class for static imports of methods that both classes provide?

[timtebeek]

For now it's then likely safest to disable these troublesome migrations, while we eagerly await assertThatClass or assertThatInterface.

• Do not ChangeMethodTargetToStatic for AssertJ's AssertionsFor...Types #693

Thanks all!