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

Let mock.Invocations.Clear() remove traces of earlier invocations more thoroughly #854

Merged
merged 3 commits into from
Aug 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1

* Improved error message that is supplied with `ArgumentException` thrown when `Setup` or `Verify` are called on a protected method if the method could not be found with both the name and compatible argument types specified (@thomasfdm, #852).

* `mock.Invocations.Clear()` now removes traces of previous invocations more thoroughly by additionally resetting all setups to an "unmatched" state. (@stakx, #854)

* Consistent `Callback` delegate validation regardless of whether or not `Callback` is preceded by a `Returns`: Validation for post-`Returns` callback delegates used to be very relaxed, but is now equally strict as in the pre-`Returns` case.) (@stakx, #876)

#### Added
Expand All @@ -19,7 +21,10 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1

#### Fixed

* `Invocations.Clear()` does not cause `Verify` to fail (@jchessir, #733)

* Regression: `SetupAllProperties` can no longer set up properties whose names start with `Item`. (@mattzink, #870; @kaan-kaya, #869)

* Regression: `MockDefaultValueProvider` will no longer attempt to set `CallBase` to true for mocks generated for delegates. (@dammejed, #874)


Expand Down
13 changes: 12 additions & 1 deletion src/Moq/InvocationCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Diagnostics;

namespace Moq
{
Expand All @@ -16,6 +16,14 @@ internal sealed class InvocationCollection : IInvocationList
private int count = 0;

private readonly object invocationsLock = new object();
private readonly Mock owner;

public InvocationCollection(Mock owner)
{
Debug.Assert(owner != null);

this.owner = owner;
}

public int Count
{
Expand Down Expand Up @@ -68,6 +76,9 @@ public void Clear()
this.invocations = null;
this.count = 0;
this.capacity = 0;

this.owner.Setups.UninvokeAll();
stakx marked this conversation as resolved.
Show resolved Hide resolved
// ^ TODO: Currently this could cause a deadlock as another lock will be taken inside this one!
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raises the question of whether Moq should generally perform more coarse-grained locking.

This would probably mean having just one lock object on the Mock instance that would be used both during setups and invocations.

Client code should probably perform all setups first, and only then start invoking the mocked object (possibly from several threads). Code that is structured that way already shouldn't be affected by a move to more coarse-grained locking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had already implemented this, but when I ran some benchmarking it turned out that more coarse-grained locking appears to be slower in general than if each collection kept its own locks.

It also appears that dead-locks currently cannot happen, because we don't have any cyclic call dependencies between SetupCollection and InvocationCollection (IIRC); but that could always change in the future if these collections are modified.

For now, let's leave this as a future TODO.

}
}

Expand Down
11 changes: 11 additions & 0 deletions src/Moq/MethodCall.cs
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,12 @@ public override MockException TryVerifyAll()
return (this.flags & Flags.Invoked) == 0 ? MockException.UnmatchedSetup(this) : null;
}

public override void Uninvoke()
{
this.flags &= ~Flags.Invoked;
this.limitInvocationCountResponse?.Reset();
}

public void Verifiable()
{
this.flags |= Flags.Verifiable;
Expand Down Expand Up @@ -388,6 +394,11 @@ public LimitInvocationCountResponse(MethodCall setup, int maxCount)
this.count = 0;
}

public void Reset()
{
this.count = 0;
}

public void RespondTo(Invocation invocation)
{
++this.count;
Expand Down
2 changes: 1 addition & 1 deletion src/Moq/Mock.Generic.cs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ public Mock(MockBehavior behavior, params object[] args)
this.constructorArguments = args;
this.defaultValueProvider = DefaultValueProvider.Empty;
this.eventHandlers = new EventHandlerCollection();
this.invocations = new InvocationCollection();
this.invocations = new InvocationCollection(this);
this.name = CreateUniqueDefaultMockName();
this.setups = new SetupCollection();
this.switches = Switches.Default;
Expand Down
4 changes: 4 additions & 0 deletions src/Moq/Setup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -109,5 +109,9 @@ public MockException TryVerifyInnerMock(Func<Mock, MockException> verify)

return null;
}

public virtual void Uninvoke()
{
}
}
}
11 changes: 11 additions & 0 deletions src/Moq/SetupCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,17 @@ public IEnumerable<Setup> GetInnerMockSetups()
return this.ToArrayLive(setup => setup.ReturnsInnerMock(out _));
}

public void UninvokeAll()
{
lock (this.setups)
{
foreach (var setup in this.setups)
{
setup.Uninvoke();
}
}
}

public Setup[] ToArrayLive(Func<Setup, bool> predicate)
{
var matchingSetups = new Stack<Setup>();
Expand Down
27 changes: 27 additions & 0 deletions tests/Moq.Tests/InvocationsFixture.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,5 +128,32 @@ public void Invocations_for_object_methods_on_interface_proxy_record_return_valu
var invocation = mock.MutableInvocations.ToArray()[0];
Assert.Equal(42, invocation.ReturnValue);
}

[Fact]
public void Invocations_Clear_also_resets_setup_verification_state()
{
var mock = new Mock<IComparable>();
mock.Setup(m => m.CompareTo(default));
_ = mock.Object.CompareTo(default);
mock.VerifyAll(); // ensure setup has been matched

mock.Invocations.Clear();
var ex = Assert.Throws<MockException>(() => mock.VerifyAll());
Assert.Equal(MockExceptionReasons.UnmatchedSetup, ex.Reasons);
}

[Fact]
[Obsolete()]
public void Invocations_Clear_resets_count_kept_by_setup_AtMost()
{
var mock = new Mock<IComparable>();
mock.Setup(m => m.CompareTo(default)).Returns(0).AtMostOnce();
_ = mock.Object.CompareTo(default);

mock.Invocations.Clear();
_ = mock.Object.CompareTo(default); // this second call should now count as the first
var ex = Assert.Throws<MockException>(() => mock.Object.CompareTo(default));
Assert.Equal(MockExceptionReasons.MoreThanOneCall, ex.Reasons);
}
}
}