assertj / assertj

AssertJ is a library providing easy to use rich typed assertions
https://assertj.github.io/doc/
Apache License 2.0
2.6k stars 694 forks source link

Support for `has*CauseInstance*` variants of `isInstanceOfAny`, ... #3573

Open rsampaths16 opened 3 weeks ago

rsampaths16 commented 3 weeks ago

Feature summary

Provide isInstanceOfAny, isNotInstanceOfAny variants for hasCauseInstanceOf, hasCauseExactlyInstanceOf, hasRootCauseInstanceOf, hasRootCauseExactlyInstanceOf ... throwable asserts.

Example

Before

assertThat(result.getResolvedException())
                .isInstanceOfAny(ConstraintViolationException.class, MethodArgumentNotValidException.class)

After

assertThat(result.getResolvedException())
                .hasCauseExactlyInstanceOfAny(ConstraintViolationException.class, MethodArgumentNotValidException.class)
scordio commented 3 weeks ago

Have you considered the [cause()](https://www.javadoc.io/static/org.assertj/assertj-core/3.26.3/org/assertj/core/api/AbstractThrowableAssert.html#cause()) and [rootCause()](https://www.javadoc.io/static/org.assertj/assertj-core/3.26.3/org/assertj/core/api/AbstractThrowableAssert.html#rootCause()) navigation methods?

assertThat(result.getResolvedException())
  .cause()
  .isInstanceOfAny(ConstraintViolationException.class, MethodArgumentNotValidException.class);
rsampaths16 commented 3 weeks ago

Thanks @scordio - you've helped me catch an understanding gap here. What I need to use to test my case was assertThat(...).isInstanceOfAny(...). I didn't realise cause() only verified the immediate cause for the exception.

The solution you've provided should work for anyone willing to do the full instance check on the exception causes & root causes.

Given that, I still think having similar set of interfaces for cause and rootCause will provide a richer experience in using AssertJ.

P.S: Linking to AssertJ throwable assertion docs for anyone winding up here: https://assertj.github.io/doc/#assertj-core-throwable-cause-and-root-cause-assertions

scordio commented 3 weeks ago

[rootCause()](https://www.javadoc.io/static/org.assertj/assertj-core/3.26.3/org/assertj/core/api/AbstractThrowableAssert.html#rootCause()) is also available, I updated the previous comment.

We prefer relying on navigation methods to avoid blowing up the API. Is there a use case you can't achieve with cause or rootCause?

rsampaths16 commented 3 weeks ago

Is there a use case you can't achieve with cause or rootCause?

No, both cause & rootCause should solve for the all the usecases.

We prefer relying on navigation methods to avoid blowing up the API.

The ticket can be closed if this is the stance.


On a tangent, are there APIs which tests if at least one cause ( from the throwable given, to the root cause ) matches the given test. Ex:

// a = new T1();
// b = new T2(a);
// c = new T3(b);
// d = new T4(c);
assertThat(d).hasAnyCauseExactlyInstanceOf(T4.class);
assertThat(d).hasAnyCauseExactlyInstanceOf(T3.class);
assertThat(d).hasAnyCauseExactlyInstanceOf(T2.class);
assertThat(d).hasAnyCauseExactlyInstanceOf(T1.class);

If this is something you're willing to explore, we can create a different ticket for this.

scordio commented 3 weeks ago

If I understand it correctly, you're looking for something to verify the type of a cause, which might be direct or indirect, up to the root cause.

As of now, we don't have any assertion to achieve that.

Out of curiosity, are you facing a concrete use case where you cannot predict at which level the cause to be verified is?

rsampaths16 commented 3 weeks ago

This is not something that happens during unit testing, but during integration testing, where we want to check if certain business exception has been raised, which could be intercepted by other layers ( framework or otherwise ).

scordio commented 3 weeks ago

Isn't where rootCause() would help to verify the business exception which was originally raised?

rsampaths16 commented 3 weeks ago

This is not if the exceptions are abstracting the underlying cause. Ex: errors for different persistent layers are caught and re-thrown wrapped as an abstract business exception ( empty error, not found ... etc -> resource not found ).

scordio commented 3 weeks ago

What if we provide a cause(Class<?>), which would recursively search for a cause of the given type and allow further assertions?

I imagine a usage like the following:

assertThat(result.getResolvedException())
  .cause(MyBusinessException.class) // fails if no cause of the given type is found
  .hasMessage("...") // further assertions

I'm not entirely satisfied with my proposal because the recursive behavior might be too hidden, but it might be a direction worth refining.

rsampaths16 commented 3 weeks ago

I feel a navigation method will be helpful, but it should be something other than cause & rootCause since they denote immediate cause & root cause.

Maybe forCause, forExactCause navigation methods. Where forCause will navigate to the first match even with inheritance relationship and forExactCause will navigate to the first exact match. Both of them will give assertion failure if no cause matched them.

scordio commented 3 weeks ago

I feel a navigation method will be helpful, but it should be something other than cause & rootCause since they denote immediate cause & root cause.

I agree. I wouldn't go with a for prefix but rather try to find a suitable adjective that could be used together with cause, perhaps .intermediateCause() or something similar... naming is hard

Thanks for the input, we'll discuss it internally and get back to you.

rsampaths16 commented 3 weeks ago

What are you thoughts on the names atCause and atExactCause ( or ) theCause and theExactCause?

assertThat(exception).atCause(MyException.class).hasMessage("...") assertThat(exception).theCause(MyException.class).hasMessage("...")

scordio commented 3 weeks ago

We tend to name navigation methods after getters but without the "get" part, i.e., cause() from Throwable::getCause().

Using an "at" preposition might be less intuitive and usually adjectives fit better.