88

I have a debate with a programmer colleague about whether it is a good or bad practice to modify a working piece of code only to make it testable (via unit tests for example).

My opinion is that it is OK, within the limits of maintaining good object oriented and software engineering practices of course (not "making everything public", etc).

My colleague's opinion is that modifying code (which works) only for testing purposes is wrong.

Just a simple example, think of this piece of code that is used by some component (written in C#):

public void DoSomethingOnAllTypes()
{
    var types = Assembly.GetExecutingAssembly().GetTypes();

    foreach (var currentType in types)
    {
        // do something with this type (e.g: read it's attributes, process, etc).
    }
}

I have suggested that this code can be modified to call out another method that will do the actual work:

public void DoSomething(Assembly asm)
{
    // not relying on Assembly.GetExecutingAssembly() anymore...
}

This method takes in an Assembly object to work on, making it possible to pass your own Assembly to do the testing on. My colleague didn't think this is a good practice.

What is considered a good and common practice ?

8
  • 5
    Your modified method should take a Type as a parameter, not an Assembly. May 22, 2013 at 20:47
  • You're right - Type or Assembly, the point was that the code should allow supplying that as a parameter, although it may seem that this functionality is there only to be used for testing...
    – liortal
    May 22, 2013 at 20:51
  • 5
    Your car has an ODBI port for "testing" - if things were perfect it would not be needed. My guess is you car is more reliable than his software.
    – mattnz
    May 22, 2013 at 22:06
  • 25
    What assurance does he have that the code "works"?
    – Craige
    May 23, 2013 at 1:09
  • 3
    Of course - do so. Tested code is stable over time. But you will have a much easier time to explain newly introduced bugs if they came with some feature improvement rather than a fix for testability May 23, 2013 at 22:11

11 Answers 11

137

Modifying code to make it more testable has benefits beyond testability. In general, code that is more testable

  • Is easier to maintain,
  • Is easier to reason about,
  • Is more loosely coupled, and
  • Has a better overall design, architecturally.
7
  • 3
    I didn't mention these things in my answer, but I totally agree. Reworking your code to be more testable tends to have several happy side effects. May 22, 2013 at 20:46
  • 2
    +1: I generally agree. There are certainly clear cases where making the code testable harms these other beneficial goals, and in those cases testability lies at the lowest priority. But in general, if your code isn't flexible/extensible enough to test, it won't be flexible/extensible enough to use or to age gracefully.
    – Telastyn
    May 22, 2013 at 20:50
  • 6
    Testable code is better code. There is always inherent risk in modifying code that doesn't have tests around it though, and the easiest, cheapest way to mitigate that risk in the extremely short term is to leave it well alone, which is fine...until you actually need to change the code. What you need to do is sell the benefits of unit testing to your colleague; if everyone is on board with unit testing, then there can be no argument that the code needs to be testable. If not everyone is onboard with unit testing there is no point.
    – guysherman
    May 22, 2013 at 21:08
  • 3
    Exactly. I remember I was writing unit tests for about 10k source lines of my own code. I knew the code was working perfectly but the tests forced me to rethink some situations. I had to ask myself "What exactly is this method doing?". I found several bugs in working code only by looking at it from a new point of view.
    – Sulthan
    May 23, 2013 at 9:24
  • 2
    I keep clicking the up button, but I can only give you one point. My current employer is too short-sighted to let us actually implement unit tests, and I still am benefiting from consciously writing my code as if I were working test-driven. May 23, 2013 at 12:04
69

There are (seemingly) opposing forces in play.

  • On the one hand, you want to enforce encapsulation
  • On the other hand, you want to be able to test the software

Proponents of keeping all 'implementation details' private are usually motivated by the desire to maintain encapsulation. However, keeping everything locked down and unavailable is a misunderstood approach to encapsulation. If keeping everything unavailable was the ultimate goal, the only true encapsulated code would be this:

static void Main(string[] args)

Is your colleague proposing to make this the only access point in your code? Should all other code be inaccessible by external callers?

Hardly. Then what makes it okay to make some methods public? Isn't it, in the end, a subjective design decision?

Not quite. What tends to guide programmers, even on an unconscious level, is, again, the concept of encapsulation. You feel safe exposing a public method when it properly protects its invariants.

I wouldn't want to expose a private method that doesn't protect its invariants, but often you can modify it so that it does protect its invariants, and then expose it to the public (of course, with TDD, you do it the other way around).

Opening up an API for testability is a good thing, because what you're really doing is applying the Open/Closed Principle.

If you only have a single caller of your API, you don't know how flexible your API really is. Chances are, it's quite inflexible. Tests act as a second client, giving you valuable feedback on the flexibility of your API.

Thus, if tests suggest that you should open up your API, then do it; but maintain encapsulation, not by hiding complexity, but by exposing complexity in a fail-safe manner.

5
  • 4
    +1 You feel safe exposing a public method when it properly protects its invariants. May 23, 2013 at 12:46
  • 1
    I loooooove your answer! :) How many times I hear: "Encapsulation is the process to make private some methods and properties". It's like saying when you program in object oriented, this is because you program with objects. :( I'm not surprised that a so clean answer come from THE master of dependency injection. I will surely read a couple of times your answer to make me smile about my poor old legacy code.
    – Samuel
    May 28, 2013 at 22:03
  • I added a few tweaks to your answer. As an aside, I read your Encapsulation of Properties post, and the only compelling reason I've ever heard for using Automatic Properties is that changing a public variable to a public property breaks binary compatibility; exposing it as an Automatic Property from the beginning allows validation or other internal functionality to be added later, without breaking client code. Jun 18, 2013 at 21:25
  • As someone that has had to work in codebases with tens of thousands of classes publicly exposed only to be tested....opening everything just for testing may offer some power, but it can also add enormous noise and complexity for actual usage of the system as a consumer. I think there may be something more here. Large API surface areas aren't all benefits. Presuming that's what applying open/closed just for it's own merits could mean in also supporting unit testing. I think hiding implementation can make things simpler to use and easier to make sensible choices up front. Jun 21, 2023 at 21:24
  • 1
    @JoshuaEnfield, yes, I actually agree, and had I written my answer today, I would have worded it differently, instead of relying on links to nuance the message. By coincidence, I recently published a new article that discusses this topic, but does (I hope) offer a more refined perspective. Jun 22, 2023 at 10:36
23

Looks like you're talking about dependency injection. It's really common, and IMO, pretty necessary for testability.

To address the broader question of whether it's a good idea to modify code just to make it testable, think of it this way: code has multiple responsibilities, including a) to be executed, b) to be read by humans, and c) to be tested. All three are important, and if your code doesn't fulfill all three responsibilities, then I'd say it's not very good code. So modify away!

2
  • DI is not the major question (i was only trying to give an example), the point was whether it would be OK to provide another method that originally wasn't intended to be created, only for the sake of testing.
    – liortal
    May 22, 2013 at 20:54
  • 4
    I know. I thought I addressed your main point in my second paragraph, with a "yes". May 22, 2013 at 20:57
13

It's a bit of a chicken and egg problem.

One of the biggest reasons why it's good to have good test coverage of your code is that it allows you to refactor fearlessly. But you're in a situation where you need to refactor the code in order to get good test coverage! And your colleague is fearful.

I see your colleague's point of view. You have code which (presumably) works, and if you go and refactor it - for whatever reason - there's a risk that you'll break it.

But if this is code which is expected to have ongoing maintenance and modification, you're going to be running that risk every time you do any work on it. And refactoring now and getting some test coverage now will allow you take that risk, under controlled conditions, and get the code into better shape for future modification.

So I would say, unless this particular codebase is fairly static and not expected to have significant work done in the future, that what you want to do is good technical practice.

Of course, whether it's good business practice is a whole 'nother can of worms..

2
  • An important concern. Usually, I do IDE supported refactorings freely since the chance to break anything there is very low. If the refactoring is more involved I'd do it only when I need to change the code anyway. For changing stuff you need tests to reduce risks, so you even get business value. May 28, 2013 at 6:32
  • The point is: do we want to keep the old code because we want to give the responsibility to the object to query for the current assembly or because we don't want to add some public properties or changing the method signature? I think the code violate the SRP and for this reason, it should be refactored no matter how afraid the colleagues are. Sure if this is a public API used by a lot of users, you will have to think about a strategy like implementing a facade or anything that help you granting to old code an interface preventing too much changes.
    – Samuel
    May 28, 2013 at 22:19
7

This may just be a difference in emphasis from the other answers, but I'd say that the code should not be refactored strictly to improve the testability. Testability is highly important for maintenance, but testability is not an end in itself. As such, I would defer any such refactoring until you can predict that this code will need maintenance to further some business end.

At the point that you determine that this code will require some maintenance, that would be a good time to refactor for testability. Depending on your business case, it may be a valid assumption that all code will eventually require some maintenance, in which case the distinction I draw with the other answers here (e.g. Jason Swett's answer) disappears.

To sum up: testability alone is not (IMO) a sufficient reason to refactor a code base. Testability has a valuable role in allowing maintenance on a code base, but it is a business requirement to modify your code's function that should drive your refactoring. If there is no such business requirement, it'd probably be better to work on something your customers will care about.

(New code, of course, is being actively maintained, so it should be written to be testable.)

3

Your problem here is that your test tools are crap. You should be able to mock out that object and call your test method without changing it - because while this simple example is really simple and easy to modify, what happens when you have something a lot more complicated.

A lot of people have modified their code to introduce IoC, DI and interface-based classes simply to enable unit testing using the mocking and unit test tools that require these code changes. I don't thin they're a healthy thing, not when you see code that was quite straight-forward and simple turn into a nightmare mess of complex interactions driven entirely by the need to make each and every class method totally decoupled from everything else. And to add insult to injury, we then have many arguments on whether private methods should be unit tested or not! (of course they should, what's the point of unit testing if you only test part of your system) but these arguments are more driven from the point of view that its difficult to test those methods using the existing tools - imagine if your test tool could run tests against a private method as easily as a public one - everyone would be testing them without complaint.

The problem, of course, is in the nature of the test tooling then.

There are better tools available now that could put these design changes to bed forever. Microsoft has Fakes (nee Moles) that allows you to stub concrete objects, including static ones, so you no longer need to change your code to fit the tool. In your case, if you used Fakes, you'd replace the GetTypes call with your own that returned valid and invalid test data - which is pretty important, your suggested change doesn't provide for that at all.

To answer: your colleague is right, but possibly for the wrong reasons. Do not change code to test, change your test tool (or your whole test strategy to have more integration-style unit tests instead of such fine-grained testing).

Martin Fowler has a discussion around this area in his article Mocks aren't Stubs

2

I think your colleague is wrong.

Others have mentioned the reasons why this is a good thing already, but so long as you are given the go ahead to do this, you should be fine.

The reason for this caveat is that making any change to code comes at the cost of the code having to be re-tested. Depending what you do, this test work may actually be a large effort by itself.

It is not necessarily your place to make the decision of refactoring versus working on new features which will benefit your company/customer.

2

I've used code coverage tools as part of unit testing in checking whether all paths through the code are exercised. As a pretty good coder/tester on my own, I usually cover 80-90% of the code paths.

When I study the uncovered paths and make an effort for some of them, that's when I discover bugs such as error cases that will "never happen". So yes, modifying code and checking for test coverage makes for better code.

1

A good and common practice is to use Unit testing and debug logs. Unit tests ensures that if you make any further changes in program, your old functionality is not breaking. Debug logs can help you trace the program at run time.
Sometimes it happens that even beyond that we need to have something only for testing purpose. It is not unusual to change the code for this. But the care should be taken such that the production code is not affected because of this. In C++ and C this is achieved using the MACRO, which is compile time entity. Then the testing code do not come into picture at all in production environment. Don't know if such provision is there in C#.
Also when you add testing code in your program, it should be clearly visible that this portion of code is added for some testing purpose. Or else the developer trying to understand the code is simply going to sweat over that part of code.

1

There are some serious differences between your examples. In the case of DoSomethingOnAllTypes(), there is an implication that do something is applicable to types in the current assembly. But DoSomething(Assembly asm) explictly indicates that you can pass any assembly to it.

The reason I point this out is that a lot of dependency-injection-for-testing-only steps outside the bounds of the original object. I know you said "not 'making everything public'", but that's one of the biggest errors of that model, followed closely by this one: opening the object's methods up to uses they aren't intended for.

0

Your question gave not much context in which your colleague argued so there is room for speculation

"bad practice" or not depends on how and when the changes are made.

In my opition your example to extract a method DoSomething(types) is ok.

But i have seen code which is not ok like this:

public void DoSomethingOnAllTypes()
{
  var types = (!testmode) 
      ? Assembly.GetExecutingAssembly().GetTypes() 
      : getTypesFromTestgenerator();

  foreach (var currentType in types)
  {
     if (!testmode)
     {
        // do something with this type that made the unittest fail and should be skipped.
     }
     // do something with this type (e.g: read it's attributes, process, etc).
  }
}

These changes made the code more difficuilt to understand because you increased the number of possible code-paths.

What i mean with how and when:

if you have a working implementation and for the the sake of "implementing testing-capabilities" you made the changes then you have to re-test you application because you may have broken your DoSomething() method.

The if (!testmode) is more more difficulilt to understand and test than the extracted method.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.