It has been already some time since I started to oppose to idea of the Best Practice. Somehow it felt like an excuse to avoid thinking and using out-of-box solution. But I could not express my concerns well enough for the Best Practice Seekers and Followers to even start doubting it.

Now, I can use very good article Death to Best Practices written by Ted Neward. I encourage everybody to read it as well as an article linked there, but I offer my favourite part:

The point is, best practices just don’t exist. They are an attempt to take a solution to a problem out of the context and apply them across the entire spectrum, and that essentially invalidates the entire thing. A solution is only useful when considered in context.

Don’t believe me? Consider this challenge: when is it not a best practice to breathe? When you’re under water, of course.

(Unless you’re a fish. Or a frog. Or maybe a prince turned into a frog.)

Point is… context matters, folks.

I am looking forward for comments saying you actually can breathe under water, you just need to... But that would be only cognitive dissonance at work.

There is a value in the best practices, I have never doubted it, but I always suggested to use name good practices to show they are not universal truth. IMHO their value is in learning how experts solve particular problem in particular context, allowing to think whether it is a good match in our context, discussing differences in contexts and to think whether we could change our assumptions about problem we are trying to solve.

I have touched this topic before in Giving gifts versus conceiving rules and also in Is Javadoc useful?

More than year ago I was assigned to different project. To my delight team was writing not only unit tests, but also end-to-end tests. Every test class undeployed all plugins and components from server, reconfigured environment and deployed tested artefacts (not necessarily the same combination). Fantastic, tests were self-contained and independent.

But there were problems. Tests were slow and redeployment step was unnecessary in many cases, because all needed artefacts for subsequent test were already deployed and no changes were actually needed. Additionally all tests performed test setup in the old fashioned procedural way, describing in all gory details how to achieve desired state.

It could look like following code (comments are place holders for real functionality I am not going to show here).

@BeforeClass public static void beforeClass() {
    TestingUtil util = // get it ...

    util.undeployAllComponents();
    util.undeployAllPlugins();

    // initialize Database
    util.deployPlugin("daos.jar");
    util.deployPlugin("router.jar", routerParams);
    // some necessary tasks
    util.deployComponent("authenticator.jar");
    util.deployComponent("decisions.jar");
  
    User userA = // prepare user profile
    // ...
    // save user profiles
}

Instead of comments there were many lines of other code. Number of deployed components was varying between tests and also their order was not always the same (when it did not matter). Additionally there were 2 environments thus plugin router was usually unclosed with if-else together with varying DB setup. It was rather hard to track down what exactly is going on, but after some time we got used to it. If only it was not duplicated 17 times and every time it looked differently (nearly every test tested different component, but nearly all components depended on other components). Occasionally a new dependency was introduced and we needed to modify all those tests and add new one in correct place, unless it was not needed for particular test. It was messy.

Solution

I don't remember any more if I was begging our team leader to give me time to fix it or he asked me to do so, but I got the chance to find correct abstraction for this problem and to remove duplication.

I've realized our existing approach mixed together:

• dependency management
• description of scenario being tested
• deployment tasks
• varying environment
• preparation of data in database

So I have created following:

• Artefact knows what it is (plugin, component) and what other artefacts it depends on. There are subclasses for every component and plugin in order to encapsulate and reuse that knowledge.
• Scenario collects Artefacts that need to be deployed and can iterate through them in different ways (in deployment order, in undeployment order). It got more responsibilities later so I have split it to 2 separate classes.
• ScenarioDeployer is responsible for coordination of whole redeployment of scenario
• Deployer knows what environment it is deploying to and how to perform it.
• A hook for inserting custom code (e.g. for DB) between components are undeployed and deployed.

I have decided to push as much responsibilities as I could into individual objects. In this case the key was to give Artefact responsibilities of knowing what it depends on and how to deploy itself (by delegating to Deployer). Alternative with testing superclass having different methods doing deployment led to original mess. Alternative with static methods would fail too. What we've got was following:

@BeforeClass public static void beforeClass() {

    // initialize Database
    Scenario scenario = new Scenario();
    scenario.add(new AuthenticatorComponent());
    scenario.add(new DecisionsComponnent());
    createScenarioDeployer().redeploy(scenario);
  
    User userA = // prepare user profile
    // ...
    // save user profiles
}

Notice it does not mention anything about how to undeploy artefacts or which ones to undeploy. It will happen inside redeploy() and it allows for any changes in implementation as needed. It also mentions only top level artefacts (those that are tested by this test). All others, needed as dependency do not need to be mentioned, they are implicit and will be resolved.

Real solution is a bit more complex than this example, but original pseudo code I have shown does not show everything too. Main point is it goes directly to essence of scenario setup - it shows nicely what we want to use in test instead of obscuring it with details how it is achieved. There are, however, a few even more important things that are not shown:

• It was easy to achieve optimization - to undeploy only unwanted artefacts, deploy missing ones and redeploy those whose dependency changed, everything in correct order. All without touching beforeClass at all. This was possible only because it was not done explicitly, but hidden inside of redeploy().
• whole code together with optimization was heavily unit tested, so I could do changes confidently, while original duplicated test setup was tested only indirectly through tests it was meant to configure. (To be fair, there was one bug found a year later - optimization did not work effectively under all circumstances).
• Less than 1 month later after introducing it, our team leader realized we can use whole framework and existing artefact implementations in installer, we just need to provide slightly modified Deployer.
• When I returned back from my Round The World travelling I found the framework was moved from our project into shared space and it is used by other projects. Currently we are thinking we will use it for yet another unforeseen application, but it is too soon to be sure. But I think it should be possible just by providing another implementation of Deployer.

Conclusion

I have hinted in Solution that duplication of code usually means you are missing some abstraction. If you find it and make explicit you might save a lot of pain you had before, because information hiding allows you to introduce new approaches or enhancements cheaply.

With correct abstraction and well distributed responsibilities you might be surprised how your piece of code will get reused. In early 90s everybody hoped for OO languages being a solution for reusable code, but somehow it seemed OO could not deliver. IMHO it was always problem with badly assigned responsibilities (and misused inheritance). This claim could be slight simplification, but there is a lot of truth in it.

I wish I was there at the time it was decided it will be published to wider audience. I have some suspicion I have made not-so-good-decision-after-all with how Artefact handles equality. It is OK for my original task, but with wider usage it seems to be restrictive and confusing. And it is too late to change now after it is published contract ;-)

I've just found interesting post Test Driven Code Review. Its title and the 2nd sentence caught my attention:

When reviewing or just reading such code, it's often best to first read the tests.

That is something I started to use some time ago and I had very good experience with it many times. But not always. As Philip Zembrod writes further he had similar experience and coined terms well-specifying tests and poorly-specifying tests. I like them. What could be the better place to understand code you've just got to review without deep understanding of requirements and domain it touches? Well, I agree with the author it should be tests. But they have to be clean, easy to read and simple.

After being guilty myself in writing obscure tests in the past, I try to make them as easy and concise as possible for last couple of years. I think my turning point was when I (gradually with time) created a unit test using 13 mocks. Every time some of test (usually more at once) failed I had to go to deep analysis whether I really introduced bug or I just need to tweak one of those inconvenient mocks, because requirements changed.

I have learned a lot from that experience

• Limited responsibility of class will ask for less mocks. It is always possible to rearrange responsibilities in some way to allow simpler testing (and understanding both production and test code).
• Test should explain main points, hiding irrelevant details (like creation of data it needs, but they are not the reason for existence of this test; hiding details of how mocking library works while saying what you want to achieve).
• Test should be concise, only couple of lines all focused to tell you a story. What environment you have, what you do with tested class and what effects you should see as the result.

Last two points are not dissimilar to good story. You create some tension with method name and then let the readers to read through without getting them lost or loosing interest. As the result they should learn something and hopefully it will be about expected behaviour of tested code instead of how complicated mocking can get :-)

But not everybody does

Sad part is I always find hard to read tests that are completely obscuring the main point. Apart from plenty of "easy" to fix tests I have seen also couple of ugly ones. The worst was 3500 lines long test with 32 mocks! It was worse than really thick book with unappealing name. I've never bothered to look at production code. (Fortunately I did not need to). OK, there is even sadder part: I am afraid the author(s) will never learn anything from this test experience. Maybe only one incorrect thing - unit testing is meant to be hard :-)

Back to reviews

I usually try to spread good ideas and I've found the code reviews are pretty good medium for suggesting simple improvements in tests without being caught as mentoring. Many times I found developers are too proud to accept explicit help, which is pity in my opinion. Disadvantage of this approach is high probability helped person will miss the bigger picture. But that might come later.