views:

100

answers:

2

I've been working on the best way to test an abstract class named TabsActionFilter. I've guranteed that classes that inherit from TabsActionFilter will have a method called GetCustomer. In practice this design seems to work well.

Where I've had some issues is figuring out how to test the OnActionExecuted method of the base class. This method relies upon the implementation of the the protected abstract GetCustomer method. I've tried mocking the class using Rhino Mocks but can't seem to mock the return of a fake customer from GetCustomer. Obviously, flipping the method to public will make mocking available, but protected feels like the more appropriate accessibility level.

For the time being in my test class I've added a concrete private class that inherits from TabsActionFilter and returns a faked Customer object.

  • Is a concrete class the only option?
  • Is there a simple mechanism of mocking that I'm missing that would allow Rhino Mocks to provide a return for GetCustomer?

As a note Anderson Imes discusses his opinions on this in an answer about Moq and I could be missing something key, but it doesn't seem applicable here.

Class that needs to be tested

public abstract class TabsActionFilter : ActionFilterAttribute
{
    public override void OnActionExecuted(ActionExecutedContext filterContext)
    {
        Customer customer = GetCustomer(filterContext);

        List<TabItem> tabItems = new List<TabItem>();
        tabItems.Add(CreateTab(customer, "Summary", "Details", "Customer",
            filterContext));
        tabItems.Add(CreateTab(customer, "Computers", "Index", "Machine",
            filterContext));
        tabItems.Add(CreateTab(customer, "Accounts", "AccountList",
            "Customer", filterContext));
        tabItems.Add(CreateTab(customer, "Actions Required", "Details",
            "Customer", filterContext));

        filterContext.Controller.ViewData.PageTitleSet(customer.CustMailName);
        filterContext.Controller.ViewData.TabItemListSet(tabItems);
    }

    protected abstract Customer GetCustomer(ActionExecutedContext filterContext);
}

Test Class and Private Class for "mocking"

public class TabsActionFilterTest
{
    [TestMethod]
    public void CanCreateTabs()
    {
        // arrange
        var filterContext = GetFilterContext(); //method omitted for brevity

        TabsActionFilterTestClass tabsActionFilter =
            new TabsActionFilterTestClass();

        // act
        tabsActionFilter.OnActionExecuted(filterContext);

        // assert
        Assert.IsTrue(filterContext.Controller.ViewData
            .TabItemListGet().Count > 0);
    }

    private class TabsActionFilterTestClass : TabsActionFilter
    {
        protected override Customer GetCustomer(
            ActionExecutedContext filterContext)
        {
            return new Customer
            {
                Id = "4242",
                CustMailName = "Hal"
            };
        }
    }
}
+2  A: 

I think the problem you are currently running into is that your class is not testable, or is not testable enough. This is of course assuming that you have correctly identified that GetCustomer is indeed something that needs to be mocked in order to properly test in isolation.

If GetCustomer is something that needs to be mocked in order to properly isolate and test TabsActionFilter, you will somehow need to make the implementation of GetCustomer a composable part of the class, and not an inherited method. The most common way to achieve this is to use Inversion of Control/Dependency Injection.

All that said, you COULD use something like TypeMock to achieve this. However, when you encounter a situation like this where a class is hard to test, it is usually a signal that your class has too many responsibilities and needs to be broken into smaller components.

(I am not a fan of using TypeMock FWIW).

Phil Sandler
Phil the implementation of the `GetCustomer` method is the responsibility of the inheriting classes. Unfortunately, dependency injection via a constructor won't work here because `TabsActionFilter` inherits from `ActionFilterAttribute` which limits me to default constructors. I guess I could use property injection on the inheriting classes to add a service that allowed for getting a Customer. Is that really a better design though?
ahsteele
just because it is the responsibility of the inheriting class does not mean they have to implement them themselves. your statement about being limited in constructors is not true, you can have a wider constructor than the class you derive from. please see my answer.
Alex Lo
@Alex I totally agree that it's not true in a "normal" sense but I am working with an attribute class from the MVC framework and therefore don't have the ability to use DI. See Jimmy Bogard's post: http://www.lostechies.com/blogs/jimmy_bogard/archive/2010/05/03/dependency-injection-in-asp-net-mvc-filters.aspx
ahsteele
you are saying that because you must have a parameterless constructor that you cannot do this? not true. i am updating my answer.
Alex Lo
please see my updated answer
Alex Lo
+1  A: 

Phil's answer is correct. If instead of having an abstract class, you had a class that required the injection of a customer getter (or factory, or whatever) then you'd be in good shape to test. Abstract classes are the enemy of testing (and good design).

public class TabsActionFilter : ActionFilterAttribute 
{ 
    private readonly ICustomerGetter _getter;
    public TabsActionFilter(ICustomerGetter getter)
    { _getter = getter; }

    public override void OnActionExecuted(ActionExecutedContext filterContext) 
    { 
        Customer customer = _getter.GetCustomer(filterContext); 

        ...
    } 
} 
public interface ICustomerGetter
{ 
     Customer GetCustomer(ActionExecutedContext filterContext);
}

in your test you instantiate the now non-abstract TabsActionFilter and give it a Mock getter, which should be trivial to mock.

EDIT: seems there is concern that you must have a parameterless constructor. this is easy. given the above code, you then have your "real" filters implemented as

public class MyFilter : TabsActionFilter
{
    public MyFilter() : base(new MyGetter()) {}
}

you can still test your base class the same.

Alex Lo
Alex I see where you and Phil are going with this. However, I limited by my inheritance from `ActionFilterAttribute` which is from `System.Web.Mvc`. As such I don't have the ability to do dependency injection. I could do property injection but the `getter` would be different for each `Action` these attributes would be applied to.
ahsteele
yes you do. post details about the constructors that are constraining you. it can be done. what is this "property injection"? your last sentence does not make sense.
Alex Lo
@Alex I meant I could use *property injection* to set the `getter`. Unfortunately, the getter would different in each instance where the `ActionFilter` is used.
ahsteele
@Alex, can you provide documentation on the statement that Abstract classes are the enemy of good design? How do you accomplish DRY without them?
sgriffinusa
@sgriffinusa, through composition, not inheritance. note that i was able to achieve the reuse he desired without inheritance (except the parameterless constructor bit, if there is a requirement for that inheritance is easier, but you could do it through composition). i suggest reading Bob Martin's SOLID principals in one of his books.
Alex Lo
@ahsteele, what would be different about the getter? is it relying on state from the Filter? if so then pass it in as a parameter. please demonstrate why this solution does not work.
Alex Lo