Boundaries of Coding for Testability
I was reviewing some code in a project I'm working on when I came across the following:
We've got a SaveOperationFactory (interface) that supplies instances of classes derived from SaveOperation. For example, if it were contact management system (which it is not, but it simplifies the example), we might have a SaveContactOperation. We would have a SqlSaveOperationFactory (concrete) that returns SqlSaveContactOperation. Note that "Sql" is the prefix for our classes that explicitly deal with a MS SQL Server database.
I noticed, on our SaveOperationFactory, a method named the equivalent of NewSavePreExistingContactOperation. Now you might think that this is used to update a contact, but I happen to know that we have only one NewSaveXXXOperation for each entity, and the SaveOperation is smart enough to know whether it's dealing with a brand new XXX or a pre-existing XXX. So I looked to see who might be using this method and sure enough, it was only fixtures (FitNesse) and tests. It turns out that the method is there to support tests that need to insert data such that it appears to have been entered in the past. This is something that the system should never do, but the tests require.
Agile thinking tells us that making code testable is a good thing - and this method is there to make the system easier to test. However, this particular example smells funny to me. In part because of the potential confusion over the name, but even if you change the name to NewSavePreExistingContactForTestsOnlyOperation, it seems wrong to me to have such a method as part of the interface of a class.
I wrote to the team and got some interesting responses, some supporting, some opposing. The most passionate opposition came from Micah:
!*
Micah: Ok... Maybe the name, SavePreExistingContact, could be more detailed so it won't be mis-understood. But the name is accurate for what the method does.
<snip from="David">
I'm not trying to argue against coding for testability. I feel it's a must. But I think that when there are more than one ways to handle testability, you have to consider the effects on the system's interface in your decision making. Even if it's a little more expensive to do it in the less intrusive way, I think the expense is well justified. Anybody else have any thoughts on this issue? How do we draw (guide) lines in terms of adding to the interface of the system under test for the sake of testability?
</snip>
Micah: We need to realize that testability is an implicit feature of the system. If SavePreExistingContact "feels" like it's part of the system.... that's because IT IS! It has to be there so the tests can use the system. Maybe real users won't use that particular bit of functionality, but the tests are just another user, and the tests do use it.
I do NOT think the expense is justified. We use principles and good coding practices to make sure we build high quality, maintainable code. Why then do you consider making "less intrusive" solutions? This would violate the priciples we use, producing lower quality code that is harder to maintain And the code you want to compromise, the code that makes the system testable, is really part of the system in the first place.
*!
There were some other notable points made in subsequent email:
- I noted that this was an Interface Segregation Principle (the only clients who need this method are tests)
- Micah noted that the whole SaveOperationFactory was an ISP violation because each client within the system actually only needs access to one of the methods on it.
- I suggested that if you look at ISP from a higher level - thinking of the differing clients as "the production code" and "the test code", that you can view it more clearly as a single ISP violation rather than multiple ones.
- Micah noted that removing this method from the SaveOperationFactory interface means that tests would have to switch on mode (we can run the tests against InMemory versions of our SaveOperations to test business logic without a database, or against Sql versions to do end to end testing) - a violation of Dependency Inversion Principle and Open/Closed Principle. Followed with the following quote: "Why should the tests suffer from bad design?"
There are a couple of very interesting questions this leaves me with. One is whether or not we should treat tests as a first class client of the code, providing interfaces for them the same way we provide interfaces for other parts of the system. In my opinion tests are first class citizens, but they also have additional responsibilities. They help us understand how the system works. In the example above, a test was using NewSavePreExistingContactOperation. Anyone looking at that test would be misled to believe that that's how you use the system.
The fact that they serve as documentation also means they have a responsibility to be clear. Calling a factory from within a test (that is not specifically testing that factory) means that the reader doesn't know what object he's getting when the test is run. He's got to step through a debugger, or add print statements that are hopefully (often not) removed before committing code. By explicitly instantiating a specific class, the test has more clarity. And if you can run the test in two modes, each which expect a different class, then this ...
if (inMemoryMode)
new InMemorySavePreExistingContactOperation(contact).Execute()
else
new SqlSavePreExistingContactOperation(contact).Execute()
... actually provides more clarity about how the system works to the reader, in spite of the violations of DIP (the test is depending on concrete classes) and OCP (new modes would require changes to this code). To me, this is more acceptable than the violation of ISP in the SaveOperationFactory - again, this specific violation occuring at a higher level than classes - and only making sense if you accept the notion that tests are fundamentally different from other clients.
Which leads me to the second question - do the class design principles apply to test classes as well as classes under test? I think the answer is yes, but with a caveat that contextual clarity is more important in tests than it is in classes under test, which, if they are designed well, should not care about context.
!commentForm
I think that there are problems when the SUT reflects the testing code. I think that it violates the idea of code normalization in addition to the violation of ISP given a above. In addition, it pollutes the surface area of a class. Those are all things that I take (perhaps too) seriously.
Now, that having been said, is the special save operation required to be accessible only from that factory? Does the save method have to be a SaveOperation[?] in the sense that "normal" saves have to be done that way? I think that the test scaffolding is different. It is supposed to be very broad and not very deep. It's just a way to access the real stuff. It's intentionally descriptive if read from top to bottom (hopefully). It is supposed to be all surface area, and all the "meat" of the test is in the SUT. I don't see a problem with the testing code having its own data-prep routines. I have a problem with the actual SUT having exposed methods which are not supposed to be called.
This isn't the "rogue programmer" scenario I'm talking about either. I am a python bigot, and I believe (like the smalltalk people) that you don't hide the code, you trust the programmers. Rather, I am worried about the conceptual load of a class. EVERY method of a class has to be understood by the programmer before the class is well-understood. Every method I am supposed to read and then ignore bothers me. More surface area means that you expect programmers to know more.
Now, the "rogue programmer" issue can be brought back to life as the "simpleton programmer" issue when you have things like intellisense going on. If I am in a crummy editor that lets me see only one file at a time, and which has intellisense, then I'll type the name of the factory and a dot 'theFactory.' and up will pop a list of methods I can call. Seeing only the method names, am I to make a wise decision between the real methods and the test methods?
How much do I need to know (beyond the surface area exposed by intellisense)?
If there were a separate namespace within the object for test methods, I suppose I'd have less issue. I think that a name wart is insufficient, but a good start. I'd rather see something like an extension object pattern, or maybe something like receiving the special testing interface by adaptation, but something needs to be done to segregate the "real methods" from the "testing methods" and it needs to be unambiguous.
My $.02, take it or leave it.
Now, that having been said, is the special save operation required to be accessible only from that factory? Does the save method have to be a SaveOperation[?] in the sense that "normal" saves have to be done that way? I think that the test scaffolding is different. It is supposed to be very broad and not very deep. It's just a way to access the real stuff. It's intentionally descriptive if read from top to bottom (hopefully). It is supposed to be all surface area, and all the "meat" of the test is in the SUT. I don't see a problem with the testing code having its own data-prep routines. I have a problem with the actual SUT having exposed methods which are not supposed to be called.
This isn't the "rogue programmer" scenario I'm talking about either. I am a python bigot, and I believe (like the smalltalk people) that you don't hide the code, you trust the programmers. Rather, I am worried about the conceptual load of a class. EVERY method of a class has to be understood by the programmer before the class is well-understood. Every method I am supposed to read and then ignore bothers me. More surface area means that you expect programmers to know more.
Now, the "rogue programmer" issue can be brought back to life as the "simpleton programmer" issue when you have things like intellisense going on. If I am in a crummy editor that lets me see only one file at a time, and which has intellisense, then I'll type the name of the factory and a dot 'theFactory.' and up will pop a list of methods I can call. Seeing only the method names, am I to make a wise decision between the real methods and the test methods?
How much do I need to know (beyond the surface area exposed by intellisense)?
If there were a separate namespace within the object for test methods, I suppose I'd have less issue. I think that a name wart is insufficient, but a good start. I'd rather see something like an extension object pattern, or maybe something like receiving the special testing interface by adaptation, but something needs to be done to segregate the "real methods" from the "testing methods" and it needs to be unambiguous.
My $.02, take it or leave it.
> It turns out that the method is there to support tests that need to insert data such that it appears to have been entered in the past
I wonder if, instead of having a method in the tested class, there should instead be a method in the tests themselves that creates a universe of testable data?
I wonder if, instead of having a method in the tested class, there should instead be a method in the tests themselves that creates a universe of testable data?
Matisse - are you talking about a single testable universe that all of the tests use? Have you used that approach with any success?
I was deliberatly ambiguous on the number of "universes", although, the word implies only one :-)
I have successfully used a single "universe" in a couple of recent projects - which were fairly limited - in those projects I have the setup() method in the test class call private methods that create an entire test database (or two databases in one case), and the teardown() method removes the test database(s).
Some notes about this:
1. I did assume that the database engine (MySQL) was installed on the developers machine. This is a bigger dependency than assuming that say, the JVM is installed, but in my case I thought it reasonable. I *could* also incorporate a lightweight SQL engine as part of the test suite, but that seemed like way too much in these cases.
2. I store the test data as a file of SQL statements as part of the test suite. In another situation (an employee benefits system) I've stored a dircetory of test data, containing one subdirectory for each test employee, and each test employee has one file foir each kind of data (employee info, dependents, existing benefits selections, etc.)
I have successfully used a single "universe" in a couple of recent projects - which were fairly limited - in those projects I have the setup() method in the test class call private methods that create an entire test database (or two databases in one case), and the teardown() method removes the test database(s).
Some notes about this:
1. I did assume that the database engine (MySQL) was installed on the developers machine. This is a bigger dependency than assuming that say, the JVM is installed, but in my case I thought it reasonable. I *could* also incorporate a lightweight SQL engine as part of the test suite, but that seemed like way too much in these cases.
2. I store the test data as a file of SQL statements as part of the test suite. In another situation (an employee benefits system) I've stored a dircetory of test data, containing one subdirectory for each test employee, and each test employee has one file foir each kind of data (employee info, dependents, existing benefits selections, etc.)
In Ruby on Rails, you set up "fixtures", which create a known "single universe" in the form of a preloaded database. This actually makes me really uncomfortable because sometimes when you have new requirements and you add them to the data, you break other tests that are dependent upon the data. For this reason, I prefer to set up exactly the data I need for each test. That approach is, however, very costly - a cost that you feel every time you run all of the tests. So I guess it depends :)
It certainly seems reasonable that one would create test data on a per-suite, per-class, or per-test basis, as appropriate, like you say "it depends"
The general problem of testing software that makes changes to the outside world (e.g. databases) is a very interesting one, I think.
The general problem of testing software that makes changes to the outside world (e.g. databases) is a very interesting one, I think.
I think you can go either way on this issue, but I know what my preference is. When I look at a piece of production code, I really want to know that it is production code. To me, it's cleaner and less confusing that way, and I've been seriously confused by testing code that doesn't look like testing code.
In this case, I'd make a separate factory, one that provides operations that you only need for testing. Chances are, it could reuse quite a bit from the production factory; maybe it could share a superclass with it. This assumes that the tests are the clients of the factory. If the tests exercise code that uses the factory and the code needs operations that should behave differently under test, I'd make the factory settable.
In this case, I'd make a separate factory, one that provides operations that you only need for testing. Chances are, it could reuse quite a bit from the production factory; maybe it could share a superclass with it. This assumes that the tests are the clients of the factory. If the tests exercise code that uses the factory and the code needs operations that should behave differently under test, I'd make the factory settable.
Add Child Page to BoundariesOfCodingForTestability