From ec385a6d125beb59ce8c0c511466deccc29728b0 Mon Sep 17 00:00:00 2001 From: stakx Date: Tue, 27 Oct 2020 20:06:11 +0100 Subject: [PATCH 1/3] Fix remaining event subscription inconsistency ... by matching and executing setups before event (un-) subscription is dealt with. This has the added benefit of making the interception pipeline more efficient for almost all invocations. --- src/Moq/Interception/Mock.cs | 10 +++++----- src/Moq/MethodCall.cs | 4 ++++ tests/Moq.Tests/EventHandlersFixture.cs | 6 +++--- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/Moq/Interception/Mock.cs b/src/Moq/Interception/Mock.cs index 60d3f1afc..20baedbde 100644 --- a/src/Moq/Interception/Mock.cs +++ b/src/Moq/Interception/Mock.cs @@ -14,11 +14,6 @@ void IInterceptor.Intercept(Invocation invocation) RecordInvocation.Handle(invocation, this); - if (HandleEventSubscription.Handle(invocation, this)) - { - return; - } - if (FindAndExecuteMatchingSetup.Handle(invocation, this)) { return; @@ -29,6 +24,11 @@ void IInterceptor.Intercept(Invocation invocation) return; } + if (HandleEventSubscription.Handle(invocation, this)) + { + return; + } + FailForStrictMock.Handle(invocation, this); Return.Handle(invocation, this); diff --git a/src/Moq/MethodCall.cs b/src/Moq/MethodCall.cs index 9967c5f73..02209c4e4 100644 --- a/src/Moq/MethodCall.cs +++ b/src/Moq/MethodCall.cs @@ -105,6 +105,10 @@ protected override void ExecuteCore(Invocation invocation) new ReturnBaseOrDefaultValue(this.Mock).Execute(invocation); } } + else + { + HandleEventSubscription.Handle(invocation, this.Mock); // no-op for everything other than event accessors + } this.afterReturnCallback?.Execute(invocation); } diff --git a/tests/Moq.Tests/EventHandlersFixture.cs b/tests/Moq.Tests/EventHandlersFixture.cs index e79526986..8a591c4fd 100644 --- a/tests/Moq.Tests/EventHandlersFixture.cs +++ b/tests/Moq.Tests/EventHandlersFixture.cs @@ -82,15 +82,15 @@ public void Raising_event__using_Raise__triggers_handler__if_setup_without_CallB Assert.True(handled); } - [Fact] // this is inconsistent with the above test `Raising_event__using_Raise__does_not_trigger_handler__if_CallBase_true` - public void Raising_event__using_Raise__triggers_handler__if_setup_with_CallBase_present() + [Fact] + public void Raising_event__using_Raise__does_not_trigger_handler__if_setup_with_CallBase_present() { var handled = false; var mock = new Mock(); mock.SetupAdd(m => m.Event += It.IsAny()).CallBase(); mock.Object.Event += () => handled = true; mock.Raise(m => m.Event += null); - Assert.True(handled); + Assert.False(handled); } [Fact] From ae8a42059a8a6793133128c1419c2b2bb284e886 Mon Sep 17 00:00:00 2001 From: stakx Date: Tue, 27 Oct 2020 20:21:28 +0100 Subject: [PATCH 2/3] Event subscription no longer needs to check setups because by the time we get to `HandleEventSubscription`, any setups would already have executed. --- src/Moq/Interception/InterceptionAspects.cs | 22 ++++++--------------- src/Moq/SetupCollection.cs | 16 --------------- tests/Moq.Tests/MockFixture.cs | 14 ------------- 3 files changed, 6 insertions(+), 46 deletions(-) diff --git a/src/Moq/Interception/InterceptionAspects.cs b/src/Moq/Interception/InterceptionAspects.cs index 35f618ed0..114c2b3dd 100644 --- a/src/Moq/Interception/InterceptionAspects.cs +++ b/src/Moq/Interception/InterceptionAspects.cs @@ -132,21 +132,16 @@ public static bool Handle(Invocation invocation, Mock mock) var @event = implementingMethod.DeclaringType.GetEvents(bindingFlags).SingleOrDefault(e => e.GetAddMethod(true) == implementingMethod); if (@event != null) { - bool doesntHaveEventSetup = !mock.MutableSetups.HasEventSetup; - if (mock.CallBase && !invocation.Method.IsAbstract) { - if (doesntHaveEventSetup) - { - invocation.ReturnValue = invocation.CallBase(); - } + invocation.ReturnValue = invocation.CallBase(); + return true; } else if (invocation.Arguments.Length > 0 && invocation.Arguments[0] is Delegate delegateInstance) { mock.EventHandlers.Add(@event, delegateInstance); + return true; } - - return doesntHaveEventSetup; } } else if (methodName[0] == 'r' && methodName.Length > 7 && methodName[6] == '_' && invocation.Method.IsEventRemoveAccessor()) @@ -155,21 +150,16 @@ public static bool Handle(Invocation invocation, Mock mock) var @event = implementingMethod.DeclaringType.GetEvents(bindingFlags).SingleOrDefault(e => e.GetRemoveMethod(true) == implementingMethod); if (@event != null) { - bool doesntHaveEventSetup = !mock.MutableSetups.HasEventSetup; - if (mock.CallBase && !invocation.Method.IsAbstract) { - if (doesntHaveEventSetup) - { - invocation.ReturnValue = invocation.CallBase(); - } + invocation.ReturnValue = invocation.CallBase(); + return true; } else if (invocation.Arguments.Length > 0 && invocation.Arguments[0] is Delegate delegateInstance) { mock.EventHandlers.Remove(@event, delegateInstance); + return true; } - - return doesntHaveEventSetup; } } } diff --git a/src/Moq/SetupCollection.cs b/src/Moq/SetupCollection.cs index a022e476a..c80dac9aa 100644 --- a/src/Moq/SetupCollection.cs +++ b/src/Moq/SetupCollection.cs @@ -11,12 +11,10 @@ namespace Moq internal sealed class SetupCollection : ISetupList { private List setups; - private volatile bool hasEventSetup; public SetupCollection() { this.setups = new List(); - this.hasEventSetup = false; } public int Count @@ -30,14 +28,6 @@ public int Count } } - public bool HasEventSetup - { - get - { - return this.hasEventSetup; - } - } - public ISetup this[int index] { get @@ -53,11 +43,6 @@ public void Add(Setup setup) { lock (this.setups) { - if (setup.Method.IsEventAddAccessor() || setup.Method.IsEventRemoveAccessor()) - { - this.hasEventSetup = true; - } - this.setups.Add(setup); this.MarkOverriddenSetups(); @@ -116,7 +101,6 @@ public void Clear() lock (this.setups) { this.setups.Clear(); - this.hasEventSetup = false; } } diff --git a/tests/Moq.Tests/MockFixture.cs b/tests/Moq.Tests/MockFixture.cs index 5793b7f25..6c7fdb152 100644 --- a/tests/Moq.Tests/MockFixture.cs +++ b/tests/Moq.Tests/MockFixture.cs @@ -1279,20 +1279,6 @@ public void Reset_clears_configured_default_return_values() Assert.Empty(mock.ConfiguredDefaultValues); } - [Fact] - public void Reset_clears_event_setup_flag() - { - var mock = new Mock(); - mock.SetupAdd(m => m.EventHandler += It.IsAny()); - - var before = mock.MutableSetups.HasEventSetup; - mock.Reset(); - var after = mock.MutableSetups.HasEventSetup; - - Assert.True(before, "Before reset"); - Assert.False(after, "After reset"); - } - #if FEATURE_DYNAMICPROXY_SERIALIZABLE_PROXIES [Serializable] public class BadSerializable : ISerializable From 77328dc8a7ec78601d41ce31f451b9adef9435ca Mon Sep 17 00:00:00 2001 From: stakx Date: Tue, 27 Oct 2020 20:27:11 +0100 Subject: [PATCH 3/3] Update the changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f935cab6..00818f5a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1 #### 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) +* 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, #1084) #### Fixed