Using Methods not Defined in Interface on Fakes within Unit Tests

Question

In my ViewModel, portions of functionality are enabled/disabled depending on the logged-in individual's permissions. The ViewModel relies on a dependency-injected ISecurity object to check if a user has a specific permission. Different portions of functionality require different permissions.

public Interface ISecurity
{
   bool UserHasPermision(int userId, string permission);
}

In my production code, the concrete implementation of ISecurity interacts with an external application which does not allow me to change an individual's permissions. I created a FakeSecurity class that would allow me to do this in unit tests.

class FakeSecurity: ISecurity
{
     private Dictionary<int, List<string>> permissions = new Dictionary<int, List<string>>();

     public bool UserHasPermission(int userId, string permission)
     {
         return permissions.ContainsKey(userId) && 
                permissions[userId].Contains(permission);
     }

     //Not defined in ISecurity
     public void SetPermission(int userId, string permission, bool hasPermission)
     {
         if (!permissions.ContainsKey(userId))
         {
              permissions[userId] = new List<string>();
         }
         List<string> userPermissions = permissions[userId];
         if (hasPermission) 
         {
              userPermissions.Add(permission);
         }
         else 
         {
              userPermissions.Remove(permission);
         }
     }
}

The problem here is that SetPermission() is not defined in the ISecurity interface, so in order for my Unit Tests to set an individual's permissions I need to cast the ISecurity object registered with my IUnityContainer to a FakeSecurity object. I am told that my unit test should be ignorant of the specific type of implementation that is being used for a particular interface and that calling methods that are not defined in the interface is an anti-pattern.

[TestMethod]
public void UserDoesNotHavePermission()
{
   // test setup
   IUnityContainer iocContainer = GetIocContainer();
   ISecurity sec = iocContainer.Resolve<ISecurity>(); //registered singleton
   (sec as FakeSecurity).SetPermission(GetCurrentUser().Id, "Save Colors", false);
   var viewModel = iocContainer.Resolve<MaintainColorsViewModel>(); //per-request

   // asserts
   Assert.IsFalse(viewModel.CanSave);
}

[TestMethod]
public void UserHasPermission()
{
   // test setup
   IUnityContainer iocContainer = GetIocContainer();
   ISecurity sec = iocContainer.Resolve<ISecurity>(); //registered singleton
   (sec as FakeSecurity).SetPermission(GetCurrentUser().Id, "Save Colors", true);
   var viewModel = iocContainer.Resolve<MaintainColorsViewModel>(); //per-request

   // asserts
   Assert.IsTrue(viewModel.CanSave);
}

Is this a bad practice or not? I realize that I shouldn't cast my ISecurity instace to a particular type within my application code, but is this really an issue Unit Tests?

Answer 1

Based on my experience, when you feel something not natural in Unit Test, you may want to re-factor your code.

According to this code, there are a couple choices.

1. Define the permission dictionary as a property in the interface. So it is easy to set values at unit testing.
2. Define a permission layer, IPermission, to add/retrieve/remove permission. Then you can mock IPermission in unit testing your Security implementation.

3. At least I would define the SetPermission method in ISecurity, your current code does not let you define an ISecurity object and let you set permission. Think the following code.

   { ISecurity sec = CreateSecurity() sec.SetPermission() // ERROR, SetPermission is not a method in ISecurity. }

   private ISecurity CreateSecurity() { return new Security() }

   However, I am not sure how to unit test in this case on top of my head.

Answer 2

I am told that my unit test should be ignorant of the specific type of implementation

This is incorrect. It is completely normal and good practice to let tests use both fake implementations and the class under test directly.

You however, are using the DI container in your unit tests, and that actually is bad practice. Although the use of the DI container is okay when you're writing integration tests (since you want to test components in integration with other components), using the DI library in unit tests leads to hard to read and maintain tests. With unit tests, you test code in isolation. This means that you usually create the class under test by hand, and inject the required fake dependencies to get the test running.

I would therefore expect such unit test to look like this:

public void CanSave_CurrentUserHasNoPermission_ReturnsFalse() {
    // Arrange
    var noPermission = new FakeSecurity { CurrentUserHasPermission = false };
    var viewModel = new MaintainColorsViewModel(noPermission);

    // Act
    bool actualResult = viewModel.CanSave;

    // Assert
    Assert.IsFalse(actualResult);
}

public void CanSave_CurrentUserHasPermission_ReturnsTrue() {
    // Arrange
    var hasPermission = new FakeSecurity { CurrentUserHasPermission = true };
    var viewModel = new MaintainColorsViewModel(hasPermission);

    // Act
    bool actualResult = viewModel.CanSave;

    // Assert
    Assert.IsTrue(actualResult);
}

public void CanSave_Always_QueriesTheSecurityForTheSaveColorsPermission() {
    // Arrange
    var security = new FakeSecurity();
    var viewModel = new MaintainColorsViewModel(security);

    // Act
    bool temp = viewModel.CanSave;

    // Assert
    Assert.IsTrue(security.RequestedPermissions.Contains("Save Colors"));
}    

There are a few things to note about this code:

• Both the FakeSecurity and the MaintainColorsViewModel are created directly in the tests here; no DI library is used. This makes the tests much more readable and maintainable (and faster).
• I considerably simplified the FakeSecurity class (shown below), because you want fake classes to be as simple as possible.
• A third test is added to check explicitly whether the MaintainColorsViewModel requests the expected permission.
• The AAA pattern (Arrange/Act/Assert) is implemented explicitly.

To allow these tests to be written the way they are, the following change has been made to the ISecurity abstraction:

interface ISecurity
{
    bool UserHasPermission(string permission);
}

The userId parameter has been removed from the UserHasPermission method. The reason for this is that the ISecurity implementation will be able to find out who the current user is by itself. Allowing consumers of ISecurity to pass this parameter along only means that the API is getting more complex, there is more code to write, there's a bigger chance of programming errors, and we therefore need more supporting tests. In other words, the sole addition of this userId property forces a lot of extra production and test code to write and maintain.

Here is the simpflified FakeSecurity class:

class FakeSecurity : ISecurity
{
    public bool CurrentUserHasPermission;
    public List<string> RequestedPermissions = new List<string>();

    public bool UserHasPermission(string permission)
    {
        this.RequestedPermissions.Add(permission);
        return this.CurrentUserHasPermission;
    }
}

The FakeSecurity class now has very little code and that makes it, just by looking at it, very easy to check for correctness. Remember, test code should be as simple as possible. Side note: replacing this class with a generated mock object, doesn't make our code easier. In most cases it will actually make our unit tests harder to read, understand and maintain.

One reason for developers to start using a DI container inside their unit tests is because the manual creation of the class under test (with all its fake dependencies) causes maintenance issues in their tests. This is true actually; if the MaintainColorsViewModel has multiple dependencies, and we would create that MaintainColorsViewModel in each test, the addition of a single dependency would cause us to change all our MaintainColorsViewModel tests. This often is a reason for developers to either use a DI container -or- revert to mocking frameworks.

This however is not a good reason to start using a DI container or mocking library. A simple refactoring can completely remove the maintenance problem; we just have to create a factory method as follows:

private static MaintainColorsViewModel CreateViewModel(params object[] dependencies) {
    return new MaintainColorsViewModel(
        dependencies.OfType<ISecurity>().SingleOrDefault() ?? new FakeSecurity(),
        dependencies.OfType<ILogger>().SingleOrDefault() ?? new FakeLogger(),
        dependencies.OfType<ITimeProvider>().SingleOrDefault() ?? new FakeTimeProvider(),
        dependencies.OfType<IUserContext>().SingleOrDefault() ?? new FakeUserContext());
}

Here I assume that the MaintainColorsViewModel contains 4 dependencies (namely ISecurity, ILogger, ITimeProvider and IUserContext). The CreateViewModel factory method allows passing in all dependencies using a params array, and the method tries to get each abstraction from the array and when missing replaces it with the default fake implementation.

With this factory, we can now rewrite our tests to the following:

[TestMethod]
public void CanSave_CurrentUserHasNoPermission_ReturnsFalse()
{
    // Arrange
    var noPermission = new FakeSecurity { CurrentUserHasPermission = false };

    MaintainColorsViewModel viewModel = CreateViewModel(noPermission);

    // Act
    bool actualResult = viewModel.CanSave;

    // Assert
    Assert.IsFalse(actualResult);
}

Or we can pass in multiple dependencies if the test requires this:

[TestMethod]
public void CanSave_CurrentUserHasNoPermission_LogsWarning()
{
    // Arrange
    var logger = new FakeLogger();
    var noPermission = new FakeSecurity { CurrentUserHasPermission = false };

    MaintainColorsViewModel viewModel = CreateViewModel(logger, noPermission);

    // Act
    bool temp = viewModel.CanSave;

    // Assert
    Assert.IsTrue(logger.Entries.Any());
}

Do note that this test is just here for educational purposes. I don't suggest the view model to actually do the logging; that should not be its responsibility.

The moral of the story here is actually that good design can simplify your testing efforts considerably to the point that you can write less code and less tests, while improving the quality of your software.

Answer 3

You shouldn't use a DI container in unit tests, see the answer in this question.

In unit tests, the object graph that you are testing is usually small (usually a single class). So you don't need a DI container.

Without a container, here is how your test would look like:

//Arrange
FakeSecurity fake_security = new FakeSecurity();

fake_security.SetPermission(GetCurrentUser().Id, "Save Colors", false);

MaintainColorsViewModel sut = new MaintainColorsViewModel(fake_security);

//Act
...

Please note that I am assuming that you are using constructor injection to inject ISecurity into MaintainColorsViewModel.

Please note that instead of creating a FakeSecurity class, you can use auto-generated mocks by using mocking frameworks. Here is a link to one of the mocking frameworks called FakeItEasy.