Fast fail for internal interfaces makes unit testing Entity Framework break.

[KarlZ]

It seems the change to cause "fast failure" for interfaces that are internal has caused many of our tests to fail. With the Entity Framework (EF) it seems Microsoft makes heavy use if internal interfaces and the previous code without the access check worked fine.
Borrowing from the work of others, we have a class called MockDbSet. This class allows us to setup and mock a DbSet in an EF DbContext. Before this strategy we had lots of code that could only be tested by hitting a real database. We currently have 2500 unit tests that leverage the MockDbSet class and they all work on 4.7.99 and 4.7.127. But 250 unit tests fail with 4.7.137 and higher. It seems that the call to ThrowIfSetupMethodNotVisibleToProxyFactory (127) combined with "always returning the exact same proxy object" (137) causes this.
Is there a suggested pattern to follow to allow MockDbSet to continue to work? If not, is there a way we could override the behavior and allow the use of internal interfaces?

Here is the code for MockDbSet:

using Moq;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Data.Entity;
using System.Data.Entity.Infrastructure;
using System.Linq;

namespace Entities
{
    // References:
    //  http://codethug.com/2015/03/20/mocking-dbset/
    //  https://msdn.microsoft.com/en-us/data/dn314431.aspx
    public sealed class MockDbSet<T> : Mock<DbSet<T>> where T : class
    {
        public IList<T> Items { get; }

        /// <summary>
        /// Use this constructor if you just want an empty DbSet for the type T.
        /// </summary>
        public MockDbSet()
            : this(new List<T>())
        { }

        /// <summary>
        /// Use this constructor if you have just one item you want in the DbSet.
        /// </summary>
        /// <param name="entity"></param>
        public MockDbSet(T entity)
            : this(new List<T> { entity })
        { }

        /// <summary>
        ///  Use this constructor if you have a list of items you want to have in the DbSet.
        /// </summary>
        /// <param name="items"></param>
        public MockDbSet(IList<T> items)
        {
            Items = items;
            var itemsAsQueryable = items.AsQueryable();
            As<IQueryable<T>>().Setup(m => m.Provider).Returns(itemsAsQueryable.Provider);
            As<IQueryable<T>>().Setup(m => m.Expression).Returns(itemsAsQueryable.Expression);
            As<IQueryable<T>>().Setup(m => m.ElementType).Returns(itemsAsQueryable.ElementType);
            As<IQueryable<T>>().Setup(m => m.GetEnumerator()).Returns(items.GetEnumerator());
            As<IQueryable>().Setup(m => m.GetEnumerator()).Returns(items.GetEnumerator());
            //Setup(m => m.Find(It.IsAny<object[]>())).Returns(i => Items.FirstOrDefault(d => d.Id))
            Setup(m => m.Include(It.IsAny<string>())).Returns(Object);
            Setup(m => m.Add(It.IsAny<T>())).Returns<T>(i => { items.Add(i); return i; });
            Setup(m => m.Remove(It.IsAny<T>())).Returns<T>(i => { items.Remove(i); return i; });
            Setup(m => m.AddRange(It.IsAny<IEnumerable<T>>())).Returns<IEnumerable<T>>((i) =>
            {
                foreach (var item in i)
                    items.Add(item);
                return i;
            });
            Setup(m => m.Local).Returns(new ObservableCollection<T>(items));
            Setup(m => m.Find(It.IsAny<int>())).Returns(
                (object[] id) => items.Single(item => item.GetPropertyValue<int>("Id") == ((int)id[0])));

            // Async Support (e.g. ToListAsync())
            // See https://msdn.microsoft.com/en-us/data/dn314429#Testing-with-async-queries
            As<IDbAsyncEnumerable<T>>()
                .Setup(m => m.GetAsyncEnumerator())
                .Returns(new TestDbAsyncEnumerator<T>(itemsAsQueryable.GetEnumerator()));
            As<IQueryable<T>>()
                .Setup(m => m.Provider)
                .Returns(new TestDbAsyncQueryProvider<T>(itemsAsQueryable.Provider));
            //As<IQueryable<T>>().Setup(m => m.GetEnumerator()).Returns(itemsAsQueryable.GetEnumerator());
        }
    }

    public static class MockDbSet
    {
        /// <summary>
        /// Returns a Moq of an IDbSet using the given collection.
        /// </summary>
        public static MockDbSet<T> Create<T>(IList<T> items) where T : class
            => new MockDbSet<T>(items);

        /// <summary>
        /// Returns mocked DbSet. (Not the Moq.Mock)
        /// </summary>
        public static DbSet<T> With<T>(IList<T> items) where T : class
            => Create(items).Object;
    }
}

Attached is a very simple test project with one class library that has demonstration tests that run fine with Moq 4.7.127 but fail with Moq 4.7.137.
MockDbSetIssue.zip

[stakx]

Hi @KarlZ. Thanks for taking the time to send a detailed error report, and for providing repro code that works right away. 👍

It seems that the call to ThrowIfSetupMethodNotVisibleToProxyFactory (127) combined with "always returning the exact same proxy object" (137) causes this.

You are definitely right regarding the first (I'll tell you why in a second). But the latter seems unrelated. Is there a reason why you suspect it, too?

I believe it is correct that Moq should let you know (by throwing an exception) when you're trying to set up something that it can't mock / intercept—such as inaccessible methods. But perhaps it shouldn't do the same (throw an exception) when it sets up all members of a type, or even a whole object graph, due to some internal automatism. In that latter case, perhaps it should simply ignore what it can't set up.

The automatic setting up of all members of a type happens e.g. when you call mock.SetupAllProperties(). The same call also happens, possibly without you realising it, when you use dotted / chained expressions à la c.People.Add(...) in Setup or Verify calls.

Let's for example take your Add_Never_Called test:

context.Setup(c => c.People).Returns(() => set.Object);
...
context.Verify(c => c.People.Add(It.IsAny<Person>()), Times.Never);

Note your using a chained / dotted expression, c.People.Add(...). (IIRC, Moq traditionally calls that kind of thing "recursive mocking".) Your other failing tests use those, too. If you got rid of that by changing the last line to the following:

set.Verify(people => people.Add(It.IsAny<Person>()), Times.Never);

Your test would suddenly pass. Again, this is because processing such chained expressions can cause a hidden SetupAllProperties call, which will hit the internal interfaces, which in turn will cause the "inaccessible" exception to be thrown.

Please note that I am not recommending that you rewrite all of your tests. (Of course, if you want to use the latest version of Moq instead of waiting for a corrected version, the above should give you enough hints to fix your tests.) I suggest that perhaps Moq's current behaviour needs a minor adjustment, namely that it should complain about inaccessible members only if you've explicitly asked Moq to set up such a one.

P.S.: I'm not 100 % convinced of my own last suggestion. Perhaps Moq is actually doing the right thing right already, and the real issue is either (a) that mocking DbSet<T> and DbContext correctly simply is that hard to get right, due to them being very complex types, or (b) that dotted expressions such as your c.People.Add(...) really have no business calling SetupAllProperties on any intermediate objects. Regarding the latter, see #426 (and, to a lesser degree, #424).

[KarlZ]

I guess a quick question. You said, "...something that it can't mock/intercept -- such as inaccessible methods." But I think in our case, we are trying to mock an internal interface, so we should never reach the internal method? So is it OK to mock an internal interface, but not an internal method?

[stakx]

So is it OK to mock an internal interface, but not an internal method?

Both is OK, if they are made accessible to Castle DynamicProxy (using the [assembly: InternalsVisibleTo(...)] custom attribute; see the quickstart guide). And neither will work if they are not.

Note that members such as types and methods can be discoverable (using reflection) even when they are inaccessible. Inaccessible in the sense of what you can actually do with a member (calling it, overriding it, inheriting it, implementing it, etc.).

Let me try to lay out the rules (and sorry if there are minor inaccuracies, there are several edge cases to consider that I might not remember right now):

• Creating a mock of type T requires T to be accessible. When you create a mock of type T, what you will receive from mock.Object isn't actually a instance of T itself, but an instance of a subclass of T. Moq creates a dynamic type inheriting / implementing T (using DynamicProxy). For that reason, T itself must be accessible. After all, you cannot inherit / implement something which you don't have access to.

• Setting up a method M of type T requires both T and M to be accessible. When you set up a method M of type T, M must also be accessible because it needs to be either implemented (if it's an interface method), or overridden (if it's a virtual method). You cannot implement or override a method to which you don't have access. A method can be inaccessible either because it's private or internal, or because its containing type is inaccessible.

In your specific case (tests involving Entity Framework), [assembly: InternalsVisibleTo(...)] won't help, because that custom attribute would have to be placed inside the Entity Framework assemblies.

[stakx]

P.S.: If Entity Framework Core is an option for you at all, perhaps look into its InMemory provider, which can be used in testing scenarios instead of a real DB. Using that would possibly be a lot cleaner than mocking DbContexts.

[KarlZ]

I guess I didn't answer your question,

You are definitely right regarding the first [fast fail exception in 127] (I'll tell you why in a second). But the latter [exact same object returned in 137] seems unrelated. Is there a reason why you suspect it, too?

The reason I suspect it is the combination of the two is that all our tests work on 127, which includes the exception. It isn't until 137 that our tests fail. So 4.7.99 all works... same on *.127, but *.137 and *.142 both cause these test failures.
We also used this to test some of our code that must use the DbSet<T>.Local property. I'm not sure I see a workaround for that at the moment.

[stakx]

The reason I suspect it is the combination of the two is that all our tests work on 127, which includes the exception. It isn't until 137 that our tests fail. So 4.7.99 all works... same on *.127, but *.137 and *.142 both cause these test failures.

I see. If indeed #460 broke Entity Framework mocking, then I suspect that mocking Entity Framework DbContext etc. relied on buggy behaviour. It wouldn't be the first time this happened.

[...] test some of our code that must use the DbSet<T>.Local property. I'm not sure I see a workaround for that at the moment.

Perhaps I'm misunderstanding you, but the workaround is the same: get rid of chain expressions by splitting them up. For example, you can get your Person_Returned_Local test to pass by replacing this:

context.Setup(c => c.People.Local).Returns(local);

with this:

var people = new Mock<DbSet<Person>>();
people.Setup(p => p.Local).Returns(local);
context.Setup(c => c.People).Returns(() => people.Object);

(Again, I'm not recommending that you rewrite all your tests that way, it would obviously affect easy of understanding. I only want to point out that it's feasible.)

[stakx]

This is a duplicate of #498 (and fixed by #499).

[stakx]

@KarlZ - I just published a pre-release version 4.8.0-rc1 on NuGet. The fail-fast issue when mocking Entity Framework types that you reported should now be fixed. Feel free to test.

[KarlZ]

@stakx Works great! I'm looking forward to that release. Thanks for your help.

[KarlZ]

@stakx My unit tests (>9000 of them) run in ~3 min on 4.8.0-rc1, but take over 7 minutes on 4.7.99. I went back and forth between these and have these times for 4.7.99 of 7:58 and 7:36, but on 4.8.0-rc1 have 2:50 and 3:03. I noticed back on October 20 that my test times more than doubled and have been blaming Visual Studio. (Reported here on Developer Community )
Is there a reason I would expect the test runs to be so much faster? I couldn't really tell since there have been lots of changes here.
I usually don't run with rc code, but since this is our tests and not the release code AND since I can do tests so much faster I changed. On my Surface Pro they went from 8 min down to 4. I did notice that the upgrade did contain a reference to System.Threading.Tasks.Extentions. So perhaps some work done with parallel work?

Curious and grateful!
Karl

[stakx]

I noticed back on October 20 that my test times more than doubled [...].

The most likely explanation for this is that you received the .NET Framework 4.7 (in-place) update around that time. .NET 4.7 ships with a regression that affected Moq very badly—see #500, which was posted on Oct 23). I then published a hotfix release (4.7.145; see #500 (comment)) that includes a workaround for this regression.

If you want to avoid pre-release versions, you might be just fine with 4.7.145.

That being said, I have been working on Moq's performance, and the final 4.8.0 should be even faster than the current pre-release! Just today, I discovered what appears to be a major bottleneck that Moq has suffered from since version 4.5.16. Right now, I am looking at a test suite that, once I apply my fix, ends up running twice as fast! 😮

I don't want to over-promise at this time; perhaps you won't see anything quite as drastic. Do stay tuned, though! 😀