views:

225

answers:

9

I'm wondering how I should be testing this sort of functionality via NUnit.

Public void HighlyComplexCalculationOnAListOfHairyObjects()
{
    // calls 19 private methods totalling ~1000 lines code + comments + whitespace
}

From reading I see that NUnit isn't designed to test private methods for philosophical reasons about what unit testing should be; but trying to create a set of test data that fully executed all the functionality involved in the computation would be nearly impossible. Meanwhile the calculation is broken down into a number of smaller methods that are reasonably discrete. They are not however things that make logical sense to be done independently of each other so they're all set as private.

+3  A: 

To solve your immediate problem, you may want to take a look at Pex, which is a tool from Microsoft Research that addresses this type of problem by finding all relevant boundary values so that all code paths can be executed.

That said, had you used Test-Driven Development (TDD), you would never had found yourself in that situation, since it would have been near-impossible to write unit tests that drives this kind of API.

A method like the one you describe sounds like it tries to do too many things at once. One of the key benefits of TDD is that it drives you to implement your code from small, composable objects instead of big classes with inflexible interfaces.

Mark Seemann
+1 for TDD. As Mark says, write tests first and you won't end up here.
TrueWill
Water under the bridge. The code in question was written long before there was any official interest in any sort of automated testing on the team.
Dan Neely
Pex looks interesting but the commercial evaluation version requires the Team edition version of VS and everyone on my team only has 2k8Pro.
Dan Neely
+5  A: 

You've conflated two things. The Interface (which might expose very little) and this particular Implementation class, which might expose a lot more.

  1. Define the narrowest possible Interface.

  2. Define the Implementation class with testable (non-private) methods and attributes. It's okay if the class has "extra" stuff.

  3. All applications should use the Interface, and -- consequently -- don't have type-safe access to the exposed features of the class.

What if "someone" bypasses the Interface and uses the Class directly? They are sociopaths -- you can safely ignore them. Don't provide them phone support because they violated the fundamental rule of using the Interface not the Implementation.

S.Lott
Exposing methods just to facilitate testing doesn't feel right.
Dan Neely
@Dan Neely: (1) That's why it's Test *Driven* Design. You pretty much have to. More importantly. (2) That's why Interface and Implementation are separate. The Interface exposes nothing. And (3) In Python we don't bother much with "private" and we're perfectly happy and successful. "exposure" doesn't -- in the long run -- count for much.
S.Lott
@S. Lott - interesting thought, thanks.
Andrew
@S. Lott - one question, how does that help if your previously private methods call other private methods? Even if you make all of these public you don't have an interface for them so you cannot mock them (unless you make them virtual too, which seems a bit strange). Any thoughts?
Andrew
The implementation (not the interface) has little use for private methods. Further, the implementation also has methods which are the "official" API -- the testable part -- distinct from more private implementation details. You don't have to test *every* method. You only have to test the methods that are the "official" interface. If you can't distinguish, then do away with `private` and use the Interface to provide all the privacy you really need.
S.Lott
+1  A: 

I've seen (and probably written) many such hair objects. If it's hard to test, it's usually a good candidate for refactoring. Of course, one problem with that is that the first step to refactoring is making sure it passes all tests first.

Honestly, though, I'd look to see if there isn't some way you can break that code down into a more manageable section.

taylonr
A: 

Your question implies that there are many paths of execution throughout the subsystem. The first idea that pops into mind is "refactor." Even if your API remains a one-method interface, testing shouldn't be "impossible".

Paul Sasik
+2  A: 

Personally I'd make the constituent methods internal, apply InternalsVisibleTo and test the different bits.

White-box unit testing can certainly still be effective - although it's generally more brittle than black-box testing (i.e. you're more likely to have to change the tests if you change the implementation).

Jon Skeet
The code's been around and in production that barring any bugs being discovered change is unlikely at this point. At the hand waving level the algorithm is doing something similar to lossy compression in that depending on the implementation there is more than one possible output for a given input that would be "correct" and that any major changes. As a result any major changes would likely break blackbox testing as well.
Dan Neely
+2  A: 

HighlyComplexCalculationOnAListOfHairyObjects() is a code smell, an indication that the class that contains it is potentially doing too much and should be refactored via Extract Class. The methods of this new class would be public, and therefore testable as units.

One issue to such a refactoring is that the original class held a lot of state that the new class would need. Which is another code smell, one that indicates that state should be moved into a value object.

kdgregory
+3  A: 

As mentioned, InternalsVisibleTo("AssemblyName") is a good place to start when testing legacy code.

Internal methods are still private in the sense that assemblys outside of the current assembly cannot see the methods. Check MSDN for more infomation.

Another thing would be to refactor the large method into smaller, more defined classes. Check this question I asked about a similiar problem, testing large methods.

Finglas
A: 

trying to create a set of test data that fully executed all the functionality involved in the computation would be nearly impossible

If that's true, try a less ambitious goal. Start by testing specific, high-usage paths through the code, paths that you suspect may be fragile, and paths for which you've had reported bugs.

Refactoring the method into separate sub-algorithms will make your code more testable (and might be beneficial in other ways), but if your problem is a ridiculous number of interactions between those sub-algorithms, extract method (or extract to strategy class) won't really solve it: you'll have to build up a solid suite of tests one at a time.

Jeff Sternal
To an extent we already have that. All the common cases are covered by a set of test files for a human driven acceptance test; and one of my coworkers is (ab)using NUnit to create a set of integration level tests that loads, processes, and examines the output from the test files. Due to the complexity of what's going on inside the main push is to test the internal methods separately to make sure we haven't missed any edge cases. For my sins (having done the VBA->C# port years ago), and having the best idea of what's going on inside the second part's fallen to me.
Dan Neely
+1  A: 

Get the book Working Effectively with Legacy Code by Michael Feathers. I'm about a third of the way through it, and it has multiple techniques for dealing with these types of problems.

TrueWill
Second this. It's an excellent resource for building islands of tested code out of untested and seemingly untestable code. Once you have enough tests in place, you will be able to fix and refactor the existing codebase.
Paddyslacker