VerifyNoOtherCalls is failing a lot for existing tests due to unverified event registrations

[Onkelborg]

Due to #1084 (#1082), VerifyNoOtherCalls have started to fail for lots of existing unit tests.

I understand the intention behind the change, however, for existing unit tests, the change is a big issue. For example, in our code base, we have approx 2500 unit tests, and 70 of them started failing due to this change of behavior.

May I suggest adding an overload to VerifyNoOtherCalls (with ex. enum flags or something as parameters) to tall what kind of backwards compatibility we want? That way, we could simply just take all our existing tests and change all calls to VerifyNoOtherCalls to explicitly opt out verification of event registrations, avoiding the need to try to understand all unit tests now (both failing, and passing..)

Edit: Oops, 40 are failing to this change, 30 are failing due to some other change - tried the previous version too, something else has also changed in terms of unverified calls.

[stakx]

As I'm sure you're already aware, this change was intentional. I decided to make it because VerifyNoOtherCalls hasn't been around for that long yet, so it was likely to affect only a small part of all Moq-dependent code out there. Adding a new overload to cater for the old behavior doesn't seem like a good idea to me TBH, because the old behavior was arguably "wrong". If you're ready to adjust your unit tests (by calling another VerifyNoOtherCalls overload), why not instead add the missing verification of event subscriptions?

You said 30 of your tests fail for some other reason. Please do report a repro test case if possible; if there's been any regression I'd love to look at it.

[Onkelborg]

I understand that it was intentional, it was more a "I want to use this new feature, but I don't want to understand and fix a lot of old unit tests that broke due to the change not being backwards compatible". But hey, I'm fixing them now :)

(Just updating all calls to VerifyNoOtherCalls could be done with a simple "search and replace" and would be a lot quicker than trail and error on lots of old tests - could do that another day since it's really unrelated to stuff I'm actually working on right now. It's not a big deal for me, but with an even larger code base I guess it could be a bigger issue.)

Regarding the other tests that have started failing - I'm not sure what it's about yet. I think it's something that some of my mocks have returned mocked objects on their own, and that the calls on those objects have not been verified. I tried to find something in the changelog about that, but I didn't spot anything obvious - but I haven't dug into this one yet. If I find something that is causing issue, I'll create an issue about that.

Being backwards compatible is a major annoyance, I know.. Keep up the good work :)

[Onkelborg]

Regarding the other issue, I think #1093 is related

Edit: I did have some issues in a few tests where I had accidentally created a mock, and passed it directly into the Setup methods instead of passing a delegate that creates a mock. Doing that change seems to fix that one for me :)

[stakx]

If the remaining cases are indeed due to #1093 then I think we can close this issue. Otherwise, please do post a small repro case.

[Onkelborg]

I didn't look that much into it, it's not related to the change here though, and by making a few adjustments mentioned in the other issue, I got those to work.

However, I do think that major changes like done in this issue are a bit troublesome, but I don't see a "good" solution - the idea of adding an overload with what behavior to expect was simply an idea of a solution from my side. I'm perfectly fine with closing this issue.

[stakx]

@Onkelborg I agree that breaking changes are not pleasant and should be avoided. The best solution would be to get things right from the start. Where that goes wrong, the next best thing is probably to increase the major version so that automatic package updates don't randomly break code (i.e. semver); unfortunately that is not an option because this project is bound to major version 4. So I try to not make breaking changes whenever I can. But every now and then, the positives outweigh the negatives (at least IMHO) and so occasionally I decide to risk them anyway. I realise this will be annoying to those users who have come to rely on the old behavior. At the same time, it's good to clean up inconsistencies before they become too cemented.