dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.65k stars 3.15k forks source link

Make it easier to mock DbSet.Add #27110

Open RitikaPoddar opened 2 years ago

RitikaPoddar commented 2 years ago

upgrading from EF Core 5.0.2 to 6.0.1 making Nunit unit test to fail

Currently I have upgraded from .net core 5 to .net 6, so I also have to update ef core from 5 to 6.0.1. However doing this is failing some of my unit test written in nunit and using Moq. Issue is occuring while mocking 'InternalEntityEntry' class which is sealed in 6.0.1 but abstract in 5.0.2.

Below is the code that I am using to create Moq:

var entry = new Mock<InternalEntityEntry>(new Mock<IStateManager>().Object,
                new EntityType(typeof(T), new Model(), true, ConfigurationSource.Explicit)).Object;
            var entityEntry = new Mock<EntityEntry<T>>(entry);
            entityEntry.SetupGet(self => self.Entity).Returns(data);
            mockSet.Setup(d => d.AddAsync(It.IsAny<T>(), default)).Returns(() => new ValueTask<EntityEntry<T>>(entityEntry.Object));

and below is the error received:

System.NotSupportedException : Type to mock (InternalEntityEntry) must be an interface, a delegate, or a non-sealed, non-static class.

Help me in resolving this.

Include provider and version information

EF Core version: 6.0.1 Database provider: Microsoft.EntityFrameworkCore.SqlServer Target framework: .NET 6.0 Operating system: IDE: Visual Studio 2022

roji commented 2 years ago

@RitikaPoddar mocking an internal EF type is not something that should be done in application testing - by definition these types don't represent the public API of the library, may change at any moment and should not be needed.

Can you provide more context on exactly what you're trying to test, and provide the code for the entire test? We may be able to provide a better approach to doing that testing.

RitikaPoddar commented 2 years ago

@roji Below is the repo URL, where I have added demo code: https://github.com/RitikaPoddar/Net6IssueDemo

I have used SQL database and dotnet ef tool to do database migrations

roji commented 2 years ago

@ajcvickers that issue here is that mocking DbSet.AddAsync is difficult: AddAsync returns EntityEntry<TEntity>, which has only a constructor accepting an InternalEntityType. So in effect we're forcing users to reference InternalEntityType if they want to mock AddAsync. For example, if AddAsync returned an IEntityEntry<TEntity> interface this wouldn't have been a problem.

roji commented 2 years ago

The reason the user's approach regresses in 6.0, is that InternalEntityEntry was made sealed in #24905. We could consider removing sealed in a patch to unblock this, and possibly consider a redesign for a more proper fix: if InternalEntityEntry is now sealed, it may make sense to merge it into EntityEntry.

ajcvickers commented 2 years ago

@roji It is not required that InternalEntityEnrtry be mocked for this. For example:

var internalEntityEntry = new InternalEntityEntry(
    new Mock<IStateManager>().Object,
    new RuntimeEntityType("T", typeof(T), false, null, null, null, ChangeTrackingStrategy.Snapshot, null, false),
    data);

var entityEntry = new Mock<EntityEntry<T>>(internalEntityEntry);

That being said, EntityEntry<T> is not easy to mock. At the time we considered it something that people shouldn't do, but I don't think that took into account that methods like Add would return one. (In EF6, we returned just the instance, not the entry.)

roji commented 2 years ago

Yeah, the above looks like a good workaround (@RitikaPoddar).

Am renaming the issue to track a more 1st-class experience for mocking, i.e. have Add return an interface, merge InternalEntityEntry into EntityEntry or similar.

RitikaPoddar commented 2 years ago

@roji @ajcvickers Yeah, the above workaround worked for me. Thank you!

akshay-zz commented 1 year ago

@roji It is not required that InternalEntityEnrtry be mocked for this. For example:

var internalEntityEntry = new InternalEntityEntry(
    new Mock<IStateManager>().Object,
    new RuntimeEntityType("T", typeof(T), false, null, null, null, ChangeTrackingStrategy.Snapshot, null, false),
    data);

var entityEntry = new Mock<EntityEntry<T>>(internalEntityEntry);

That being said, EntityEntry<T> is not easy to mock. At the time we considered it something that people shouldn't do, but I don't think that took into account that methods like Add would return one. (In EF6, we returned just the instance, not the entry.)

If it is an internal API why are we returning EntityEntry<TEntity> from methods like Add? Does it serve any purpose?

roji commented 1 year ago

EntityEntry is not an internal API, and can be used by the user to further manipulate the entry. The inconvenience here is that constructing EntityEntry (something which usually is done only by EF itself) requires an InternalEntityEntry, which is internal.

ctrl-alt-d commented 1 year ago

I would like to share this StackOverflow's Q&A that is closely related to the issue at hand:

Trying to Unit Test Generic Repository based on .NET EF Core fails when dealing with DbContext.Entry