views:

79

answers:

5

This is a class I'm a bit concerned about. My goal is to unit test the addresses list:

public class LabelPrinter
{
    private readonly IEnumerable<Address> _addresses;

    public LabelPrinter(IEnumerable<Address> addresses)
    {
        _addresses = addresses;
    }

    public Document Create()
    {
        // ... Generate PDF, etc ...
    }
}

So what is best:

  1. Use reflection to inspect the private property, or
  2. Since the original IEnumerable can be modified from outside anyway, make a public getter and test it instead?
A: 

I'm for the second solution because it's KISS compliant :)

Hemme
A: 

Particularly while learning unit testing, concerns about keeping fields private are trumped by easy testing and better coverage. Option 2.

derdo
+6  A: 

In general, private members shouldn't be unit tested, since anything the class is doing with it's private members should somehow be reflected in the externally testable behavior of the object. In other words, who cares what's going on in there, as long as its external behavior is what it should be.

Unit testing private members also couples your tests to the internals of a class, making them more brittle. If you decide to use a more efficient collection later down the road, your tests will break, even though the behavior of the object hasn't changed. You especially want to avoid reflection, since looking up properties by name means your tests break if the property name ever changes.

In other words - if you need to test the Address class, do it from its own unit tests, rather than from the LabelPrinter's tests. If you must use one of your two methods, use the second one, rather than reflection.

womp
The Address is just a data container. What I want to test is the length of the IEnumerable for example, without exposing it to the outside.
ciscoheat
@ciscoheat - what would you gain by testing the length of the `IEnumerable`? You don't need to write unit tests for .NET's assignment statements. (`_addresses = addresses`).
Jeff Sternal
I would like to know if a query returned the proper list of addresses for example.
ciscoheat
@ciscoheat - that sounds like you need to unit test your query logic. Since you're just injecting the `LabelPrinter` with the data, it's not the right place to be testing the results of your query.
womp
I know, and I wish things were that simple. :) I'm doing BDD and mostly integration testing with controllers. So most of the time I have to test the return value of a return value. If I moved all LINQ queries into a repository class I'd do that, but there are so many and I don't want to bloat the repositories with methods like "GetAddressesUsingThisSpecificQuery". Seems like it's always a fine line between convenient and clean code...
ciscoheat
+1  A: 

What are you trying to test about the addresses list here? In the example code you've provided above your job is actually rather easy because you can inject the list through your constructor. So in your tests you can access the list as such and therefore don't necessarily need to expose it again:

[Test]
public void Test()
{
    IEnumerable<Address> addresses = new List<Address>();
    LabelPrinter printer = new LabelPrinter(addresses);

    ... // execute your test here

    Assert.AreEqual(blah, addresses.get(0));
    // or any other assertion you want to make on the addresses list
    // created up above.
}
Cem Catikkas
A: 

Test Create and not the setter (which is effectively what you have here). I find testing setters/getters to be a bit of a waste of time. Esp. as most of the time the setter will have to be executed for some other test to work. They are also for the most part too simple to fail.

So rather than validate that LabelPrinter has a _addresses and it is Y, check that the output of Create includes the relevant details.

mlk