Gotta say I’m not a fan of that matcher change :sm...
# kotest
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
Copy code
expected:<com.example.MyException> but was:<com.example.MyException>
Expected :com.example.MyException
Actual   :com.example.MyException
Yeah I tend to agree. @Ashish Kumar Joy made the change, what do you think about reverting it ?
aand also
and not
exception shouldHaveMessage other.message
doesn’t work 😄
Yeah this seems ill thought through
Let's wait and see what Ashish says but we could easily revert and push out 4.4.4
+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)
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.
I am thinking we can add another matching,
(or something with a better name)
@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.
I think the ThrowableEq should at least check that the exception classes are compatible
IOException("foo") shouldBe NumberFormatException("foo")
should be false
Class compatibility check was already present in ThrowableEq
👍🏻 1
I reverted the previous commit
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
seems like a good addition though, if someone does want to verify causes too
Let's add a new matcher and I'll release 4.4.4 with details of what happened in the changelog
👍 1