EvidentSolutions / dalesbred

Dalesbred - a database access library for Java
https://dalesbred.org
MIT License
54 stars 15 forks source link

Add public interface to Database class to facilitate unit testing / mocking #30

Closed mmadson closed 8 years ago

mmadson commented 8 years ago

the Database class is defined

public final class Database {
...
}

I use Mockito for unit testing which does not support mocking final classes. I could use Powermock, but from my experience Powermock can lead to some unexpected test failures relating to the classpath wizardry it performs.

As such, I'd prefer to continue using Mockito and the easiest way to accomplish this is if you create a Database interface and change your Database class to something like DatabaseImpl or DefaultDatabase that way in my code i could create definitions of the form:

final Database db = new DefaultDatabase();

and come unit testing time I could mock the Database interface using mockito.

for now I'll create my own delegate, just wanted to offer a suggestion.

komu commented 8 years ago

Hi,

Thanks for your suggestion! I understand your concerns and use mocking frameworks myself as well. That said, I'm not too keen on this change. Let me explain my thinking.

First of all, renaming the implementation class and changing the way a Database is constructed would break existing users. (Of course I could keep the name of Database class as it is and just add a new interface (e.g. DatabaseService) which Database would implement, in which case there would be no breakage.)

There is, however, a second problem: having an interface would make adding new functionality harder, because simply adding a new method would be a breaking change (unless it could be added as a default method). During Dalesbred's lifetime, I've added several new methods to Database that could not have been added as default methods. If Database would have been an interface, I could not have added those methods, because it would have broken possible implementations of Database in client code. This (and API ergonomics) were there main reasons why I ended up using a final class to begin with.

Now, of course I could take a middle road and document Database clearly marking that you're not supposed to implement it (apart from automatic implementation by proxies and such) and if you do, you should expect breakage between upgrades. I considered this as well, but the benefits didn't seem to be big enough.

How do I test then? Personally I don't think that mocking at the level of Database makes much sense. At that level of abstraction mocking is really hard and makes tests hard to read. It makes more sense say to a mocking framework "when findUser is called with this parameter, return this user" than to say "when findUnique is called with this class, this SQL and these parameters, return this user".

So generally when I write code, I will hide the data access code behind a DAO-interface.

public interface UserDao {
    User findUserByLogin(String login);
}

Now that I test my code I have two options readily available:

  1. I can easily mock UserDao at a level of abstraction that makes sense for mocking.
  2. I can use an in-memory database (such as HSQL) to actually test the SQL at the same time.

I used to mock quite heavily, but during the last ten years I've grown quite fond to the second option. The tests are usually fast enough (Dalesbred's own test suite of database tests executes in a few seconds), the code to insert test data to database often ends up clearer than mocking calls and I get more coverage from my tests. Sure, they might not be orthodox "unit tests", but I'm hard pressed to see downsides if they're still fast and require no external systems to be configured.

With mocking frameworks, you can quite easily end up with test code that tests very little, but makes the system harder to change because the tests make too many assumptions about how the components are going to work instead of just verifying inputs and outputs.

Well, I guess I got a bit sidetracked. Anyhow, while I'm not going to implement this at the moment, I will definitely reconsider it if I'll release 2.x some day because then I'll get to break things. Thanks for your input!