Mock To Test the Outcome, Not the Implementation

The other day a colleague came to me with a question about a problem they had trying to verify an expectation through a mock – they were trying to mock the invocation of an indexer property in C# with Rhino Mocks. Not knowing Rhino Mocks at all well I had to pass on the specific question, but it did raise other questions and it seems there was a ‘smell’ emanating from the test & code that my colleague had also picked up on…

The Feature

The code they were trying to test was a factory method that was going to be invoked by some 3rd party application framework. The method it seems has more than one responsibility forced upon it by the framework. The code  looked something like this:-

public IView CreateView(IState state, string viewName,
                        string areaName)
{ 
  var view  = IoC.CreateObject<TView>(); 
  var viewModel = IoC.CreateObject<TViewModel>(); 

  viewModel.Load(state); 
  . . . 
  view.ViewModel = viewModel; 
  . . . 
  var area = ViewManager.Areas[areaName]; 
  area.AddView(viewName, view); 
  . . . 
  return view;
}

Now, the code is already doing quite a bit more than I would expect a factory method to do, but that is down to the framework having what I consider excessive demands.

The Test

As I said above I’ve had very little experience with Rhino Mocks, and this post is not about that product which is undoubtedly very clever and very useful. No, it’s about what is being tested with it (the syntax is probably slightly wrong but I hope you get the idea):-

[Test]
public void testCreateView()
{ 
  . . . 
  Expect.Call( viewModel.Load ); 
  Expect.Call( ViewManager.Areas[areaName] ); 
  . . . 
  var view = factory.CreateView(. . .); 

  Assert.That(view, Is.Not.Null); 
  . . .
}

Let’s put aside test naming conventions for now and focus on what’s inside the curly braces. The test is attempting to verify (amongst other things) the following two behaviours:-

• The state is loaded into the ViewModel object
• The view is registered with the view manager

The big problem I have with the test is that it is not verifying that the outcome of the test matches these expectations but that it is verifying exactly how those expectations are being met. In essence the feature specification appears to be saying this:-

• The state is loaded into the ViewModel object by calling the Load() method
• The view is registered with the view manager by associating the view with named area accessed via the indexer property

In short, why should the test prescribe the mechanism used to implement the feature?

Variations On a Theme

I’m going to discuss the “view registration” expectation first because I have another thing to say about the “state loading” that is more design related.

The expectation of the application framework is that the created view will already be registered with the “view manager” by the time the factory method returns. But we may be able to implement that in more than one way depending on what the interface to the view manager looks like. It’s not uncommon to augment a simple interface with different overloads (and extension methods) to allow the caller to write simpler code:-

viewManager.AddView(area, view);

vs

viewManager[area] = view;

You may also choose to introduce your own facade over the abstraction and therefore invoke it indirectly:-

// Does all 3 necessary steps
myViewManagerFacade.Register(area, name, view);

I’ll leave it up to you to decide which you prefer, but my point is that your test should not care which of these many forms are used. In fact it should be possible to switch between them so long as they still adhere to the contract. By not being overly specific in your tests you stand a better chance of not writing brittle tests that break in the face of minor refactorings. One such refactoring that I’ve done in the past is to simplify an interface by introducing extension methods to add syntactic sugar so that the interface remains as simple as possible; should I change the expectation to now be on the extension method or the new interface method? I don’t believe that you can mock extension methods and so now your test code looks inconsistent with your production code. If you introduce a facade, that itself is mockable, do you verify that interface call or the underlying one? From the test’s perspective the outcome is the same and that is what ultimately matters.

So what would my test be? Well, if the real view manager type brings too much baggage to allow me to use it directly in the test I’ll create a mock that will implement the bits I need, i.e. record the results of the interaction:-

public Dictionary<…> m_views = new Dictionary<…>();

public void AddView(string area, IView view)
{ 
  Views.Add(area, view);
}

In mocks I have absolutely no qualms about using public fields because the test is the important thing and the mock is a means to a different end than production code. Then in the test I just assert that the view was registered with the correct name:-

{ 
  var viewManager = new MockViewManager(); 
  var factory = new Factory(viewManager, . . .); 
  . . . 
  var view = factory.CreateView(state, . . .); 

  Assert.That(view, Is.Not.Null);  
  Assert.That(viewManager.m_views[area],
                                 Is.EqualTo(view)); 
  . . .
}

This is a manual mock and no doubt Rhino Mocks makes this kind of thing even easier to write if I just bothered to RTFM. The key point is that I’m only testing the outcome, not the means by which that outcome is achieved. I can change the factory implementation in the ways mentioned earlier and it in no way invalidates the test because from the application framework’s perspective I have met the contract.

Reinventing The Wheel

One thing that worries some people is that I’ve just re-implemented the ViewManager purely to test some other code and have therefore wasted my time re-inventing the wheel and created even more code that needs to be maintained. Well, yes and no. Note that I said I’d only do it if I couldn’t reuse the real ViewManager in my tests. Unit testing is about isolation and determinism – you don’t want a lot of dependencies making your tests brittle. If the real ViewManager cannot easily be plugged in by mocking its dependencies then yes I’d mock it, but if I can reuse it then no I wouldn’t.

When it comes to integration and system testing things get harder and at that point you may have to substitute more fully featured mocks to avoid taking a troublesome dependency on, say, an externally owned service. I may provide many mock implementations of the same service to suit the different types of automated tests being run.

Flexible Designs

So back to my other issue. The reason my colleague added the Load method was in part so that he could verify that the ViewModel object had been initialised from the persistent state. He suggested that in real world use the view model is unusable if this hasn’t happened and I get his point. But I would argue that it is not necessarily the responsibility of the factory to ensure that this happens. Once again we have to put ourselves in the shoes of the client (the application framework) and think how this would be observed – how can we interact with the resulting object in a way that depends on this interaction having being played out?

There are two ways to ensure that this orchestration happens:-

1. Explicitly, by the factory itself or some other mediator in the chain
2. Implicitly, as part of the ViewModel object’s construction

The first approach, which is the one my colleague had chosen, raises other questions such as what happens if someone calls Load a second time later? Or what behaviour can be relied upon between construction and Load being called? Two-phase construction is something I really try and avoid because when somebody does do the unexpected it can manifest as one of those hard to find bugs. And so I favour that latter, after all it’s called a constructor for a reason because it’s expected to construct an object :-).

The obvious follow-on question is “if I delegate responsibility of state loading to the ViewModel constructor then do I still need to test if the object returned from the factory is fully constructed?” Almost definitely “Yes”. There is a reason the method takes arguments and so you are probably expected to use them. If you have overloads of a factory method that do different things, say, one default constructs a view and the other constructs from an external resource you’d want to know that the objects were constructed in the appropriate way.

Now let’s add another common design choice into the mix – lazy loading. Does it matter if the state loading happens directly during construction, asynchronously in the background or even synchronously as some by-product of a method call that depends on the object having been initialised? The test should succeed either way because the object is either fully constructed up front or by the time the assertions attempt to verify the expectations.

Once again let’s compare the two approaches. The original test constrains the implementation into calling a specific method called Load, whereas the latter allows for a number of different implementations. It also allows the code to be refactored between the various implementations without affecting the tests.

Test Driven (Development|Design)

I think this kind of problem highlights the difference between the last word in TDD being interpreted as [D]evelopment or [D]esign. In the former you know what production code you’re going to write and therefore you bias the test towards verifying what you’re going to implement. But in the latter you don’t really know or care how it’s going to be implemented and so the test only verifies that the design allows for it to be done in some way. And the best way to achieve that is to only verify the results of any interactions and not the interactions themselves.

Using Extension Methods to Extend A C# Interface

When I first read about extension methods being added to C# (which was a long time before I actually started using C#) I thought they were a liability. Superficially it seemed to be “an accident waiting to happen” because it’s hard enough trying to understand the responsibilities of a class or interface when you have the code in front of you. With extension methods there could be behaviours bolted-on by a third party that you just can’t see.

Once more the classic quote from Spider-Man “with great power comes great responsibility” rears it’s head. Used judiciously extension methods are a great way to graft on functionality that a class author hasn’t provided, or a way to provide a Fluent Interface to a particular problem domain. I would hazard a guess that the String class has had significantly more extension methods than any other as I personally have a written a few obvious ones such as IsEmpty and JoinUsing.

Extending concrete classes probably makes a lot of sense, but extension methods can also be used to extend interfaces. That sounds a but more surreal – how do you extend an abstraction in a concrete way that applies to all implementations of that abstraction? There is nothing magical it’s really just turning the idea of an abstract base class on it’s head…

An Initial Configuration Mechanism

All non-trivial systems end up with some common configuration mechanism and my current one is no exception. Whilst bootstrapping the initial components I didn’t want to get into the nitty-gritty of the .Net ConfigurationManager class so I introduced a simple interface:-

public IConfiguration
{ 
  string Setting(string name); 
  string Setting(string name, string defaultValue);
}

The first overload will return the setting value or an empty string if there is no setting with that name[*]. The second overload returns the default value specified when there is no value to return. The natural implementation for the first overload then becomes:-

public string Setting(string name)
{ 
  return Setting(name, “”);
}

I tossed and turned about leaving the first overload out as it forces the implementer of IConfiguration to implement something that is really just syntactic sugar. Initially the only way round this that I knew was to introduce a default implementation so that the implementer just had to derive from it to get the syntactic sugar method:-

public abstract class ConfigurationDefault
                       : IConfiguration
{ 
  public string Setting(string name) 
  { 
    return Setting(name, “”); 
  }
}
. . .
public class XmlConfiguration : ConfigurationDefault
{ 
  . . .
}

The use of abstract base classes is a tried-and-tested approach to reusing code in a polymorphic class hierarchy and so I was happy. At least until some new requirements came along.

Improvements to the Configuration Mechanism

The implementation served us well to start with but I knew from experience that there are certain behaviours I’ve found really useful, such as being able to specify environment variables like %ProgramFiles% and %TEMP% in file paths that are then expanded on demand. By now I had started getting to grips with extension methods and quickly realised that this was a behaviour that was also independent of any IConfiguration implementation, so I wrote it as an extension method:-

public static class IConfigurationExtensions
{ 
  public string PathSetting(this IConfiguration 
      configuration, string name, string defaultValue) 
  { 
    var setting = Setting(name, defaultValue); 

    // Do substitutions 
    . . . 
  }
}

One Method to Serve Them All

After adding this support and a further behaviour to allow arbitrary substitution of other settings I found that it was all too easy to mess one up and get odd behaviour. We decided that the silent failure when doing substitutions was a bad idea and that instead we’d prefer to throw an exception if a setting or environment variable did not exist. But that caused a problem because the original interface was not designed to allow the absence of a setting to be detected, it just returned an empty string or the default value and I didn’t want to get into passing a random default value just to detect if the setting existed or not.

At first I thought that I should just extend the interface with a TryXxx() style method that would return true or false along with the value if it existed. But it was whilst thinking through the ramifications of this change that I realised that this was in fact the only method that I needed in the interface:-

public IConfiguration
{ 
  bool TryGetSetting(string name, out string value);
}

All the other methods could be written as extension methods implemented in terms of this one interface method:-

public static class IConfigurationExtensions
{ 
  public string Setting(this IConfiguration
                        configuration, string name) 
  { 
    return Setting(name, “”); 
  } 

  public string Setting(this IConfiguration 
      configuration, string name, string defaultValue) 
  { 
    string value; 

    if (configuration.TryGetSetting(name, out value)) 
      return value; 

    return defaultValue; 
  } 

  public string PathSetting(this IConfiguration 
      configuration, string name, string defaultValue) 
  { 
    . . . 
  }
}

This also meant that I could drop the abstract base class that provided the default implementations of the syntactic sugar methods.

OK, so I haven’t exactly turned the abstract base class on its head, I’ve really swapped an abstract base class for a static helper class and with that I have lost the ability to re-implement the methods another way in the derived classes. But the whole point of the base class was not to enable polymorphism but to provide reusable helper methods – inheritance was just a means to a different end.

My original fears about extension methods lay unfounded and instead have provided a much cleaner solution to the problem of extending an interface to provide syntactic sugar (e.g. a fluent interface) where there previously was none. This revelation will no doubt be nothing new to experienced LINQ users because that’s exactly what it provides for IEnumerable, but for me it’s added a new dimension to how I think about and design my own interfaces.

 

[*] Yes I could have returned ‘null’ but my C++ roots were telling me that I should return a value not an empty reference (or NULL pointer in the C++ world). I still struggle with this duality of the String type – null vs “” – although as I start to use nullable value types more in the Data Access Layer I feel I’m getting to grips with it slowly.

You Write Your SQL Unit Tests in SQL?

One of the questions that came up after my ACCU London talk back in January (xUnit Style Database Unit Testing) also came up last week after my ACCU Conference talk (Using xUnit As a Swiss Army Testing Toolkit):-

Why do you write your database unit tests in SQL?

My immediate and somewhat flippant response is:-

Why wouldn’t you write your SQL tests in SQL? You wouldn’t write your Python unit tests in C++ would you?

Now, if you swap the words Python and C++ around in that sentence it begins to sound more plausible. In fact it’s an idea that I’ve looked into in the past, but not so much at the unit test level, more at the component/integration level. Kevlin Henney, in his ACCU London talk last year, pointed out that you could use NUnit[*] to unit test C++ code via the magic of C++/CLI so using a different language to express your tests is by no means wrong but it comes with another cost…

Portability (People Skills)

Steve Love did a talk at the ACCU Conference back in 2009 about portability. Of course what most people think of when you say portability is about the source code and how it works across multiple platforms (e.g. Windows/Unix) or across multiple toolchains (e.g. Visual C++/GCC). But there is another aspect to it and that is portability across people – can you hire another person to do the same job?

I’ve worked in small teams where everyone has to do everything and also in larger teams where people specialise in a particular language or technology. It is hard enough to find good people as it is, adding another orthogonal requirement to the role only makes the search that much more difficult. OK, so many experienced T-SQL developers will probably also have some knowledge of C# and the Cool Kids will happily remind you how we should all be Polyglot Programmers and be able to work with multiple languages, but the organisations I work in don’t get to attract the superstar programmers. Sometimes you’re ‘given’ members of staff from another team, presumably because they can save money by avoiding firing one and hiring another in the mistaken belief that you will turn them into a superstar programmer.

Isolation (Tooling)

Even so, just because they might know C# doesn’t meant they feel comfortable using, say, Visual Studio as their main development tool. Our automated test runner currently uses SQLCMD and we (can) write our SQL code and tests using just SQL Server Management Studio (SSMS). This means that not only are the tests expressed in their language of choice, but the tooling is also the one they are likely most comfortable with. I’ve not tried SQL debugging with SSMS but I would have thought that it is far simpler if you’re not trying to debug across a high technology stack (it feels akin to debugging across RPC calls).

That last point sums up my approach to unit testing, which is that it is all about isolation. That doesn’t just apply at the code level – at the thing under test – but also to the toolchain that sits around it. That doesn’t mean that you should only use primitive tools like Notepad & SQLCMD to develop your code, but that you try and avoid having too many other layers (e.g. C#, ADO.NET) ‘above’ the code in your test scaffolding as it makes it unnecessarily complicated and can make it harder to do things like debugging.

The Right Tool For the Job

Deciding what the right tool is for any job is getting harder every day because new tools are springing up all the time that bring the best bits from other tools together to solve some other hybrid problem. I’m definitely sold on the idea of writing SQL unit tests in SQL as the language provides enough features to allow you to write fairly clear tests, but our Heat Robinson style test runner is clearly begging to be replaced by something better. I also need to look again at how Visual Studio studio approaches the problem because they have an army of developers [hopefully] much cleverer than me that should be able to balance these requirements. I also promised myself I’d got back and look at how TSQLUNIT was coming along as that is also more mature than our framework.

We never set out to write a SQL unit test framework, we just did what felt natural to bootstrap ourselves and that’s the way it ended up. Looking back I still think it feels the right way to go.

[*] Whereas other languages seem to have a few key unit test frameworks, C++ is blessed with a bucket load. At the ACCU Conference last week someone asked for a show of hands of those people that had written a unit test framework and there were quite a few hands. I’ve said before that I believe it’s somewhat of a Rite of Passage and writing a unit test framework seems to be common part of that voyage of discovery.

Database Enumerations – A Non-Performant Query

Back in September last year I wrote about how we have implemented constants and enumerations in our SQL Server database (Implementing Constants & Enumerations in a Database). Essentially we have used parameterless User Defined Functions (UDFs) to act as the symbol for the constant or enumeration. At the time I said that I was not aware of any performance impact that this had, but that I would write it up when I found one. Well, I wrote a support query the other day and noticed that there was quite a difference between the query with and without the UDF. It looked something like this:-

SELECT *
FROM   Requests r
JOIN   Tasks t
ON     r.RequestId = t.RequestId
AND    t.TaskType = dbo.TaskType_Calculation()
WHERE  r.ValueDate = . . .

The two tables involved in the join have millions of rows in them and although there is an index covering part of the query the selectivity is quite low in this case. I didn’t look at the execution plans because this is a template query that I use regularly and the only difference was the explicit filter on TaskType using the UDF. The query took 13 seconds to execute.

Given how often I run very similar queries to this I suspected this was the evidence I was looking for to show how a UDF could affect query performance, and so I did the obvious thing which was to substitute the call to the UDF with the relevant constant:-

AND    t.TaskType = 2 -- dbo.TaskType_Calculation()

Lo and behold the query ran in just 2 seconds. I switched back and forth between the two forms to see if the performance was consistent and it was – 2s vs 13s. Somewhat perturbed now that our lovely idea may be an accident waiting to happen I checked the performance using an intermediate variable, which is effectively how we have used this idiom in our code (because you can’t always reference the UDF directly and it’s a bit verbose):-

DECLARE @calculationTask udt.TaskType_t
SET     @calculationTask = dbo.TaskType_Calculation()

SELECT *
FROM   Requests r
JOIN   Tasks t
ON     r.RequestId = t.RequestId
AND    t.TaskType = @calculationTask
WHERE  r.ValueDate = . . .

Tentatively I pressed CTRL+E in SSMS to execute the query… and… it returned in just 2 seconds. Just to be sure I then alternated between all 3 forms but the intermediate variable version still performed exactly the same as the hard-coded constant. Phew! This is clearly not an exhaustive test but it does give me some continuing comfort that it is a sound idea but that we now know for sure that it can have a performance impact in some scenarios.

P.S. This was on SQL Server 2008 R1.

Describe the Problem, Don’t Prescribe the Solution

The other day whilst scanning the backlog of change requests in JIRA I came across the following entry:-

Replace the use of nchar(x) with nvarchar(x)

And that’s all it said[#]. Now I knew instantly which tables the author was referring to because, like many teams, we only use a fixed width char(x) column for flag type fields such as Y/N or true fixed width strings like currency codes. The reasons why and how these couple of table definition slipped past the informal code review process is somewhat irrelevant, but suffice to say that we were days away from release and chose to take the ‘hit’ instead.

The fact that variable width text was being stored in a fixed width column was not causing any problems within the database or the back-end server code. Yes it was annoying when you cut-and-pasted result values from SSMS (SQL Server Management Studio) as you got a load of trailing spaces that you just had to manually trim:-

...WHERE t.Column = ‘VALUE     ‘

So I put this one aside mentally filing it with the other issues of Technical Debt as there was nothing to fix per-se – the mechanism was ugly but it worked. There was also no reason to suspect it may cause any performance problems either as the tables contain relatively few rows of data.

Then a conversation some weeks later with the change request submitter brought up the subject of raising Technical Debt style change requests and I opined that now we were live the database schema is potentially a whole lot harder to change because we have data to migrate as well[+]. In this instance it wasn’t even vaguely in the area of rocket science, but nevertheless it was non-trivial and ultimately had to be paid for. And then the real requirement came to the surface and brought with it a new perspective…

These fixed width columns were not causing the back-end any problems because we never consumed them – we only every compared them to string literals or ran support queries. The impending GUI on the other hand had controls that displayed some of these fields and apparently the UI framework would bloat the size of the control to allow room for what it presumably thought were always going to be long textual values. My gut reaction was abhorrence at the thought of changing the database schema of a base table just to fix a layout issue in the GUI; that is the kind of nasty coupling that causes change paralysis because managers get worried about what will break. I understood it was a pain having to manually deal with the fallout in the GUI layer but there should be other much simpler solutions that didn’t involve writing SQL scripts to fix the table schema and migrate the data – namely trimming the string somewhere in the data access layer or the SQL query used to fetch the data[*].

Hold on a second I thought. What are you doing directly accessing base tables anyway? The rest of the public interface to the database is via stored procedures and views – you’ll need to abstract that access away behind one of those before going to production. More importantly that gives us another option for remediating this behaviour without touching the schema and data, and better yet, that would be inside the database public interface and so allow the refactoring to occur without changes to any clients. Of course this then forces the issue of what the public interface to this mechanism should really be and at that point it seems prudent to design it properly if the existing implementation is going to leak out badly. But if it doesn’t we can save that job for another day.

The important thing though is that we have identified the root cause and can therefore now begin to assign some value to the potential choices and costs. When the request implies changing the schema of persistent data purely for the sake of it, the risk seems high and costly and the reward seems low. But when faced with a path that allows for incremental change so that the full cost and risk is distributed over smaller steps it seems much more appealing as you can put your spade down at any point.

So how would I have initially phrased that change request? Well a few weeks ago I would have gone for something like this (I can’t remember what the exact issue with the GUI was so it still seems somewhat lame I’m afraid):-

Laying out the XYZ controls has to be done manually because the ABC data has extra whitespace padding due to the use of nchar(x) instead of nvarchar(x) for the FGH columns. These columns should probably be nvarchar(x) anyway as they contain variable width text.

However a recent article by Allan Kelly in the ACCU magazine Overload (The Agile 10 Steps Model) contained a sidebar titled “What’s the Story?” about User Stories. This described a common format for stories:- “As a [Role] I can [Action] So that [Reason]” that felt akin to the use of “[action] [should] [when]” that I follow when writing test case names (itself a variation of the “[given] [when] [then]” style). I felt that this would allow those of us less natural writers to still construct a succinct requirement even if it does sound somewhat robotic, but more importantly it would help bring the value of the request to the forefront. User Stories is something that I’m vaguely aware of but have largely glossed over because my requirements are nearly always technical in nature and tend to come through very informal channels so any connection with ‘End Users’ is usually missing; it just hadn’t clicked that “User Stories” could just as easily be read as “Developer Stories” or “Support Stories”. So this is my attempt at writing it this way:-

As a [developer] I can [trim the whitespace from the ABC data] so that [the XYZ controls are not laid out manually].

OK, so this reads very unnaturally because the story format implies a repeatable action rather than a one off event. A small and hopefully obvious change to the wording and order though should make this more natural:-

As a developer it would save time if the XYZ controls were laid out automatically by ensuring the ABC data contains no excess whitespace.

Do you think that reads better or not? I think the value, while still somewhat intangible as all technical requirements like this are, is clearer because you are forced to justify the action rather than providing it as an afterthought. The beneficiary of the requirement is also clearly stated which helps easily separate the functional from the non-functional.

Of course what the original submitter would probably have written would still be something like this:-

As a developer I need to replace the nchar(x) columns with nvarchar(x) columns so that the GUI layout is automatically laid out.

…because the [action] would likely still be the same, but one would hope that when writing the [reason] alarm bells should start ringing as the tight coupling between the database schema and GUI becomes apparent.

 

[#] This is also a common failing of n00b posters on sites like Stack Overflow. They have already decided on what their solution is and are trying to elicit an answer that helps implement that solution rather than taking the time to explain the underlying problem in case the reason they can’t see the proverbial wood is because there are a load of trees in the way.

[+] As you will see later our well defined SQL public interface and set of SQL unit test means that it is actually quite easy to refactor the schema.

[*] Eventually an O/RM such as Entity Framework or NHibernate will no doubt be in place to tackle this, and so it will be interesting to see how you would go about dealing with this kind of problem when you don’t immediately have access to either the SQL or the code that binds the result values to the DTO members. I’m sure it is easy, but how much out of the way would you have to go?