Expose invocations

[Code-Grump]

Second take at exposing invocation information.
Invocation information exposed via IReadOnlyInvocation interface.
Mock invocations exposed via property IReadOnlyList<IReadOnlyInvocation> Invocations on Mock class.

[Code-Grump]

1. Define new interface types IReadOnlyInvocation and IReadOnlyInvocationList in namespace Moq:

public IReadOnlyInvocationList : IReadOnlyList<IReadOnlyInvocation>
{
    IReadOnlyInvocation[] ToArray();
}

public interface IReadOnlyInvocation
{
    IReadOnlyList<object> Arguments { get; }
    MethodInfo Method { get; }
}

I've created the IReadOnlyInvocation interface. I haven't created the IReadOnlyInvocationsList interface as yet.

2. Let Invocation implement IReadOnlyInvocation. The two properties trivially map to Invocation.Arguments and Invocation.Method. (For the former, explicit implementation is required.)

Added implementation to Invocation; I'm not clear whether we need to copy-on-read the Arguments property.

3. Let InvocationCollection implement IReadOnlyInvocationList. The interface accesses the underlying List directly without copying, but the implementation should take care to perform synchronization properly (e.g. by proper lock-ing and maintaining a version that gets updated by all mutating methods; if version changes during enumeration, a InvalidOperationException is thrown).

(The reason for declaring a dedicated interface instead of just using IReadOnlyList directly is the additional ToArray method; unlike LINQ's ToArray extension method, it can ensure proper synchronization for safely copying the whole collection.)

I've made InvocationCollection implement IReadOnlyList<IReadOnlyInvocation>. I've used a copy-on-read strategy to make sure the enumerator is tolerant of changes to the collection. This should make any Linq extensions safe to call, including ToArray().

4. Rename the existing internal property Mock.Invocations to something else, e.g. InternalInvocations or MutableInvocations. (This in Mock, Mock, and AsInterface and any other subclasses of Mock I might be forgetting about right now.)

I performed a Refactor->Rename operation on the property to call it MutableInvocations; I believe I have caught all references, to the property, unless there's some scary string-based reflection happening somewhere that I would've missed.

5. This allows us to then add a public property, public IReadOnlyInvocationList Invocations { get; }. For Mock<T>, this returns this.InternalInvocations. For AsInterface<T>, it returns this.owner.Invocations.

As mentioned, the property is implemented as IReadOnlyList<IReadOnlyInvocation> Invocations { get; } at present.

[stakx]

Hi @Tragedian, nice to hear back from you! I think your PR is in a mergable state as it is. Some feedback:

I performed a Refactor->Rename operation on the property to call it MutableInvocations; I believe I have caught all references, to the property, unless there's some scary string-based reflection happening somewhere that I would've missed.

Looks good to me. I believe you've caught all occurrences of Invocation with the rename.

Added implementation to Invocation; I'm not clear whether we need to copy-on-read the Arguments property.

Looks good to me. Copying Arguments on read shouldn't be necessary, since Invocation objects in InvocationCollection can be considered completed and therefore "frozen".

I've made InvocationCollection implement IReadOnlyList<IReadOnlyInvocation>. I've used a copy-on-read strategy to make sure the enumerator is tolerant of changes to the collection. This should make any Linq extensions safe to call, including ToArray().

Quick question, is there a reason why you opted not to make Invocations non-copying? (I grant you that your approach is absolutely fine from a practical perspective. I would prefer a non-copying approach however, because I think mock.Invocations should ideally act like any other collection in .NET and throw when a change happens during enumeration. That's why I suggested the additional interface with a .ToArray() method, which would make copying an explicit action.)

A small look ahead: If Moq 5 is going to take much longer than expected, I might clean up the Moq 4 API a little bit. This may encompass, among other things, replacing the mock.ResetCalls() extension method in favor of a new mock.Invocations.Clear() method. In that case, I might opt to rework some of what you're adding here (e.g. introducing some IInvocationList interface with the necessary methods, in place of IReadOnlyList<IReadOnlyInvocation>). However, that's of yet undecided, so I think we're good to go. I just thought I'd mention it so you won't think I'm changing things behind your back just to get it my way. 😄

[stakx]

Sorry, I do have two minor change requests after all:

First, please add an entry to CHANGELOG.md at the top, like this:

 # Moq Changelog

 All notable changes to this project will be documented in this file.

 The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).


+## Unreleased
+
+#### Added
+
+ * Description of the new feature (@informatorius, #627)
+
+
 ## 4.8.3 (2018-06-09)

Second, if you find the time, it would be nice to have a few unit tests documenting the new feature.

[Code-Grump]

Quick question, is there a reason why you opted not to make Invocations non-copying?

It's the lock statements.

I'm assuming the collection is heavily synchronised internally because there's an expectation it will be modified and read from multiple concurrent executions. On this assumption, the simplest implementation is a copy-on-read model which allows consumers to read their copy of the collection whilst allowing modifications to proceed on the synchronised collection.

This is much better to manage for a consumer than an enumerator which is always racing with potential modifications and has to be coded around failing at any moment.

It is a sub-optimal implementation of this pattern because it will produce allocations which aren't really needed. There are alternatives we could explore around using a copy-on-write method, or using some form of immutable collection internally (which would significantly help on allocations). It's implemented this way for simplicity, with an eye on potential future improvements without breaking behaviour contracts.

[stakx]

It's the lock statements.

Agreed, I am not exactly thrilled to have them, either. The reason they are there (for now) is because (IIRC) something like a ConcurrentQueue<Invocation> actually performs worse than a combination of List<Invocation> and locks in single-threaded scenarios. The benefits of .NET's concurrent collections might only become apparent when there actually is some degree of concurrency.

I just ran a quick experiment with ConcurrentQueue<Invocation> again. I only discovered now that my expectation that all collections throw when a change happens during enumeration was naïve; this is (obviously) no longer true for concurrent collections. Enumerating over a ConcurrentQueue<> sees a snapshot of the collection made at the time when enumeration started—which is exactly what you implemented, also.

So in summary, I'm convinced. Your implementation will allow us to later get rid of the locks by moving to a ConcurrentQueue<Invocation> without any externally observable change.

Thanks for talking me through this. Always good to have one's assumptions challenged. 👍

[stakx]

P.S.: Perhaps my look towards ConcurrentQueue<> is also naïve? If you can recommend a better way to get rid of the locks in InvocationCollection, I'd be interested to learn about it.

[stakx]

Merging... thank you! I'll try to release Moq 4.9.0 reasonably quickly; say, in approx. 3 weeks' time. (I'd like to allow some time to gather feedback on possible issues in 4.8.3.)

Like I mentioned above, if you have any insights regarding concurrency that you'd like to share here, I'm happy to listen.

[Code-Grump]

I'd have to ask why you'd want to remove those lock statements. I personally have no problems with them; they're simple to understand and (just as you say) very fast when resources aren't contested.

I do have slight "this-could-be-better" pains with allocating memory every time a read is performed, especially if there's no concurrency in that scenario. I'd need to understand what scenario is most common and optimise for that. If the 99% case is where there is no concurrency, it would be better to not create a copy on every read. If the collection only has to support append and clear functions, using an mutable collection protected by locks and implementing a Range enumeration over the collection would probably give the best balance of speed and memory use.

public IEnumerator<IReadOnlyInvocation> GetEnumerator()
{
    lock (this.invocations)
    {
        return this.invocations.Take(this.invocations.Count).GetEnumerator();
    }
}

The Clear() method would have to replace the existing collection with a new one, so existing enumerators could still proceed.

[Code-Grump]

If you still want those unit tests, I will try to put together some.

[stakx]

If you still want those unit tests, I will try to put together some.

That would be good, if you find the time. 👍 (I decided to merge your PR without tests anyway because I really wanted to see this through—I've held this back way too long, and it's a fairly straightforward feature after all. :)

If the 99% case is where there is no concurrency, it would be better to not create a copy on every read.

I don't have numbers—perhaps we should look at some user projects such as ASP.NET that uses Moq to gather actual data—but I suspect that "no concurrency" is probably the predominant scenario. Perhaps not quite a ≥99% case, though.

Yes, InvocationCollection is indeed an append-and-clear-only collection, so your suggestion of a range-based enumeration and Clear replacing the previous collection entirely makes good sense to me. I'll leave it up to you whether (and when) to change the implementation.

[Code-Grump]

Tests and collection changes: #628