Assertj-core: Custom Assertions and comparison failures

[kriegfrj]

Summary

Trying to write custom assertions. The examples given show the use of failWithMessage().

The problem with failWithMessage() is that it doesn't populate the "expected" and "actual" fields of the generated assertion. These fields are useful to have populated where it makes sense to do so (eg, equality assertions on individual fields) as the IDEs use them to support graphical diff.

I can ignore failWithMessage() and generate an org.opentest4j.AssertionFailedError and populate these fields manually; however when I do so it is hard to honour the error reporting customisation interface of AbstractAssert. Similarly, it is hard to inherit/honor things like comparison strategy.

Possible solutions

-Introduce a new method in AbstractAssert which provides functionality for generating a ComparisonFailure or related assertion.
-AssertJ currently divides its packages into two types:
--Internal
--Consumer
I believe that AssertJ would benefit from the addition of a "service provider interface" (SPI) category of packages - comprised of classes and utilities that are intended for use by assertion implementors but not assertion users. At present, there are many classes in the "internal" package which understandably we wouldn't want to expose to assertion users, but could be really useful to implementors of custom assertions.
-It is possible that all of this is possible in AssertJ already and I simply don't know how to do it. If that is the case, it would be nice if the custom assertion doc page gave some examples of how to do this.

[bjhargrave]

Making AbstractAssert.withAssertionState public (currently package private) would be a great initial step here. This method is used when mapping assertion types with AbstractAssert.asInstanceOf and also when extracting with AbstractObjectAssert.extracting to propagate assertion state information to the new assertions. Custom assertions could use AbstractAssert.withAssertionState to propagate their assertion state onto any child assertions they may create like asInstanceOf and extracting currently do.

[scordio]

Making AbstractAssert.withAssertionState public (currently package private) would be a great initial step here.

In your original comment you wrote protected where I tend to agree, but public maybe is too much, or do you see a use case where a custom assertion would not inherit from AbstractAssert?

[bjhargrave]

In your original comment you wrote protected where I tend to agree, but public maybe is too much

The issue is that I would like to write something like:

return assertThat(actual.someMethod()).withAssertionState(myself);

So that I can create a new child assertion for some aspect of actual and copy myself's assertion state into the new assert. So having withAssertionState be protected is of no help since I need to call it on another assert object and not on my assert object. (I realized that as I reread my original comment.)

If my custom assert class subclasses AbstractObjectAssert, I can use extracting(Function) as a map method to do what I want:

return extracting(ACTUAL::someMethod);

This is basically fine BUT many custom assertions should not extend AbstractObjectAssert, they should extend AbstractAssert which has no equivalent to extracting(Function).

So an alternate to making AbstractAssert.withAssertionState public would be to pull extracting(Function) up to AbstractAssert so that it is available for all custom assert classes. If the extracting name seems wrong for AbstractAssert, then the method could be called map since this really is a mapping operation.

For now, in my custom assert class which extends AbstractAssert, I must first "map" the assert to an AbstractObjectAssert using asInstanceOf and then further "map" it to the desired child assert with extracting(Function).

See https://github.com/osgi/osgi-test/blob/73f897268f2a1fc4c63deec3c8d3c496ae6f6460/org.osgi.test.assertj/src/main/java/org/osgi/test/assertj/promise/AbstractPromiseAssert.java#L258-L261.

[scordio]

Hi @bjhargrave, I plan to take another look at your comment/example to understand your use case better, meanwhile about the following:

This is basically fine BUT many custom assertions should not extend AbstractObjectAssert, they should extend AbstractAssert which has no equivalent to extracting(Function).

Why do you think that AbstractPromiseAssert extending AbstractObjectAssert would be a bad solution? Are you concerned about exposing additional APIs (e.g., hasFieldOrProperty(), returns(), etc.) which are not relevant to your custom assertion?

[bjhargrave]

Why do you think that AbstractPromiseAssert extending AbstractObjectAssert would be a bad solution? Are you concerned about exposing additional APIs (e.g., hasFieldOrProperty(), returns(), etc.) which are not relevant to your custom assertion?

Yes. If you look at the standard assertions in AssertJ, you find most of them do not extend AbstractObjectAssert. Most extend AbstractAssert. CompletableFutureAssert is the closest to PromiseAssert (in that CompletableFuture and Promise address similar problem spaces) and CompletableFutureAssert extends AbstractAssert. OptionalAssert also extends AbstractAssert and Optional, like Promise and CompletableFuture, is a holder of a value.

Extending AbstractAssert makes for smaller API surface for the user when doing code completion and users should not be writing assertions against the internal members of a Promise. Promise is an interface and can be implemented by anyone. Having PromiseAssert extend AbstractObjectAssert "exposes" implementation details that the user should not be testing.

In general, I would say that custom assertions should extend AbstractAssert since the user should not be seeing methods to test implementation details of the object. The purpose of the custom assertion is to define the testing abstraction and thus the test methods the user should use to test the object. AbstractObjectAssert is useful when there is no custom assertion and the user can use this assert to test implementation details since they have no custom assertion to define a testing abstraction.

[scordio]

@bjhargrave thanks for the explanation, it is exactly what I thought.

I still would avoid exposing withAssertionState() if possible. What if we "promote" extracting(Function)/extracting(String) as protected methods of AbstractAssert?

@joel-costigliola thoughts?

[bjhargrave]

What if we "promote" extracting(Function)/extracting(String) as protected methods of AbstractAssert?

That would work for my use case. Thanks.

You may also want to promote the InstanceOfAssertFactory variants such as extracting(Function, InstanceOfAssertFactory) to AbstractAssert. They are not strictly necessary, but they are a nice complement.

[kriegfrj]

You may also want to promote the InstanceOfAssertFactory variants such as extracting(Function, InstanceOfAssertFactory) to AbstractAssert. They are not strictly necessary, but they are a nice complement.

:+1: for promoting the InstanceOfAssertFactory variants too. They are not strictly necessary for the PromiseAssert use case because the type of the value is not known at compile time and so you can't narrow the assertion class beyond AbstractObjectAssert. But we had discussed the possibility of a BundleAssert.hasVersionThat() method which would ideally return an AbstractVersionAssert (see https://github.com/osgi/osgi-test/pull/50#discussion_r358313990), and I don't think that would be possible without one of the InstanceOfAssertFactory variants being visible to AbstractBundleAssert.

However, I'm also wary of @bjhargrave's comment above (https://github.com/joel-costigliola/assertj-core/issues/1731#issuecomment-581413818):

In general, I would say that custom assertions should extend AbstractAssert since the user should not be seeing methods to test implementation details of the object. The purpose of the custom assertion is to define the testing abstraction and thus the test methods the user should use to test the object.

I agree with this principle, and also with the idea of keeping the "API surface" as small as possible, but if we promote all of these extracting() variants to AbstractAssert then we are starting to violate these principles.

A couple of possible workarounds:

1. Make the extracting() variants in AbstractAssert protected, and then override them in AbstractObjectAssert to make them public for that class. Or:
2. Instead of (or perhaps as well as) making withAssertionState() protected, create a protected static version like this:

java protected <S extends AbstractAssert<S, ?>> S copyAssertionState(S to, AbstractAssert<?,?> from) { return to.withAssertionState(from); }

Then you could write:

java return copyAssertionState(assertThat(actual.someMethod()), myself);

The syntax is not as pretty, but it gives you the same functionality as withAssertionState() without needing to make it public. I have verified that this works.

I'm leaning towards promoting the extracting() variants into AbstractAssert, but making them protected. I think that these methods belong in AbstractAssert in the same way that asInstanceOf() belongs.

I would also consider making withAssertionState() protected rather than package-private. I can see there have been instances where specialised assertion subclasses have needed to override this method, so while I don't have a use case right now it is reasonable to assume that one may surface.

[scordio]

Make the extracting() variants in AbstractAssert protected, and then override them in AbstractObjectAssert to make them public for that class.

This would be my proposal, definitely not being public in AbstractAssert. And yes, I would move the InstanceOfAssertFactory variants too.

Instead of (or perhaps as well as) making withAssertionState() protected

As far as I understood, if extracting() is protected there is no need to have also withAssertionState() as protected. I would leave the latter as package-private and we can react when a use case appears.

[kriegfrj]

Instead of (or perhaps as well as) making withAssertionState() protected

As far as I understood, if extracting() is protected there is no need to have also withAssertionState() as protected. I would leave the latter as package-private and we can react when a use case appears.

Yes, this proposal is quite separate to the use cases that are the focus of this issue. However, there are already other use cases that demonstrate the need for this functionality in AssertJ itself - some of the assertion classes override withAssertionState() (AbstractObjectAssert, AbstractObjectArrayAssert, AbstractIterableAssert). They do this because they store their own specific state that is not part of AbstractAssert and they want to copy it. It is reasonable to expect that a custom assertion implementer may wish to have access to the same copy hook, for the same reason (ie, they want to maintain their own custom state and they want to copy it if possible).

[joel-costigliola]

-- edited --
I'm fine with moving extracting as protected in AbstractAssert.

@kriegfrj @bjhargrave would that address your use cases?

[kriegfrj]

Sounds good to me!