is there a better way to test this method?

Question

I would like to run some unit tests on the method below I am passing a mocked interface(vehicleObject) into ProcessVehicles but as soon as it is passed it gets reassigned by DetermineVehicleType so my mocked object is of no use. My first idea would be to create a boolean to determine if DetermineVehicleType should be run and add it as a param, but that sounds so very messy. Is there a better way to get around this ?

Method with Mock object being injected:

public void ProcessVehicles(ICarObject CarObject)
{
    IObject vehicleObject = DetermineVehicleType(carObject);
    vehicleObject.ProcessVehicle(carObject);
}

Original Code:

public void ProcessVehicles()
{
    IObject vehicleObject = DetermineVehicleType(carObject);
    vehicleObject.ProcessVehicle(carObject);
}

Note: I can't check if vehicleObject is null before calling DetermineVehicleType because it might not be null when the class is actually used. In the long run maybe total refactor is the answer, at this point that is not the answer I am looking for maybe there is not another option.

the method DetermineVehicleType is private

Note: I know there are code smells this is legacy code that currently works. I want to get tests around it not change it so it looks pretty and then breaks in production. Total refactor might be the only option I just want to make sure there is not another solution with the mock tools.

What is it that you want to test if you mock vehicleObject? If ProcessVehicle is called or something? — Wouter de Kort, Mar 21 '12 at 19:27
can you give a detailed explanation of what is the aim of ProcessVehicles method? I feel a bad code smell here; by passing an object to a function and changing its reference with a function having carObject as a parameter. — daryal, Mar 21 '12 at 19:28
@WouterdeKort I am wanting to get the method ProcessVehicles under test — Micah Armantrout, Mar 21 '12 at 19:28
@daryal I edited my code like it was originally, I will not argue that there is a bad code smell I can change it just trying to figure out the best way to get it under test — Micah Armantrout, Mar 21 '12 at 19:30
Yeah ok..but what is it that you want to test? Do you want to make sure that DetermineVehicleType returns the correct value? Or that ProcessVehicle does something? Can you show your test code in an Arrange, Act, Assert way? — Wouter de Kort, Mar 21 '12 at 19:32
At this point I just want to verify that DetermineVehicleType is called — Micah Armantrout, Mar 21 '12 at 19:34
Looks like I didn't get question, you said that `I am passing a mocked interface(vehicleObject) into ProcessVehicles but as soon as it is passed it gets reassigned by DetermineVehicleType` but you really passing `carObject` into the `DetermineVehicleType(carObject)`, so you mean that you have mocked `carObject`? — sll, Mar 21 '12 at 20:40
carObject is mocked I edited my question to be more understandable — Micah Armantrout, Mar 21 '12 at 20:50

Justin Pihony · Accepted Answer · 2012-03-21T20:12:05.403

What access modifier does DetermineVehicleType have? You could stub out that method so that it returns your mocked interface (Roy Osherove calls this the abstract test driver pattern, I believe). Otherwise, this looks like a prime candidate for refactoring :)

To refactor your code you would do something like this

First, change your method signature

protected virtual IObject DetermineVehicleType(CarObject obj)
{
    //Do whatever you normally do
}

Then, in your test, you can create a stub out of the above class, and have it return your stubbed IObject no matter the CarObject passed in. You can either manually create a stub class by inheriting from the class, or you could use something like MOQ to accomplish this. Let me know if you need me to elaborate on this a little more.

Another note, however:

A better way to refactor this would be to simply pass in the IObject to the ProcessVehicles, as it seems from this example that you have a SRP violation here, where the ProcessVehicles method is doing more than processing them. But, maybe that is just from this simplified example

FULL Implementation Update

    [Test]
    public void TestMethod()
    {
        var testerStub = new TesterStub();
        testerStub.ProcessVehicles();
        //Assert something here
    }

    public class TesterStub : Tester
    {
        public override IObject DetermineVehicleType(CarObject obj)
        {
            var mockObject = new Mock<IObject>();
            mockObject.Setup(x => x.SomeMethod).Returns(Something);
            return mockObject.Object;
        }
    }

    public class Tester
    {
        protected virtual IObject DetermineVehicleType(CarObject obj)
        {
            return new ObjectTester();
        }

        public void ProcessVehicles()
        {
            var carType = DetermineVehicleType(new CarObject());

        }
    }

    public class ObjectTester : IObject
    {
    }

    public interface IObject
    {
    }

    public class CarObject
    {
    }

Yes I agree there are many violations I want to get the project under test before I refactor the project. Can you elaborate more on your refactoring idea ? — Micah Armantrout, Mar 21 '12 at 19:42
Updated my code. This pattern is really only useful for asserting a return value. If all you want is to make sure that something is called, then you will have to use some sort of injection to feed in a helper class that will do the DetermineVehicleType, which you can stub out using Moq. Or, if you are open to it, you could turn your DetermineVehicleType into a public virtual method and mock it without the manual inheritance. At that point, you could set up mock to verify that it is called. — Justin Pihony, Mar 21 '12 at 20:11

score 1 · Answer 2 · answered Mar 21 '12 at 19:57

If we are being very literal, I believe your code demonstrates a smell.

Consider this: the method ProcessVehicles calls a method on the instance called DetermineVehicleType. What does your class do? Does it Process Vehicles, or does it Determine Vehicle Type? This to me indicates violation of SRP, if you take it literally. Your class is trying to do more than one job.

Arguably this implies that SRP disapproves of private helper methods. Some commentators do indeed hold this to be the case. I'm not sure I do; as always, common sense is key.

If I were to refactor this code, I would give the class something like an IVehicleCategoryHelper which exposes DetermineVehicleType. Perhaps this would be passed in through its constructor, or if we are implementing full-on Fat Dependency Injection, an IFactory so that the instance can retrieve an IVehicleCategoryHelper when it needs one, dependent on context.

Take everything I've said with a pinch of salt. I don't necessarily believe that this is the right approach - it will ultimately be up to you to decide.

is there a better way to test this method?

2 Answers2