tags:

views:

816

answers:

7

Hello,

I'm trying to learn TDD by applying it to a simple project of mine. Some details (and an earlier question) are here:

http://stackoverflow.com/questions/473679/tdd-help-with-writing-testable-class

The specifics are I have a PurchaseOrderCollection class that has a private List of PurchaseOrders (passed in at constructor), and the PurchaseOrders have a boolean property IsValid. The PurchaseOrderCollection has a property HasErrors that returns a true if any PurchaseOrders in the list have IsValid as false. This is the logic I want to test.

[TestMethod]
public void Purchase_Order_Collection_Has_Errors_Is_True_If_Any_Purchase_Order_Has_Is_Valid_False()
{
    List<PurchaseOrder> orders = new List<PurchaseOrder>();

    orders.Add(new PurchaseOrder(--some values to generate IsValid false--));
    orders.Add(new PurchaseOrder(--some values to generate IsValid true--));

    PurchaseOrderCollection collection = new PurchaseOrderCollection(orders);

    Assert.IsTrue(collection.HasErrors);
}

This is similar to my previous question in that this test is too coupled in that I have to know the logic of what makes a PurchaseOrder IsValid false or true to pass the test, when really this test shouldn't care. The question is different (imo) in that the classes themselves aren't the problem.

Essentially I want to be able to declare a PurchaseOrder that has IsValid false or true without knowing anything more about what a PurchaseOrder is.

From my limited TDD knowledge, this is something you use Stubs or Mocks for. My main question, is this correct? Or should I be using a different method for this? Or am I completely flawed and am just writing this test and thinking about it wrong?

My initial thought was to just use some kind of mock framework and create a PurchaseOrder that always returns true or false. From what I've read though, I'd need to declare IsValid virtual. So my second thought was to change it to add IPurchaseOrder as an interface for PurchaseOrder and just create a fake PurchaseOrder that always returns false or true. Are both of these valid ideas?

Thanks!

A: 

I'm no expert in unit testing, but here's what I've done in the past. If you have a PurchaseOder class that can be valid/invalid, then I'm sure you also have unit tests for those, to see if they do in fact validate. Why not call those methods to generate valid and invalid PurchaseOrder objects, and then add that to your collection?

BFree
+2  A: 

...this test is too coupled in that I have to know the logic of what makes a PurchaseOrder IsValid false or true to pass the test, when really this test shouldn't care...

I'd actually argue the reverse -- that for your test to know that validity is modeled as a boolean within the purchase order means that your test knows too much about the implementation of PurchaseOrder (given that it's actually a test of PurchaseOrderCollection). I don't have a problem with using real-world knowledge (i.e. actual values that would be valid or invalid) to create appropriate test objects. Ultimately, that's really what you're testing (if I give my collection a purchase order with ridiculous values, will it correctly tell me there are errors).

In general I try to avoid writing an interface for an "entity" object such as a PurchaseOrder unless there's some reason to do so other than for testing (e.g. there are multiple kinds of PurchaseOrders in production and an interface is the best way to model that).

It's great when testing reveals that your production code could be better designed. It's not so good, though, to change your production code just to make a test possible.

As if I haven't written enough, here's another suggestion -- and this is the way I would actually solve this in real life.

Create a PurchaseOrderValidityChecker that has an interface. Use that when setting the isValid boolean. Now create a test version of the validity checker that lets you specify which answer to give. (Note that this solution probably also requires a PurchaseOrderFactory or the equivalent for creating PurchaseOrders, so that each purchase order can be given a reference to the PurchaseOrderValidityChecker when it is created.)

JacobM
I like your answer too, but can only pick one~In this case, the logic behind what constitutes a valid purchase order grew beyond being able to be calculated by a single purchase order. I haven't written the code yet, but pulling the logic out to PurchaseOrderValidityChecker seems likely.
eyston
The hesitation I had with this initially is trying to find a balance between big untestable classes, and class explosions where I pull every method into its own class. I guess experience will help here. Thanks for your answer.
eyston
+6  A: 

You are on the right track with either creating a stub or a mock. I prefer using a Mocking framework.

How it would work using a mocking framework is that you would want to mock your PurchaseOrder class therefore abstracting away it's implementation. Then setup expectations that IsValid is called and when it is called return this value.

Example using Moq, if you're using C# 3.0 and .NET Framework 3.5:

[TestMethod]
public void Purchase_Order_Collection_Has_Errors_Is_True_If_Any_Purchase_Order_Has_Is_Valid_False()
{    
    var mockFirstPurchaseOrder = new Mock<IPurchaseOrder>();
    var mockSecondPurchaseOrder = new Mock<IPurchaseOrder>();

    mockFirstPurchaseOrder.Expect(p => p.IsValid).Returns(false).AtMostOnce();
    mockSecondPurchaseOrder.Expect(p => p.IsValid).Returns(true).AtMostOnce();

    List<IPurchaseOrder> purchaseOrders = new List<IPurchaseOrder>();
    purchaseOrders.Add(mockFirstPurchaseOrder.Object);
    purchaseOrders.Add(mockSecondPurchaseOrder.Object);

    PurchaseOrderCollection collection = new PurchaseOrderCollection(orders);

    Assert.IsTrue(collection.HasErrors);
}

Edit:
Here I used an interface to create the mock of PurchaseOrder, but you don't have too. You could mark IsValid as virtual and mock the PurchaseOrder class. My rule of thumb when which way to go is to use virtual first. Just to create an interface, so I can mock an object without any architectual reason is a code smell to me.

Dale Ragan
A: 

Are both of these valid ideas?

Yes.

You might also create an Object Mother that can return both valid and invalid PurchaseOrders.

Jeffrey Fredrick
+1  A: 

I recently asked a somewhat similar question about testing. Don't forget this: do the simplest thing that you need to do, then refactor when necessary. I personally try to keep the bigger picture in mind, but I also resist the urge to overengineer my solutions. You could add two PurchaseOrder fields into your test class where one is valid and one is invalid. Use those fields to put your PurchaseOrderCollection into the state that you want to test. You will need to learn how to mock eventually, but in this case you don't need a sledgehammer when a regular hammer will solve the problem. You don't gain any value by using a mock PurchaseOrder instead of a concrete PurchaseOrder that's in the state that you want.

Most importantly, you gain much more from testing the behavior of PurchaseOrderCollection instead of just testing the state of your PurchaseOrderCollection. After your tests verify that the PurchaseOrderCollection can be put into its different states, then the more important tests are the behavioral tests. Put your purchase order collection into both a valid and invalid state by whatever means you feel is appropriate (mocking or newing up concrete classes in the desired state), and test that the logic for each state of PurchaseOrderCollection is properly executed and not just that PurchaseOrderCollection is simply in a valid/invalid state.

PurchaseOrderCollection will always have a dependency on another class since it's a specialized collection. Knowing that IPurchaseOrder has a IsValid property isn't any different than knowing that the concrete PurchaseOrder has a IsValid property. I'd stick with the simplest thing that works, e.g. a concrete PurchaseOrder, unless you have much reason to believe that you will have multiple types of PurchaseOrders in your system. At that point, a PurchaseOrder interface would make more sense.

Ben Robbins
+1  A: 

i may be missing some context here, but it seems to me that you must 'couple' your test in the manner of your example, otherwise you aren't really testing anything (except an IsValid property, which is trivial).

mocking the purchase order gains nothing - you've tested the mock, not the real class

using a stub - same thing

it is okay - if not mandatory - to white-box test when using TDD

Steven A. Lowe
Sorry, the context might be that I have tests covering IsValid and PurchaseOrder in general. If I change the definition of IsValid, or something modifying PurchaseOrder, I would expect IsValid tests to be changed. I just didn't want HasErrors to be changed -- it only cares about that single prop.
eyston
@huey: i still don't see the problem - your test looks correct to me, given the description of the mechanism and the intent of the test code. Are you antitipating things that might not happen? YAGNI! ;-)
Steven A. Lowe
A: 

First, remember that you're testing the collection, not the PurchaseOrder, so that's where your effort goes. It depends on how complex PurchaseOrder is. If it's a simple entity with obvious behaviour, then it probably makes sense just to create instances. If it's more complex, then it makes sense to extract an interface as you described.

The next question that raises is what's in that interface. What is the role that objects in the collection need to perform? Maybe you only need to know if they're valid, in which case you can extract IValidatable and narrow the dependencies in the code. I don't know what's true in this case, but I often find that I can use interfaces to push me to more focussed code.

Steve Freeman