Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to construct iterable assertions across a specific type easily #3490

Open
ascopes opened this issue May 26, 2024 · 7 comments
Open

Unable to construct iterable assertions across a specific type easily #3490

ascopes opened this issue May 26, 2024 · 7 comments
Assignees
Labels
type: regression A regression from a previous release
Milestone

Comments

@ascopes
Copy link
Contributor

ascopes commented May 26, 2024

Within the new AssertJ 3.26.0 release, I can see that the factory based navigable assertion for iterables has been deprecated internally, along with other mechanisms to do the same thing.

This raises an issue for me, as I extend the AssertJ API and provide additional custom assertions that wrap the assertj ones.

In this case, I have a requirement to be able to return an assertion object across a list of strings for the user to be able to perform further assertions on. The current implementation looks like this: https://github.com/ascopes/java-compiler-testing/blob/1a92ef59df49cf2f45ae83a109569be006b08df2/java-compiler-testing/src/main/java/io/github/ascopes/jct/assertions/JctCompilationAssert.java#L70.

Unfortunately with these APIs being deprecated, I can not find a way to return an iterable assert across strings that automatically expand to string assertions. I can see new API methods such as .first(AssertFactory) and last(AssertFactory), but no way to instruct AssertJ how to expose assertions on internal objects anymore such that the user does not need to explicitly cast these assertions with an explicit InstanceOfAssertFactory object.

Is there any advice for how to migrate off of this, or is this something that will need to be a breaking API change on my side?

@scordio
Copy link
Member

scordio commented May 26, 2024

Thanks for your feedback, @ascopes!

Please give me a bit of time to get familiar with your use case – I'll get back to you.

@scordio scordio self-assigned this May 26, 2024
@scordio scordio added this to the 3.26.1 milestone May 27, 2024
@scordio scordio added the status: pending investigation An issue that needs additional investigation label May 27, 2024
@scordio
Copy link
Member

scordio commented Jun 1, 2024

Although it's possible to obtain the previous behavior, similar to how you did it with TypeAwareListAssert, I believe we should provide better support for such use cases.

However, I wouldn't reintroduce what we've been deprecating as it's less discoverable.

What if we would add an API to configure the assertions for the available navigation methods?

Something like:

assertThat(List.of("a", "b", "c"))
  .withElementAssert(StringAssert::new)
  // further assertions on the list
  .hasSize(3)
  // navigation method
  .first() // returns StringAssert instead of ObjectAssert<String>
  // further String assertions on the element
  ...

Then, your example would become:

public AbstractListAssert<?, List<? extends String>, String, ? extends AbstractStringAssert<?>> arguments() {
  isNotNull();

  var arguments = actual.getArguments();

  return Assertions.assertThat(arguments)
      .withElementAssert(StringAssert::new)
      .as("Compiler arguments %s", StringUtils.quotedIterable(arguments));
}

@scordio scordio added status: waiting for feedback We need additional information before we can continue and removed status: pending investigation An issue that needs additional investigation labels Jun 1, 2024
@ascopes
Copy link
Contributor Author

ascopes commented Jun 1, 2024

This sounds like a very good idea, and would be very helpful! I struggled with getting generics to work consistently for this so any help would be greatly appreciated like this on AssertJ's side to simplify this.

@scordio
Copy link
Member

scordio commented Jun 1, 2024

Would it be okay if we introduce such a change in 3.27.0?

I'd prefer to have enough time to digest it properly, and the deprecations will only be removed in version 4.

@ascopes
Copy link
Contributor Author

ascopes commented Jun 1, 2024

Sounds sensible to me! I'd need to release a new version to change to a new API anyway so there is no rush

@scordio scordio modified the milestones: 3.26.1, 3.27.0 Jun 1, 2024
@scordio scordio added type: regression A regression from a previous release and removed status: waiting for feedback We need additional information before we can continue labels Jun 1, 2024
@Adrodoc55
Copy link

I also heavily use FactoryBasedNavigableListAssert and love the proposal for withElementAssert. I actually wanted to open an issue for improving the generics, but then saw this issue about it beeing deprecated. So I'll post here.

Currently FactoryBasedNavigableListAssert and by extension the deprecated assertThat method requires the ELEMENT_ASSERT to extend AbstractAssert<ELEMENT_ASSERT, ELEMENT>. This sounds reasonable at first and works fine for concrete assert classes, but the bound is too tight for generic assert classes such as abstract ones.

Consider the following example:

import java.util.List;
import org.assertj.core.api.AbstractAssert;
import org.assertj.core.api.AbstractIntegerAssert;
import org.assertj.core.api.AssertFactory;
import org.assertj.core.api.Assertions;
import org.assertj.core.api.ClassAssert;
import org.assertj.core.api.FactoryBasedNavigableListAssert;

public class Test {
  public static void main(String[] args) {
    List<Class<?>> actualClasses = List.of(Integer.class);
    ClassAssert classAsserter1 = Assertions.assertThat(actualClasses.get(0));
    ClassAssert classAsserter2 = Assertions.assertThat(actualClasses, Assertions::assertThat).element(0);
    ClassAssert classAsserter3 = assertThat2(actualClasses, Assertions::assertThat).element(0);

    List<Integer> actualIntegers = List.of(0);
    AbstractIntegerAssert<?> integerAsserter1 = Assertions.assertThat(actualIntegers.get(0));
    AbstractIntegerAssert<?> integerAsserter2 =
        Assertions.assertThat(actualIntegers, Assertions::assertThat).element(0);
    AbstractIntegerAssert<?> integerAsserter3 = assertThat2(actualIntegers, Assertions::assertThat).element(0);
  }

  public static <ACTUAL extends List<? extends ELEMENT>, ELEMENT, ELEMENT_ASSERT extends AbstractAssert<? extends ELEMENT_ASSERT, ELEMENT>> FactoryBasedNavigableListAssert<?, ACTUAL, ELEMENT, ELEMENT_ASSERT> assertThat2(
      List<? extends ELEMENT> actual, AssertFactory<ELEMENT, ELEMENT_ASSERT> assertFactory) {
    return null;
  }
}

For the List<Class<?>> everything works fine, because assertThat(Class<?> actual) returns the non-generic type ClassAssert, but for integers it does not compile. The reason is that AbstractIntegerAssert<?> which is the return type of assertThat(Integer actual) does not satisfy the bound i mentioned above: AbstractAssert<ELEMENT_ASSERT, ELEMENT>. The bound should really be AbstractAssert<? extends ELEMENT_ASSERT, ELEMENT>. To prove this change makes it work I added the method assertThat2 in the example. This method doesn't compile because the incorrect upper bound also appears in the declaration of FactoryBasedNavigableListAssert and its superclasses, but the call to assertThat2 does compile.

@scordio
Copy link
Member

scordio commented Jun 20, 2024

Thanks a lot for your feedback, @Adrodoc55. I'll keep that in mind!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A regression from a previous release
Projects
None yet
Development

No branches or pull requests

3 participants