Is it a bad practice to modify code strictly for testing purposes

Question

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 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 ?

Your modified method should take a Type as a parameter, not an Assembly.
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...
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.
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

Robert Harvey · Accepted Answer · 2013-05-22 20:43:38Z

up vote 73 down vote accepted

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.

answered May 22 at 20:43

Robert Harvey
48.6k10106201

2

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. – Jason Swett May 22 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 at 20:50

4

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 at 21:08

2

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 at 9:24

1

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. – AmericanUmlaut May 23 at 12:04

show 1 more comment

Mark Seemann · Answer 2 · 2013-05-23 06:16:01Z

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, the only true encapsulated code would be this:

static void Main(string[] args)

Is your colleague proposing to making 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.

+1 You feel safe exposing a public method when it properly protects its invariants.
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.

Jason Swett · Answer 3 · 2013-05-22 20:43:34Z

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!

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.
I know. I thought I addressed your main point in my second paragraph, with a "yes".

Carson63000 · Answer 4 · 2013-05-23 03:29:49Z

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..

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.
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.

Manoj R · Answer 5 · 2013-05-23 06:21:14Z

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.

Aidan Cully · Answer 6 · 2013-05-23 11:56:32Z

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.)

Ross Patterson · Answer 7 · 2013-05-23 22:03:24Z

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.

Ozz · Answer 8 · 2013-05-24 15:13:20Z

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.

Mat · Answer 9 · 2013-05-24 16:28:04Z

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.

asked	18 days ago
viewed	2706 times
active	16 days ago

Is it a bad practice to modify code strictly for testing purposes

9 Answers

Your Answer

Not the answer you're looking for? Browse other questions tagged c# programming-practices testing unit-testing refactoring or ask your own question.

Is it a bad practice to modify code strictly for testing purposes

9 Answers

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c# programming-practices testing unit-testing refactoring or ask your own question.

Related