Udostępnij za pośrednictwem

Response to comments : You suck at TDD #3–Design sensitivity and improvement

  • Artykuł

I got some great comments on the post, and I answered a few in comments but one started to get very long-winded so I decided to convert my response into a post.

Integration tests before refactoring

The first question is around whether it would be a good idea to write an integration test around code before refactoring.

I hate integration tests. It may be the kinds of teams that I've been on, but in the majority of cases, they:

  1. Were very expensive to write and maintain
  2. Took a long time to run
  3. Broke often for hard-to-determine reasons (sometimes randomly)
  4. Didn't provide useful coverage for the underlying features.

 

Typically, the number of issues they found was not worth the amount of time we spent waiting for them to run, much less the cost of creating and maintaining them.

There are a few cases where I think integration tests are justified:

  1. If you are doing something like ATDD or BDD, you are probably writing integration tests. I generally like those, though it's possible they could get out of hand as well.
  2. You have a need to maintain conformance to a specification or to previous behavior. You probably need integration tests for this, and you're just going to have to pay the tax to create and maintain them.
  3. You are working in code that is scary.

 

"Scary" means something very specific to me. It's not about the risk of breaking something during modification, it's about the risk of breaking something in a way that isn't immediately obvious.

There are times when the risk is significant and I do write some sort of pinning tests, but in most cases the risk does not justify the investment. I am willing to put up with a few hiccups along the way if it avoids a whole lot of time spent writing tests.

I'll also note that to get the benefit out of these tests, I have to cover all the test cases that are important. The kinds of things that I might break during refactoring are the same kind of things I might forget to test. Doing well at this makes a test even more expensive.

In the case in the post, the code is pretty simple and it seemed unlikely that we could break it in non-obvious way, so I didn't invest the time in an integration test, which in this case would have been really expensive to write.  And the majority of the changes were done automatically using Resharper refactorings that I trust to be correct.

Preserving the interface while making it testable

This is a very interesting question. Is it important to preserve the class interface when making a class testable, or should you feel free to change it? In this case, the question is whether I should pull the creation of the LegacyService instance out of the method and pass it in through the constructor, or instead use another technique that would allow me to create either a production or test instance as necessary.

Let me relate a story…

A few years ago, I led a team that was responsible for taking an authoring tool and extending it. The initial part had been done fairly quickly and wasn't very well designed, and it had only a handful of tests.

One day, I was looking at a class, trying to figure out how it worked, because the constructor parameters didn't seem sufficient to do what it needed to do. So, I started digging and exploring, and I found that it was using a global reference to an uber-singleton that give it access to 5 other global singletons, and it was using these singletons to get its work done. Think of it as hand-coded DI.

I felt betrayed and outraged. The constructor had *lied* to me, it wasn't honest about its dependencies.

And that started a time-consuming refactoring where I pulled out all the references to the singletons and converted them to parameters. Once I got there, I could now see how the classes really worked and figure out how to simplify them.

I prefer my code to be honest. In fact, I *need* it to be honest. Remember that my premise is that in TDD, the difficulty of writing tests exerts design pressure and that in response to that design pressure, I will refactor the code to be easier to test and that aligns well with "better overall". So I am hugely in preference of code that makes dependencies explicit, both because it is more honest (and therefore easier to reason about), and because it's messy and ugly and that means I'm more likely to convert it to something that is less messy and ugly.

Or, to put it another way, preserving interfaces is a non-goal for me. I prefer honest messiness over fake tidiness.

Comments

  • Anonymous
    December 18, 2015
    The comment has been removed

  • Anonymous
    December 20, 2015
    When validating, the exception message should say exactly what's wrong.

  • Anonymous
    December 20, 2015
    There's a using statement in the original code which looks wrong (I assume there's elided code??). If it's not wrong, it's a badly applied pattern. The original code is creating the object, calling a method on it which has Create in the name (we'll come back to that) and then disposing it. The method has Create in it, so you kind of assume it's creating something, but where? If it returns an object, which I'd expect, then it should have set a variable to the return value. If it's creating something internal to LegacyService, it's immediately disposed. This kind of leaves us with the third option which is you have a class that has a life cycle (Needs to be disposed) which is creating something in a completely unrelated area of the code. Context is everything and it might make more sense in the wider code, but from here it's unclear. So reading on, the using statement which disposes the class disappears. I assume this is because you're now injecting LegacyService as ILegacyService via the ctor, but if you're doing that, you've changed the lifecycle length of LegacyService. It used to live just as long as the method call, now it lives as long as the parent object. (There's also no indication the parent object implements IDisposable and/or disposes LegacyService at the end (or that its calling code would). I could have missed something, there's a lot of elided code, but the odd application of using() was one of the first things that irritated my developerSense(tm) when I read the code. The article itself makes sense and includes some good advice, it's possibly just a bad code example, or you might have just introduced a bug into your codebase :) Also, if I'm being picky (I am a detail oriented developer after all), this article is about TDD and didn't include any TDD stuff in it. It would have been nice to see that the original code was the result of TDD, which would have meant including some of the tests. TDD really involves getting the code to pass the tests and refactoring once they do, so to be on topic for TDD I'd expect to see how the refactorings didn't break the original tests (or how the original tests evolved to managed the refactoring). That said, it would have made the article longer. Make sense? Helpful?

  • Anonymous
    December 20, 2015
    The comment has been removed

  • Anonymous
    December 21, 2015
    Ian, Assuming I'm not making things up, the using came from the original codebase because the underlying LegacyService implemented IDisposable to do early cleanup. Whether it should have - or if that cleanup was a useful thing to do - is another matter. As for it not being TDD, this code came up as a question, "how do we make this non-testable code testable?", so there were no tests at the time. I wrote a few tests as I broke things apart, but got pressed for time and didn't bother writing the rest of them. I should have. I also realized I forgot to talk about one important point - that when I broke out the separate classes, the right thing to do is to refactor the validation method, comment it all out, and then TDD the tests and the code back in.

  • Anonymous
    December 21, 2015
    Lucian, Raj: I agree.

  • Anonymous
    December 21, 2015
    Wouldn't TDD suggest that you should write a test before you refactor? I understand that it is not possible to unit test the code in the shape that it is however it should be possible to create an integration test (however large that might be) before refactoring. I think we have very different approaches to how to handle legacy code. Adding interfaces to random classes, and changing the constructor parameters can have far reaching consequnces in the code base which are not easy to forsee. To create an interface for the legacy service I would have written a thin wrapper class that implements the interface that contain the legacy part. Instead of adding a new constructor I would have added a virtual factory method for the new interface. In the test I can then simply create an inherited version of the class with the factory method overriden. By doing this the class becomes testable without changing any of the class or method signatures.

  • Anonymous
    December 21, 2015
    That makes sense. Looking forward to the next one :)

  • Anonymous
    December 22, 2015
    The comment has been removed