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

NullReferenceException when calling Verify() on a mock of DbContext with a mocked Set<> property #464

Closed
Eregrith opened this issue Oct 2, 2017 · 12 comments

Comments

@Eregrith
Copy link

Eregrith commented Oct 2, 2017

The call to Verify() raises a NullReferenceException when trying to format Performed Invocations with the following setup :

Mock<MyDbContext> mockDbContext = new Mock<MyDbContext>();
Mock<DbSet<MyEntity>> mockDbSet = new Mock<DbSet<MyEntity>>();
mockDbContext.Setup(m => m.Set<MyEntity>()).Returns(mockDbSet.Object);
var triggerObjectCreation = mockDbContext.Object; //This triggers the call to Set<MyEntity> because of the inheritance of MyDbContext (inheriting EntityFramework DbContext)

mockDbContext.Verify(m => m.SaveChanges(), Times.Once); //Should throw verify exception because no calls were made

Contrary to what's expected (verify exception with a summary of performed invocations), I have a NullReferenceException :

à System.Linq.Enumerable.<OfTypeIterator>d__92`1.MoveNext()
   à System.Linq.Enumerable.WhereSelectEnumerableIterator`2.MoveNext()
   à System.String.Join(String separator, IEnumerable`1 values)
   à Moq.Extensions.GetValue(Object value)
   à Moq.Extensions.Format(ICallContext invocation)
   à Moq.Mock.<>c.<FormatInvocations>b__63_0(ICallContext i)
   à System.Linq.Enumerable.WhereSelectListIterator`2.MoveNext()
   à System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   à System.Linq.Enumerable.ToArray[TSource](IEnumerable`1 source)
   à Moq.Mock.FormatInvocations(IEnumerable`1 invocations)
   à Moq.Mock.ThrowVerifyException(MethodCall expected, IEnumerable`1 setups, IEnumerable`1 actualCalls, Expression expression, Times times, Int32 callCount)
   à Moq.Mock.VerifyCalls(Interceptor targetInterceptor, MethodCall expected, Expression expression, Times times)
   à Moq.Mock.Verify[T,TResult](Mock`1 mock, Expression`1 expression, Times times, String failMessage)
   à Moq.Mock`1.Verify[TResult](Expression`1 expression, Times times)

I managed to browse the code here and locate the place of the alleged error :

https://github.com/moq/moq4/blob/61c35056dda76a157dd78cd9578cee86f7a16f02/Source/Extensions.cs#L91-L100

Apparently the Select triggers the cast and takes enumerable.Cast<object>().Take(maxCount + 1); and objs.Take(maxCount).

Note the inconspicuous comment just two lines above:

// This second check ensures that we have a usable implementation of IEnumerable.
// If value is a mocked object, its IEnumerable implementation might very well
// not work correctly.

Is it possible that is the value is mocked (which it is), the check on GetEnumerator() is not enough ?
Just out of curiosity (and despair) I tried to set it up like this:

Mock<MyDbContext> mockDbContext = new Mock<MyDbContext>();
Mock<DbSet<MyEntity>> mockDbSet = new Mock<DbSet<MyEntity>>();
mockDbSet .As<IEnumerable<MyEntity>>().Setup(m => m.GetEnumerator()).Returns(new List<MyEntity>().GetEnumerator()); // Why not
mockDbContext.Setup(m => m.Set<MyEntity>()).Returns(mockDbSet.Object);
var triggerObjectCreation = mockDbContext.Object; //This triggers the call to Set<MyEntity> because of the inheritance of MyDbContext (inheriting EntityFramework DbContext)

mockDbContext.Verify(m => m.SaveChanges(), Times.Once); //Should throw verify exception because no calls were made

But I have exactly the same exception, so I don't know if the mock's enumerable's implementation is at fault

@stakx
Copy link
Contributor

stakx commented Oct 2, 2017

I don't know if the mock's enumerable's implementation is at fault

Good question, I'll have to take a closer look at this.

Until I do, let me say that there's ultimately the problem that Moq internally uses your mock's implementation of IEnumerable. This shouldn't happen: Moq shouldn't have to use your setups to get its work done. Unfortunately, this problem is unlikely to be fixed in a short timeframe.

@Eregrith
Copy link
Author

Eregrith commented Oct 2, 2017

Hmm :/

Is there any way to get (or build) a debuggable version of the package? I tried checking out the repo but apparently my VS2017 won't load most of the csproj :/ I'll have to have a look at what's missing

@stakx
Copy link
Contributor

stakx commented Oct 2, 2017

Sure, the Moq.sln really should be openable in VS 2017 without any special configuration. Can't say what goes wrong on your machine. Some notes:

  • You can unload the test projects and the two numbered class libraries (they're also test related) if all you want is look around & tinker a bit.
  • There's some projects in the working directory that aren't included in the solution anymore (such as the debugger visualizer, some sample projects I think, and SandCastle stuff). They've probably all gone stale by now, I'd love to clean those up but am reluctant to just throw them away, as I'm not the original author.

Regarding the code questions you raised, the whole check against IEnumerator and the value returned by its GetEnumerator is a quick hacky fix, and far from a perfect one at that. (I know because I added one of them and that three-line comment. ;-)

A good solution would be to change Moq such that this code path will never get executed by Moq's internals; the solution should NOT be (IMHO) to bolt on even more safeguards. In other words, it might be better to take away code instead of adding more.

If you look into this, you're likely to stumble upon code paths that take you through ExpressionStringBuilder and ExpressionKey, two classes that play a fairly important role with the way how Moq stores and looks up a mock's configured setups. That's why it might not be trivial making a change in the piece of code you quoted without causing some surprising side effects.

I'll take a look at your repro code tomorrow, but please feel free to post back your own findings or to propose a fix!

@Eregrith
Copy link
Author

Eregrith commented Oct 2, 2017

Thanks ! I'll try again to get a debug dll in order to reproduce it in place, then maybe try to produce a sscce to drop by. If I manage to get my hands on good intel, i'll bring the guns :)

Thanks for your answers, I'll let you know if I find anything, maybe make a PR if I think I can fix it ^^

@stakx
Copy link
Contributor

stakx commented Oct 3, 2017

Contrary to what's expected (verify exception with a summary of performed invocations), I have a NullReferenceException

Here's the result I get with your repro code:

Moq.MockException:
Expected invocation on the mock once, but was 0 times: m => m.SaveChanges()

No setups configured.

No invocations performed.
  • What NuGet package and version are you using for Entity Framework? I used EntityFramework version 6.1.3.
  • Do you get the NullReferenceException regardless of whether you're debugging & stepping through your code, or just running it? (If you only get it while stepping through your code, be aware that debugger watches can cause mock invocations, too.)

@Eregrith
Copy link
Author

Eregrith commented Oct 3, 2017

Hmmm I think the difference mainly comes from

No invocations performed.

Apparently, when the DbSet is not mocked I have this in the list of Invocations Performed :

Moq.MockException : 
Expected invocation on the mock once, but was 0 times: m => m.SaveChanges()
No setups configured.

Performed invocations:
DbContext.Set<MyEntity>()
MyDbContext.MyEntity = null

And this only happens if i trigger the object creation through calling mockDbContext.Object

I don't know yet why those invocations are performed, but I'll bet those are the problem, as when Set is setup to return another mock's object it fails.
Please note too that when I set it up like this:

mockDbContext.Setup(m => m.Set<MyEntity>()).Returns(null);

I have no problem with getting the expected result (VerifyException and "Expected invocation but was 0 times" etc.)

I use :

  • Moq v4.7.10
  • EntityFramework v6.1.3

EntityFramework is use in Code First mode with EnabledMigrations set to true but AutomaticMigrations set to false.

the db context class looks like this:

    public partial class MyDbContext : DbContext
    {
        public MyDbContext()
            : base("name=MyDbContext")
        {
        }
        
        public virtual DbSet<MyEntity> MyEntity { get; set; }

        protected override void OnModelCreating(DbModelBuilder modelBuilder)
        {
            [...]
        }
    }

I highly suspect the constructor calling base() is what triggers those calls to Set

@stakx
Copy link
Contributor

stakx commented Oct 3, 2017

I highly suspect the constructor calling base() is what triggers those calls to Set

I can't verify your suspicion right now, but whenever constructors get involved, it helps to remember that they might call virtual functions that you haven't yet set up. So it can be beneficial to delay mock instantiation as long as possible, so that you can finish configuring the mock before it gets invoked. For example:

mockDbContext.Setup(m => m.Set<MyEntity>()).Returns(mockDbSet.Object);
                                                    ^^^^^^^^^^^^^^^^

Have you tried changing this to a lambda, i.e. .Returns(() => mockDbSet.Object)?

@Eregrith
Copy link
Author

Eregrith commented Oct 3, 2017

I have the same error using .Returns(() => mockDbSet.Object)

@Eregrith
Copy link
Author

Eregrith commented Oct 3, 2017

Ah, about opening the solution: I am not using VS2017 as I thought I was. I am currently on VS2015 here, while I use 2017 at home. Guess I'll have to have a look there then.

@stakx
Copy link
Contributor

stakx commented Oct 6, 2017

I finally found some time to take a slightly closer look at your repro code.

You will see the behavior I reported (and which you expected to begin with) if you define MyDbContext as follows:

public partial class MyDbContext : DbContext { }

You'll get the behaviour you've reported when you define it like this instead:

public partial class MyDbContext : DbContext
{
    public virtual DbSet<MyEntity> MyEntity { get; set; }
}

If you updated to a more recent version of Moq, you'd get a slightly more useful error message instead of the one you posted above:

System.NotImplementedException:
The member IEnumerable.GetEnumerator has not been implemented on type DbSet`1Proxy which inherits from DbSet`1. Test doubles for DbSet`1 must provide implementations of methods and properties that are used.

Basically, you can work around your issue by providing your mockDbSet with working implementations of IEnumerable and IEnumerable<MyEntity> (and later on, you might want to add IQueryable and IQueryable<MyEntity>, too):

// this forest of angle brackets and parentheses looks horrible, but makes things work:
var myEntities = new List<MyEntity>();
Mock<DbSet<MyEntity>> mockDbSet = new Mock<DbSet<MyEntity>>();
mockDbSet.As<IEnumerable<MyEntity>>().Setup(m => m.GetEnumerator()).Returns(() => myEntities.GetEnumerator());
mockDbSet.As<IEnumerable>().Setup(m => m.GetEnumerator()).Returns(() => myEntities.GetEnumerator());

I've seen more and more questions about mocking Entity Framework DbContexts lately here, this really seems to be a pain point. Partially caused by DbContext being a bit of a god object with lots of other objects attached, that implement lots of interfaces; partially by some inconsistencies and minor bugs in Moq's API that can't easily be changed that late in its history.

I'd be happy if you actually found a clean way to make Moq not invoke mocks' IEnumerable implementation when it does its own internal work; but perhaps all you wanted was a fix for your issue, then hopefully the above will help.

Please let me know how you would like to proceed.

@stakx stakx removed the needs-repro label Oct 6, 2017
@Eregrith
Copy link
Author

Eregrith commented Oct 7, 2017

Ah, so it was the enumerator that was at fault. It did not even occur to me to replace As<IQueryable<>> with As<IEnumerable<>> ... >_<

I'd be more than happy to try and have a look at what can be done to bypass using IEnumerable completely, but if you want to close this issue (it is resolved after all) maybe we can reopen another on the subject of IEnumerable?

Indeed, EntityFramework DbContext is a pain. While they did enhance the experience by making most properties virtual in the later versions, it is a highly impractical way of dealing with representing the database when coding in a unit test perspective.

Anyway thanks for your time 👍

@stakx
Copy link
Contributor

stakx commented Oct 7, 2017

@Eregrith: You're welcome. Regarding testing and Entity Framework, I like the idea of the InMemory provider they've introduced in EF Core. As far as I can see, it means you no longer need to mock DbContext and DbSets at all.

Yes, I'd say let's close this issue, and feel free to open a new one regarding Moq's internal use of a mock's IEnumerable.

Some background on that topic in preparation:

  • There was an issue (that I can't find right now) asking for more useful error messages, which made me add code to ExpressionStringBuilder that formats the items in a collection by iterating over them.

  • What I wasn't fully aware of at the time is that ExpressionStringBuilder, contrary to some comments, isn't only used for diagnostic messages, but also for internals (e.g. to build string representations of setups that act as a quick setup lookup key via ExpressionKey). This is probably the biggest issue here. Could be that ExpressionStringBuilder needs to be parameterized so it knows whom it's creating a string for: for Moq internally, or for a human reading an error message.

  • See also IEnumerable mocks shouldn't cause NullReferenceException #413 (quick attempt at a fix).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants