When is a unit test fragile?

Overview

Recently I had the misfortune to discover that several unit tests for a piece of code I'd put in place had been butchered, when I raised concerns with the person in question I received a tirrade of hatred.  The sad thing is I didn't disagree with everything he was saying just the vile and aggressive way in which he tried to get his point across.

His argument was that he'd changed code that had no links with the code being tested, an incorrect assumption, mind you this particular guy does assume that all the code he writes works so if something isn't working it's the other code that his uses which is very frustrating!

What he had changed was the way in which a table was constructed in a database so that instead of the id being generated it was now provided by the system under test, the unit test that failed was that the date added to the database matched the date of a file.  The trouble is this test does require data to be inserted in the table whose structure has now been altered albeit only slightly.  The real issue is that an assertion was made on the exact SQL insert text being sent to the database, the solution should have been to make the assertion less strict, however the "solution" put in place was to rip out any mock objects and any assertions being tested for in the unit test and replace the mock connection with an actual database created each time the test is run.

I didn't appreciate the unprofessional commit comment either:

"fixed broken unit test and changed it from using EasyMock to work out if the date is added correctly ( which is crap and fragile as evidenced by it breaking!) to using an actual database and then reading the values back and asserting that he values are correct."

Anyway not wanting to be as arrogant as this guy I decided to investigate if he's actually right, is the test fragile?

Is the test fragile?

I discovered a nice little site that explains everything to do with testing, following the flow chart we end up at "Probably Behaviour Sensitivity" since the tests compile, don't error and some code has changed.  Now since the table construction has changed the cause is identified as
"the functionality they use to set up the pre-test state of the SUT has been modified"

Reading further in this article you can see we're approaching the "Cause: Overspecified Software"
 section, however I see the database being the separation between the SUT and an integral module therefore think the reason does still fall under the heading of behavioural sensitivity but it is sort of a grey area, so here's my reasoning which I invite people to comment on since hearing other opinions is the only way I can improve my understanding.

Test: dataWrittenMatchesFileDates

A "DatabaseWriteHandler" requires a DBConnection, DatabaseReadHandler and HierarchyBuilder thus:

As in any good object these composites are interfaces that can be freely injected into the DatabaseWriteHandler at construction time.

The injected items can be mocks with expected behaviour ie. expected calls to be made on them.  Since "Statements" are created by making a call on the DBConnection it makes sense that DBConnection should return, when asked, a new mock PreparedStatement.

So we have this arrangement

So we get a connection object to create statements that we use to write to a database.  The database itself has a structured language to communicate with it, but not only 'it' but 'any' conformant database, we should therefore be able to replace DBConnection with details for any database but leave the rest of the system untouched.  By using mock statements we're able to do this.

Since the isolation point between our system and the database is:

1. At the point statements are requested.
2. The point statments are "sent" to the database

It seems logical to ensure that what our code is sending to the database makes sense, therefore it seems logical to ensure our code sends "valid" SQL since this is the common language we're using.  We ensure the writer makes valid inserts by using:

expect( mockStatements.execute( "INSERT INTO TABLE(COLUMNS) VALUES(THE VALUES EXPECTED)" ) );

We can now test the system to ensure that it's trying to add the data we expect without the need for an actual database in the test.

THE BAD WAY

Imagine now that rather than using mocks we're using an actual database.  The DBConnection, DatabaseReadHandler are no longer mock objects and they're all talking to an actual database, there are therefore no expects on the calls being made.

Scenario #1

The database creation fails for an unknown reason, this causes the dataWrittenMatchesFileDates tests to fail when no code has been touched!

Scenario #2 - and the worst of the two

The production system changes the database that it's using and the SQL that this new database accepts is more strict than the derby database used for the tests, or the type for TIMESTAMP changes e.t.c. The tests all pass because they're using a derby database however the data isn't really added in production and the first we know of it is when customers data is broken.  Now we see why this system is bad!

Or even the very case when this shortsighted change was made, because there's no longer any checks on the data sent to the database  if the dates come back correctly then this test will pass whereas any of the other data being written might be garbage and we wouldn't know until a customer discovered it.

Conclusion

As I said at the beginning of this article I am not sure this is the right approach, in fact I am pretty sure the units under test are too large but this is the place we're at and until we at least get this in place we can't progress further forwards.