Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove mandatory SetupAdd & SetupRemove for eventhandler subscription verification. #1058

Closed
80O opened this issue Sep 22, 2020 · 1 comment · Fixed by #1082
Closed

Remove mandatory SetupAdd & SetupRemove for eventhandler subscription verification. #1058

80O opened this issue Sep 22, 2020 · 1 comment · Fixed by #1082

Comments

@80O
Copy link

80O commented Sep 22, 2020

What is the reason that having to add this setup:
_mock.SetupRemove(d => d.Event -= It.IsAny<EventHandler>());

To verify this...
_mock.VerifyRemove(d => d.Event -= It.IsAny<EventHandler>(), Times.Once);

... is mandatory?

Normal function calls do not require to be 'registered' beforehand either. I think it will help if these setups are not required and act more like any other Verify() call.

@stakx
Copy link
Contributor

stakx commented Sep 22, 2020

IIRC (from #825 and #857), the reason was backward compatibility. Verify{Add,Remove} was added only recently (unlike Setup{Add,Remove} which has been around much longer). Those methods can only work iff event += and -= are recorded in mock.Invocations, but previously this didn't happen, and that's why those verification methods weren't introduced for so long.

If we had simply started unconditionally recording += and -=, that would have been a breaking change with respect to e.g. VerifyNoOtherCalls: existing code might have assumed that this call would succeed but now there might suddenly be additional invocations that weren't there before.

That's why we figured, if your test is already aware of event (un-) subscriptions, it should be safe in that case to record them as invocations. And we chose the presence of the setup methods as the criterion whether your test is "aware of" event (un-) subscriptions.

Does this explanation make sense?

I agree that ideally, those Verify{Add,Remove} methods should work without their Setup... counterparts (as is otherwise the case), but it was simply the safest thing to do at the time. It enabled additional verification with only a small risk of breaking existing code.

@stakx stakx added this to the 4.15.0 milestone Oct 26, 2020
@stakx stakx self-assigned this Oct 26, 2020
mburumaxwell pushed a commit to faluapp/falu-dotnet that referenced this issue Jun 12, 2021
Bumps [Moq](https://github.com/moq/moq4) from 4.14.7 to 4.15.2.

#Changelog

*Sourced from [Moq's changelog](https://github.com/moq/moq4/blob/master/CHANGELOG.md).*

> ## 4.15.2 (2020-11-26)
>
> #### Changed
>
> * Upgraded `System.Threading.Tasks.Extensions` dependency to version 4.5.4 (@JeffAshton, [#1108](devlooped/moq#1108))
>
>
> ## 4.15.1 (2020-11-10)
>
> #### Added
>
> * New method overloads for `It.Is`, `It.IsIn`, and `It.IsNotIn` that compare values using a custom `IEqualityComparer<T>` (@weitzhandler, [#1064](devlooped/moq#1064))
> * New properties `ReturnValue` and `Exception` on `IInvocation` to query recorded invocations return values or exceptions (@MaStr11, [#921](devlooped/moq#921), [#1077](devlooped/moq#1077))
> * Support for "nested" type matchers, i.e. type matchers that appear as part of a composite type (such as `It.IsAnyType[]` or `Func<It.IsAnyType, bool>`). Argument match expressions like `It.IsAny<Func<It.IsAnyType, bool>>()` should now work as expected, whereas they previously didn't. In this particular example, you should no longer need a workaround like `(Func<It.IsAnyType, bool>)It.IsAny<object>()` as originally suggested in [#918](devlooped/moq#918). (@stakx, [#1092](devlooped/moq#1092))
>
> #### Changed
>
> * Event accessor calls (`+=` and `-=`) now get consistently recorded in `Mock.Invocations`. This previously wasn't the case for backwards compatibility with `VerifyNoOtherCalls` (which got implemented before it was possible to check them using `Verify{Add,Remove}`). You now need to explicitly verify expected calls to event accessors prior to `VerifyNoOtherCalls`. Verification of `+=` and `-=` now works regardless of whether or not you set those up (which makes it consistent with how verification usually works). (@80O, @stakx, [#1058](devlooped/moq#1058), [#1084](devlooped/moq#1084))
> * Portable PDB (debugging symbols) are now embedded in the main library instead of being published as a separate NuGet symbols package (`.snupkg) (@kzu, [#1098](devlooped/moq#1098))
>
> #### Fixed
>
> * `SetupProperty` fails if property getter and setter are not both defined in mocked type (@stakx, [#1017](devlooped/moq#1017))
> * Expression tree argument not matched when it contains a captured variable &ndash; evaluate all captures to their current values when comparing two expression trees (@QTom01, [#1054](devlooped/moq#1054))
> * Failure when parameterized `Mock.Of<>` is used in query comprehension `from` clause (@stakx, [#982](devlooped/moq#982))
>
>
> ## 4.15.0
>
> This version was accidentally published as 4.15.1 due to an intermittent problem with NuGet publishing.

#Commits

- [`f2aa090`](devlooped/moq@f2aa090) ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants