1

I'm currently writing unit tests for ASP.NET Core Controllers. Some controllers inject UserManager<T> which seems to be a really hard type to mock. After some attempts to mock or even fake it, I eventually had the idea to create a facade to wrap the UserManager<T> and inject the facade interface into the controllers instead of the UserManager.

I think I'll need to explain this approach to the team and fear that my reason (to make it mockable) is not convincing enough. Is there some documentation or best practice that supports or justifies this plan?

Sandro
  • 113
  • 3
  • 2
    Your approach is correct. You should never mock third-party types, but wrap them with your own class. https://softwareengineering.stackexchange.com/questions/298145/wrapping-third-party-library-is-best-practice – Héctor Apr 26 '20 at 11:48
  • 2
    I'd flip a bit the argument and say that your idea is to "make X Controller testable", not "make UserManager mockable". It can be a bit of semantics but it makes your end goal clearer and it sparks the discussion among the team about what needs to be done to make it testable, instead of focusing in arguing in favor or against the new facade. – Gabriel Pires Apr 26 '20 at 13:55
  • 1
    As a general matter of practice, never blindly pitch an idea to your team. Create a proof of concept. Show them both versions so they can judge for themselves which is better. "The proof is in the pudding," they say. So make pudding. The proof of concept is your pudding. – Greg Burghardt Apr 26 '20 at 17:06

3 Answers3

3

Is there some documentation or best practice that supports or justifies this plan?

You might want to review Role Interfaces. The high level summary here being that most of your code (the consumer) should be focusing on what it wants done, with only a little bit being aware that UserManager[T] is how you do it. Instead of trying to do everything with a single "general-purpose" interface, we define specialized interfaces that focus on expressing a single idea very well.

That might mean that instead of accepting a single general purpose interface as an argument, you instead accept several special purpose "role interfaces".

With the role interfaces defined, we now need implementations of them. Typically, we end up with something like the Adapter pattern in play; we have some sort of factory method that accepts a UserManager[T] as an argument and returns an adapter. The adapter itself simply translates between the role protocol and the user manager protocol.

We get two benefits out of this right away. First, the role interfaces are typically a lot simpler than the UserManager, and therefore they are simpler to mock. Second, the amount of code that depends on the UserManager abstraction is greatly reduced.

To some degree, this is a shell game - we're still making the same calls to UserManager that we were before. But what we have done is created a boundary (our role interfaces) between our bespoke code (which is complicated, and needs to be tested) and the UserManager (which makes things difficult to test).

The adapters themselves use only a small slice of the user manager. The implementation of the adapters are sensitive to changes in the behavior of the UserManager... but the UserManager code is (For the most part) stable. Because the code is simple and stable, we don't expect to invest a lot in refactoring further - think "throw it away". This shifts the distribution of risk quite a bit, and in turn means that we can target the adapters with different testing strategies than we use in our more complicated core (including, potentially, choosing not to directly test the adapters).

In short "don't mock what you don't own" is intended, in part, to remind you to create more things that you are "allowed" to mock. The "thing you don't own" then becomes a hidden implementation detail of a small part of your system.

VoiceOfUnreason
  • 32,131
  • 2
  • 42
  • 79
2

In general it points to an underlying problem if you need to mock or wrap complex external components (like a user management subsystem) in order to test your own code.

If the code in question is trivial glue code just calling into the UserManager, then it doesn't provides any value to unit test it with a wrapper or mock of the UserManager. This would just test the implementation of the wrapper, which have no value except artificially inflating the coverage score. If you want to ensure the code actually works as intended you have to test with the actual component.

Since the UserManager iteself uses dependency injection, you could inject a mocked user store into it and the use the actual UserManager in the test.

If the code in question contains more complex logic than just the integration, then the complex logic should be extracted into separate functions which can then be unit-tested separately from the integration.

JacquesB
  • 57,310
  • 21
  • 127
  • 176
  • 1
    The UserManager class should have its own tests using a mock data store. When testing the controller you want to test the controller logic. Assert that certain methods are called on the user manager. Don't go any deeper. For this reason I think mocking the *entire* user manger for controller tests if the right way to go. – Greg Burghardt Apr 26 '20 at 17:12
  • 2
    @GregBurghardt: If the the method under test only calls methods on the UserManager, then testing it with a mock provides zero value. You are basically just testing that if you call method f, then method f is indeed called. And you shouldn't test UserManager in isolation unless you are Microsoft, since it is part of the framework. But you should test the whole integration as part of an integration test. – JacquesB Apr 26 '20 at 17:22
  • 1
    I think what @GregBurghardt wanted to say is that UserManager should be tested on it's own and not within controller tests. Since Microsoft is doing that already, no one else has to do it. What I'm testing is the controller logic in one and the store logic in another test. Otherwise the store would be tested twice, that's why I want the mock in the first place. – Sandro Apr 26 '20 at 22:37
0

One aproach to hide system dependencies behind a facade is called Onion-Architecture.

For argumentation see this article https://jeffreypalermo.com/2008/07/the-onion-architecture-part-1/

k3b
  • 7,488
  • 1
  • 18
  • 31