Gotta say I’m not a fan of that matcher change :smile: Do people really need to check for cause as w...

# kotest

[wasyl]

Gotta say I’m not a fan of that matcher change 😄 Do people really need to check for cause as well? I see we do that only in some places specifically, otherwise causes are pretty much private. And the error message if cause is different is pretty vague because it only includes exception class and message, so I end up with

expected:<com.example.MyException> but was:<com.example.MyException>
Expected :com.example.MyException
Actual   :com.example.MyException

😕

[sam]

Yeah I tend to agree. @Ashish Kumar Joy made the change, what do you think about reverting it ?

[wasyl]

aand also shouldHaveMessage accepts String and not String so exception shouldHaveMessage other.message doesn’t work 😄

[sam]

Yeah this seems ill thought through

[sam]

Let's wait and see what Ashish says but we could easily revert and push out 4.4.4

[wasyl]

+1 for reverting this change, I’ll postpone upgrading until a decision is made. I wouldn’t mind having to migrate to some other variant that ignores cause (or having a separate matcher that explicitly checks for cause)

[Ashish Kumar Joy]

To me it looks like cause is a part of Throwable so two throwable should be equal if both message and cause are equal.

But in case if people generally consider message as basis of equality and this is breaking existing tests. I agree with reverting that change.

[sam]

I am thinking we can add another matching, shouldHaveExactException (or something with a better name)

[sam]

@Ashish Kumar Joy I think the idea behind the change was good - we should check all fields - but we've had it the other way for so long I think we should revert it and add a new matcher.

[sam]

I think the ThrowableEq should at least check that the exception classes are compatible

[sam]

Like IOException("foo") shouldBe NumberFormatException("foo") should be false

[Ashish Kumar Joy]

Class compatibility check was already present in ThrowableEq

[Ashish Kumar Joy]

I reverted the previous commit

[wasyl]

I’d consider practical side as well: in more complex functional-like tests, the exception might have multiple causes, and it’s rarely relevant in code what the cause is. Having to verify it in tests locks the implementation a lot. Exception class + message is what I personally always relied on and I never missed verifying the cause as well

[wasyl]

shouldHaveExactException seems like a good addition though, if someone does want to verify causes too

[sam]

Let's add a new matcher and I'll release 4.4.4 with details of what happened in the changelog