Make UserManager<TUser> implement IUserManager<TUser>

[jholovacs]

We have to go through an insane amount of mocking (see #344) and creating fake testing classes to get around the fact that UserManager does not implement a service interface like just about everything else does when we want to test our code that consumes UserManager's services. This is silly. We are not testing UserManager, we should not have to mock its dependencies, or include its functionality in our own tests.

Right now, to support multiple services that use UserManager we have to make something like this:

    public class FakeUserManagerDependencies<T> where T : class
    {
        public readonly Mock<IdentityErrorDescriber> MockedIdentityErrorDescriber = new Mock<IdentityErrorDescriber>();
        public readonly Mock<IOptions<IdentityOptions>> MockedIdentityOptions = new Mock<IOptions<IdentityOptions>>();
        public readonly Mock<ILogger<UserManager<T>>> MockedLogger = new Mock<ILogger<UserManager<T>>>();
        public readonly Mock<ILookupNormalizer> MockedLookupNormalizer = new Mock<ILookupNormalizer>();
        public readonly Mock<IPasswordHasher<T>> MockedPasswordHasher = new Mock<IPasswordHasher<T>>();

        public readonly Mock<IEnumerable<IPasswordValidator<T>>>
            MockedPasswordValidators = new Mock<IEnumerable<IPasswordValidator<T>>>();

        public readonly Mock<IServiceProvider> MockedServiceProvider = new Mock<IServiceProvider>();
        public readonly Mock<IUserStore<T>> MockedUserStore = new Mock<IUserStore<T>>();

        public readonly Mock<IEnumerable<IUserValidator<T>>>
            MockedUserValidators = new Mock<IEnumerable<IUserValidator<T>>>();
    }

    public class FakeUserManager<T> : UserManager<T> where T : class
    {
        public FakeUserManagerDependencies<T> MockedDependencies { get; }

        public FakeUserManager() : this(new FakeUserManagerDependencies<T>())
        {
        }

        public FakeUserManager(FakeUserManagerDependencies<T> deps)
            : base(deps.MockedUserStore.Object,
                deps.MockedIdentityOptions.Object,
                deps.MockedPasswordHasher.Object,
                deps.MockedUserValidators.Object,
                deps.MockedPasswordValidators.Object,
                deps.MockedLookupNormalizer.Object,
                deps.MockedIdentityErrorDescriber.Object,
                deps.MockedServiceProvider.Object,
                deps.MockedLogger.Object
            )
        {
            MockedDependencies = deps;
        }
    }

Now let's say we want to test a method that will retrieve a user and perform some work with that user object. In our test, we now have to build out the IUserStore<TUser>, figure out what UserManager<TUser> does "under the hood" to return the expected response, and mock out the requests. Instead, all we should be doing is mocking the IUserManager<TUser>.FindByNameAsync(string userName) to return a specific user object.

The current way does not allow the tester to properly isolate the actual code under test; we must test the UserManager<TUser>'s functionality along with our own. If the test were to fail, we cannot say for certain that the problem is in our code; for all we know it could be in the inner workings of the UserManager<TUser> class.

The way this is set up violates many tenets and good practices of code design and unit testing, which are tenets and good practices for a reason. The level of effort does not seem like it would be especially high either; mostly a 1:1 replacement in code where it's used as a dependency, than a little rewiring in the dependency configuration.

[HaoK]

You can just mock UserManager directly, whether or not there is an interface shouldn't really affect the unit testing, since you can just treat UserManager<TUser> as the service you mock.

[HaoK]

We aren't going to be adding an interface at this point, but if we ever rewrite identity in the future we likely would have a bunch of smaller interfaces instead of a single user manager.

[jholovacs]

Why not? It's unreasonable to just close this without discussion or justification... especially since I just did the work for you guys, and once the daily build is working, you can have it in the code base.

What possible design decision would require this functionality to be ignored?

Also, please provide an example of mocking the UserManager directly in .NET Core that is not unnecessarily painful. All examples and experiments I've tried have shown that you actually can't "just mock UserManager directly".

On Tue, Apr 3, 2018 at 11:52 PM, Hao Kung notifications@github.com wrote:

Closed #1732 https://github.com/aspnet/Identity/issues/1732.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/aspnet/Identity/issues/1732#event-1555550804, or mute the thread https://github.com/notifications/unsubscribe-auth/ABr4dG2J6ddzpUoibRC7jD80O_LmXRxdks5tlEN5gaJpZM4TEAbT .

[HaoK]

Modifying interfaces with new methods are breaking changes, so having one monolithic IUserManager interface means we wouldn't be able to add methods to the manager.

It depends on what you are mocking, you can look at https://github.com/aspnet/Identity/blob/dev/test/Shared/MockHelpers.cs#L19 for how we mock it typically.

Sometimes its easier to mock the store and just use the real manager