Don’t Fight The Framework – Unit Testing On Sitecore Pt II

In the last post Unit Testing On Sitecore Pt I I took a brief glimpse into some of the common barriers faced when trying to look at Unit Testing on the Sitecore (sadly, it covers lots of similar arguments outside of Sitecore too). In this post I am going to continue on an overview of strategy on how I have looked at unit testing my code.

Abstract what you have to

This USUALLY relates to static methods / classes, but occasionally it might relate to other things. In Sitecore, the most obvious examples of this are the Sitecore Context, the HttpContext and other beauties, but on occasion, it might for example how you might take a pipeline argument and adapt it to a type you can mock / stub / fake in whatever manner you choose.

I am personally a believer in applying sense to our unit test scenarios also. I have looked at the likes of FakeDb since it’s inception and am thoroughly impressed by the level of ingenuity shown by developers in the community when approaching the obvious lack of testability in the core Sitecore codebase. However, I still don’t entirely mind doing things like the following.

  • Creating Sitecore Items using the API for use as Fake / Mock / Stub input or outputs
  • Putting in deliberately broken connection strings to do the same to allow Factory.GetDatabase() to fire and again, use them in Fake / Mock / Stub inputs or outputs .
  • Creating a Virtual Sitecore User to test user based functionality, in this area the API is pretty rich and easy to deal with.

All of the above save what (to me at least) is a level of abstraction too far. I can still use them to control the mocks that I create, and I feel – as a necessary evil. Whilst technically you are on the edge of integration / unit testing, I believe the extra wrapping code required would be a step too far in terms of maintainability and additional complexity.

Try not to abstract the abstraction

A nod to Michael Edwards @ Glass on this point, this is one of his favourite diatribes, but one I am inclined to agree with.

It is this – always keep in mind that you are testing your code’s behaviour, so at a BARE minimum you should be ensuring that maybe you are try / catching and testing that you either get the result OR log an exception. There is little point in abstracting something if you are applying no logic to it.

public class ContentRepository
{
   public ContentRepository(ISitecoreService sitecoreService)
   {
      SitecoreService = sitecoreService;
   }
   
   public ISitecoreService SitecoreService { get; private set; }

   public T GetItem<T>(ID itemId)
   {
      return SitecoreService.GetItem<T>(itemId);
   }
}

In the example above – I would be wrapping the Glass SitecoreService implementation (and I have made similar decisions / mistakes to this in implementation). At no point am I applying any logic or doing anything useful, I am in fact wasting time and space.

There is only 1 saving grace to the above piece of code in that, the intention in this case of abstracting the abstraction was to protect against change in an external 3rd party library and therefore limit it’s effect on the code base. Still – I have seen similar with code we can control. This is the only valid use case I have ever come up with on this, and I am still not 100% convinced by it.

Note though – this doesn’t apply for example if you wanted something like (hence abstracting the abstraction):

public interface IContextRepository
{
   string GetCurrentLanguage();
}

public class ContextRepository
{
   public string GetCurrentLanguage()
   {
      return Sitecore.Context.Language.Name;
   }
}

In this (semi) valid example above, we are looking at a way to allow us to mock the return from the static Context and therefore allow us to control it’s behaviour in our tests, allowing tests for empty / null / invalid language names etc.

Integration test the code you have abstracted

To put things in perspective first consider the following example:

I probably should have mentioned this in the first post, but – Sitecore has some 20,000 + installations in the wild, even if 5% are currently the same version as you are running, that gives 1000+ active installations that are currently using THE SAME CODE that you are, they will most likely encountered the same or similar issues, and will have most likely contacted Sitecore support about it, who (by now you hope) have a shippable workaround or fix. If you then consider, of the 1000 active installations, each of these will likely have minimum 2 x content editors plus say 1000 (to be conservative) uni daily visitors). It means you have an active test spread of 1002 x 1000 (= 1002000) folk testing area’s of this code base on a daily basis. My maths would fail me on the actual likelihood of hitting the same code you are using, but I am pretty sure, as a test coverage percentage, that’s pretty reasonable, and that’s not even counting users on other versions of the code base where it hasn’t changed (think kernel or nexus which remain untouched for many versions).

Even if they haven’t covered that code in the same manner as you – you (or your client) pay’s their support – let it be Sitecore’s problem and in doing so, sharing that knowledge with the remaining 1001999 users that might be suffering the same issue. SO… In short (in an extreme example)

[Test]
public void CanGetAnItem()
{
   Database webDatabase = Factory.GetDatabase("web");
   Item item = webDatabase.GetItem(new ID(TestConstants.HomeItemId));
   Assert.IsNotNull(item);
}

Is probably a massive waste of your and your companies time. Again – test YOUR code.

And therefore:


public class DatabaseWrapper
{
   public DatabaseWrapper(Database database)
   {
      Database = database;
   }

   protected Database Database { get; private set; }

   public Item GetItem(ID itemId)
   {
      return Database.GetItem(itemId);
   }
}

[Test]
public void CanGetAnItem()
{
   Database webDatabase = Factory.GetDatabase("web");
   DatabaseWrapper databaseWrapper = new DatabaseWrapper(webDatabase);
   Item item = databaseWrapper.GetItem(new ID(TestConstants.HomeItemId));
   Assert.IsNotNull(item);
}

Wastes more time still!

The constructor property convention

This is for sure a coding style thing, but I like to always give my constructor parameter arguments at least a protected property with a private set, this BY CONVENTION dictates that we are expecting the setting of this to take place in the constructor.

Of course, as a coding style you could choose to add the readonly keyword in front of your private field – this is equally valid, but for ease of reading, it should be one or the other.

Don’t limit yourself to constructors

There are many occasions that things like property injection will serve your purpose just as well, just apply intelligence to when you can use this.

Just because it can’t be mocked, doesn’t mean it can’t be tested

As with the above, there are occasions where you can look at property injection, stubs, fakes and many other techniques to work around code that is maybe a little rigid.

‘You cannot use Dependency Injection with pipelines / commands / controllers in Sitecore’

In short, if you hear the above phrase, someone is telling porkies. You CAN do it, it does introduce a dependency on either a service locator (anti) pattern or on Sitecore factory objects (google this cause it’s worth knowing about). Either for my mind are advantageous over not using it.


public class MyPipeline
{
   // WHAT IS THE POINT IN SPECIFICALLY UNIT TESTING THIS??
   public MyPipeline() : this(ServiceLocator.Resolve<IMyService>())
   {
   }

   public MyPipeline(IMyService myService)
   {
      MyService = myService;
   }

   protected IMyService MyService { get; private set; } 
}

The above is a great example of where you could commit test coverage vanity (see below). There is no point whatsoever in testing that the service locator version works. Sure… test the service locator, test the pipeline BUT – don’t test that the ‘this’ keyword works, that’s test coverage vanity.

Be sensible in your quest for code coverage – Test Coverage Vanity

Lets take the following example – Sitecore pipeline arguments. Now there are several instances where the incoming args field would require the Sitecore context to instantiate effectively – time that would be perhaps better spent elsewhere, so consider this in the context of the pipeline above:

// The context request args (imaginary) might require a lot of the Sitecore API to be instantiated - particularly the // init pipeline. 
public void Process(ContextRequestArgs args)
{
   myService.DoSomething(args.Item, args.Name);
}

If I was to test the code above – I again, would likely be committing (what I call) test coverage vanity – it is largely on the pointless side.

We will almost certainly (if we are good enough) have tested the living crap out of the IMyService.DoSomething(Item item, string name); implementation. So – for the sake of fighting with the API for a couple of hours to instantiate a method on a pipeline that is performing a single line of code vs 10 seconds for the [ExcludeFromCodeCoverage] attribute (keeping your stats high enough and giving a positive acknowledgment that there is a reason that it’s not worth testing), you are much better off taking that vanity hit and doing the best for your client.

TDD is test DRIVEN development

I am a massive advocate of TDD as I am myself in fact lousy at testing my own code without it. By forcing myself into the mindset of ‘ok, so what is gonna screw my code scenario up’. I instantly force myself to consider the basics – nulls, timeouts, empty objects, general exceptions, nothing on the other end and so on. I am therefore hugely of the opinion that correct way in reality to do TDD is actually probably closer to (T)est (D)riven (R)eview (D)riven (D)evelopment.

  • Write a test that fails
  • Write the code to make the test pass
  • Self review / pier review to ensure the success and failure conditions are covered (code coverage metrics are now your friend)
  • Write the tests that cover any missing conditions
  • Write the code that makes those tests pass

Before I have even released code, in effect I have done 3 – 4 rounds of TDD, and assuming I have followed SOLID principles, my code will likely be far better than the original implementation.

Your grandmother / mother / father / auntie / whomever no doubt once told you – ‘Never put off til tomorrow what can be done today’, a point which in principle at least is mirrored by Robert C Martin in at least a couple of his books. Trying to test after development (especially if you are not an experienced TDD developer) will just never happen.

Conclusion
I could literally keep talking on this subject for months, I hear the same excuses, same arguments going over and over. My genuine REAL WORLD opinion on TDD is that it probably adds a 15% up front cost to your new development but saves probably 40% once per development cycle when the codebase is maybe a year old. I (unfortunately) in the Sitecore world see a lot of code that reminds me remarkably of old classic asp / inline php.

Related Posts

Don’t Fight The Framework Series

Advertisements

One thought on “Don’t Fight The Framework – Unit Testing On Sitecore Pt II

  1. Pingback: Life Through A Lens – Unit Testing your .net code using Glass Mapper | CardinalCore

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s