How to avoid or UnitTest the accidental use of a concrete singleton type instead of its abstraction with StructureMap

Question

I recently had the following bug in my code which took me for ever to debug. I wanted to inject an instance based on its interface like this:

MovementController(IMotorController motorController)

However I accidentally used the concrete type like this:

MovementController(MotorController motorController)

The project still built and ran fine until I tried to access the motorController from the MovementController instance. Since the underlying implementation of IMotorController accesses hardware, it has to be a singleton or my locks code. However, since I had other classes with the injected IMotorController, I now had two instances MotorController in my object-graph, which both accessed the hardware over a serial connection. This caused an error, at run time at a much lower level, which took me forever to debug and find the true cause.

How can I avoid this type of bug or write a unit test for my StructureMap registry to catch this subtle bug?

Answer 1

You could easily check for this using a static analysis tool like NDepend. With it you would just look for types that were controllers and then inspect their constructors and warn if you found any constructor parameters that were not interface types.

---

Just to refine the Steve answer, you could write a code rule that could look like: (with NDepend a code rule is a C# LINQ query prefixed with warnif count > 0)

// <Name>Don't use MotorController, use IMotorController instead</Name>
warnif count > 0
from m in Application.Methods 
where m.IsUsing ("NamespaceA.MotorController ") &&
      m.ParentType.FullName != "NamespaceB.ClassThatCanUseMotorController "
select m

The rule can be refined easily if there are zero or many ClassThatCanUseMotorController.

Answer 2

The safest solution is to check during runtime that only one instance of MotorController is created. For instance you could count the number of instances of MotorController with a static counter variable:

public class MotorController : IMotorController
{
    private static bool instantiated;

    public MotorController(...)
    {
        if (instantiated)
            throw new InvalidOperationException(
                "MotorController can only be instantiated once.")

        ...

        instantiated = true;
    }

    ...
}

I'd usually consider this bad design, because whether a class is used as a singleton or not is something only the dependency injection framework should care about. Also note that this is not thread-safe.

Answer 3

Ok. So the solution I came up with for my Unit Test, is to get all the instances that implement IMotorController and assert that their count equals 1:

 var motorControllerInstances = container.GetAllInstances<IMotorController>().Select(x => x); // cast enumerable to List using Linq
 Assert.True(motorControllerInstances.Count == 1);

Not sure this is the most elegant way, but it seems to work.

Update 1: This code does not catch the bug I had. I am still looking for a correct answer to my problem.

Update 2: I am getting closer. This will at least catch if you have accidentally registered a concrete type of the corresponding interface. However, it does not appear to check whether an instance of it was actually built.

    var allInterfaceInstances = dicFixture.result.Model.GetAllPossible<IMotorController>();
    Assert.True(allInterfaceInstance.Count() == 1);

Answer 4

In trying to adhere to the D in SOLID

Dependency inversion principle where one should “Depend upon Abstractions. Do not depend upon concretions

for a project. In this case Asp.Net-MVC5, I wanted a way to make sure that all controllers (MVC and WebAPI2) were following this pattern where they were not dependent on concretions.

The original idea came from an article I had read where a unit test was created to scan all controllers to make sure that they had explicit authorization defined. I applied a similar thinking in checking that all controllers had constructors that depended on abstractions.

[TestClass]
public class ControllerDependencyTests : ControllerUnitTests {
    [TestMethod]
    public void All_Controllers_Should_Depend_Upon_Abstractions() {

        var controllers = UnitTestHelper.GetAssemblySources() //note this is custom code to get the assemblies to reflect.
            .SelectMany(assembly => assembly.GetTypes())
            .Where(t => typeof(IController).IsAssignableFrom(t) || typeof(System.Web.Http.Controllers.IHttpController).IsAssignableFrom(t));

        var constructors = controllers
            .SelectMany(type => type.GetConstructors())
            .Where(constructor => {

                var parameters = constructor.GetParameters();
                var result = constructor.IsPublic
                    && parameters.Length > 0
                    && parameters.Any(arg => arg.ParameterType.IsClass && !arg.ParameterType.IsAbstract);

                return result;
            });
        // produce a test failure error mssage if any controllers are uncovered
        if (constructors.Any()) {
            var errorStrings = constructors
                .Select(c => {
                    var parameters = string.Join(", ", c.GetParameters().Select(p => string.Format("{0} {1}", p.ParameterType.Name, p.Name)));
                    var ctor = string.Format("{0}({1})", c.DeclaringType.Name, parameters);
                    return ctor;
                }).Distinct();

            Assert.Fail(String.Format("\nType depends on concretion instead of its abstraction.\n{0} Found :\n{1}",
                            errorStrings.Count(),
                            String.Join(Environment.NewLine, errorStrings)));
        }
    }
}

So given the following example. (Note I adapted this to MVC)

public class MovementController : Controller {
    public MovementController(MotorController motorController) {
        //...
    }
}

public interface IMotorController {
    //...
}

public class MotorController : IMotorController {
    //...
}

the unit test would fail with ...

Result Message: Assert.Fail failed. 
Type depends on concretion instead of its abstraction.
1 Found :
MovementController(MotorController motorController)

This worked for me because I had a common type to look for with the IController and ApiController.

There is room for improvement on the test but is should be a good starting point for you.