AutoFixture refactoring

Question

I started to use AutoFixture http://autofixture.codeplex.com/ as my unit tests was bloated with a lot of data setup. I was spending more time on seting up the data than to write my unit test. Here's an example of how my initial unit test looks like (example taken from cargo application sample from DDD blue book)

[Test]
public void should_create_instance_with_correct_ctor_parameters()
{
    var carrierMovements = new List<CarrierMovement>();

    var deparureUnLocode1 = new UnLocode("AB44D");
    var departureLocation1 = new Location(deparureUnLocode1, "HAMBOURG");
    var arrivalUnLocode1 = new UnLocode("XX44D");
    var arrivalLocation1 = new Location(arrivalUnLocode1, "TUNIS");
    var departureDate1 = new DateTime(2010, 3, 15);
    var arrivalDate1 = new DateTime(2010, 5, 12);

    var carrierMovement1 = new CarrierMovement(departureLocation1, arrivalLocation1, departureDate1, arrivalDate1);

    var deparureUnLocode2 = new UnLocode("CXRET");
    var departureLocation2 = new Location(deparureUnLocode2, "GDANSK");
    var arrivalUnLocode2 = new UnLocode("ZEZD4");
    var arrivalLocation2 = new Location(arrivalUnLocode2, "LE HAVRE");
    var departureDate2 = new DateTime(2010, 3, 18);
    var arrivalDate2 = new DateTime(2010, 3, 31);

    var carrierMovement2 = new CarrierMovement(departureLocation2, arrivalLocation2, departureDate2, arrivalDate2);

    carrierMovements.Add(carrierMovement1);
    carrierMovements.Add(carrierMovement2);

    new Schedule(carrierMovements).ShouldNotBeNull();
}

Here's how I tried to refactor it with AutoFixture

[Test]
public void should_create_instance_with_correct_ctor_parameters_AutoFixture()
{
    var fixture = new Fixture();

    fixture.Register(() => new UnLocode(UnLocodeString()));

    var departureLoc = fixture.CreateAnonymous<Location>();
    var arrivalLoc = fixture.CreateAnonymous<Location>();
    var departureDateTime = fixture.CreateAnonymous<DateTime>();
    var arrivalDateTime = fixture.CreateAnonymous<DateTime>();

    fixture.Register<Location, Location, DateTime, DateTime, CarrierMovement>(
        (departure, arrival, departureTime, arrivalTime) => new CarrierMovement(departureLoc, arrivalLoc, departureDateTime, arrivalDateTime));

    var carrierMovements = fixture.CreateMany<CarrierMovement>(50).ToList();

    fixture.Register<List<CarrierMovement>, Schedule>((carrierM) => new Schedule(carrierMovements));

    var schedule = fixture.CreateAnonymous<Schedule>();

    schedule.ShouldNotBeNull();
}

private static string UnLocodeString()
{
    var stringBuilder = new StringBuilder();

    for (int i = 0; i < 5; i++)
        stringBuilder.Append(GetRandomUpperCaseCharacter(i));

    return stringBuilder.ToString();
}

private static char GetRandomUpperCaseCharacter(int seed)
{
    return ((char)((short)'A' + new Random(seed).Next(26)));
}

I would like to know if there's better way to refactor it. Would like to do it shorter and easier than that.

Answer 1

Your initial attempt looks good, but there are at least a couple of things you can simplify a bit.

First of all, you should be able to reduce this:

fixture.Register<Location, Location, DateTime, DateTime, CarrierMovement>(
    (departure, arrival, departureTime, arrivalTime) =>
        new CarrierMovement(departureLoc, arrivalLoc, departureDateTime, arrivalDateTime));

to this:

fixture.Register<Location, Location, DateTime, DateTime, CarrierMovement>(
    () => new CarrierMovement(departureLoc, arrivalLoc, departureDateTime, arrivalDateTime));

since you're not using those other variables. However, this essentially locks any creation of CarrierMovement to use the same four values. Although each created CarrierMovement will be a separate instance, they will all share the same four values, and I wonder if that was what you meant?

In the same vein as above, instead of

fixture.Register<List<CarrierMovement>, Schedule>((carrierM) =>
    new Schedule(carrierMovements));

you can write

fixture.Register(() => new Schedule(carrierMovements));

since you don't use the carrierM variable. Type inferencing will figure out that you are registering a Schedule because of the return type of the Func.

However, assuming that the Schedule constructor looks like this:

public Schedule(IEnumerable<CarrierMovement> carrierMovements)

you could instead have just registered the carrierMovements like this:

fixture.Register<IEnumerable<CarrierMovement>>(carrierMovements);

which would cause AutoFixture to automatically resolve Schedule correctly. This approach is more maintainable because it allows you to add a parameter to the Schedule constructor in the future without breaking the test (as long as AutoFixture can resolve the parameter type).

However, we can do better than that in this case because we don't really use the carrierMovements variable for anything else than registration. What we really need to do is just to tell AutoFixture how to create instances of IEnumerable<CarrierMovement>. If you don't care about the number 50 (you shouldn't), we can even use Method Group syntax like this:

fixture.Register(fixture.CreateMany<CarrierMovement>);

Notice the lack of method invocation parantheses: we're registering a Func, and since the CreateMany<T> method returns IEnumerable<T> type inferencing takes care of the rest.

However, those are all details. On a higher level, you might want to consider not registering CarrierMovement at all. Assuming this constructor:

public CarrierMovement(Location departureLocation,
    Location arrivalLocation,
    DateTime departureTime,
    DateTime arrivalTime)

autofixture should be able to figure it out by itself.

It will create a new Location instance for every departureLocation and arrivalLocation, but that's no different than what you manually did in the original test.

When it comes to the times, by default AutoFixture uses DateTime.Now, which at least ensures that the arrival time will never be before the departure time. However, they are very likely to be identical, but you could always register an auto-incrementing function if that is a problem.

Given those considerations, here's an alternative:

public void should_create_instance_with_correct_ctor_parameters_AutoFixture()
{
    var fixture = new Fixture();

    fixture.Register(() => new UnLocode(UnLocodeString()));

    fixture.Register(fixture.CreateMany<CarrierMovement>);

    var schedule = fixture.CreateAnonymous<Schedule>();

    schedule.ShouldNotBeNull();
}

---

To resolve the issue with IList<CarrierMovement> you will need to register it. Here's one way to do it:

fixture.Register<IList<CarrierMovement>>(() =>
    fixture.CreateMany<CarrierMovement>().ToList());

However, since you ask, I imply that the Schedule constructor looks like this:

public Schedule(IList<CarrierMovement> carrierMovements)

and I really think you should reconsider changing that API to take an IEnumerable<Carriemovement>. From an API design perspective, supplying a collection through any member (including a constructor) implies that the member is allowed to modify the collection (e.g. by invoking it's Add, Remove and Clear methods). That is hardly behavior you would expect from a constructor, so don't allow it.

AutoFixture will automatically generate new values for all Location objects in my above example, but due to the speed of the CPU, subsequent instances of DateTime are likely to be identical.

If you want increasing DateTimes, you can write a small class that increments the returned DateTime every time it's invoked. I'll leave the implementation of that class to the interested reader, but you could then register it like so:

var dtg = new DateTimeGenerator();
fixture.Register(dtg.Next);

assuming this API (notice once more the Method Group syntax above):

public class DateTimeGenerator
{
    public DateTime Next();
}