Testability of projects in Umbraco

[Keith Jackson]

Hi all,

I've been looking to get involved in doing some work to improve testability in the core, and Lee Kelleher suggested I should post here.

I raised a couple of issues in recent weeks, as changes recently (bear in mind my recent could be anywhere between 6.0 and 7.0) have made controllers impossible to test.

I also documented some potential solutions and put a rough patch together, here - http://issues.umbraco.org/issue/U4-4566 which extracts an interface from UmbracoContext to make it mockable.

Details (to save link jumping)...

Back in 6.1, the RenderMvcController inherited directly from System.Web.Mvc.Controller and didn't require any constructor parameters. In 7, RenderMvcController now inherits from UmbracoController, which now requires an umbracoContext parameter to be present.
I assume that this parameter is injected by some Autofac magic at some point in the normal lifecycle, but as the context seems completely unmockable it seems that it's no longer possible to unit test controllers.
This can be resolved by...
a) Allow the umbracoContext parameter to be null.
b) Remove the umbracoContext parameter and, instead, pass it via Properties instead (this is effectively where the context was at before in 6.1)
c) Replace UmbracoContext with IUmbracoContext to allow it to me mockable.
Solution c, seems to be the best option that I can envisage at the moment as that would also allow testing of context sensitive operations in the controller itself.

And...

Here's a patch I worked up of how this could work as a mockable interface. The patch is very simplistic, only covering the interface itself, making several Internal elements of UmbracoContext public (so the interface will expose them), and applying it to several dependent classes.
The larger scope of this is to replace all exposed instances of UmbracoContext across the entire codebase with IUmbracoContext (as there should be no real change at all to the outgoing behaviour it kind of makes sense).
The only annoying issue here is namespacing - Moving to IUmbracoContext means that a lot of the existing internal code that calls the statics, such as UmbracoContext.Current no longer compiles. This is an interesting side effect and indicates that the calls are using the type of the non static UmbracoContext property on the affected classes in order to be able to resolve UmbracoContext.Current rather than resolving it statically.
In order to resolve this issue, I've added several lines of...
using UWeb = Umbraco.Web, so that I can call UWeb.UmbracoContext.Current instead.
The affected classes also have an Umbraco property which blocks direct access to the Umbraco namespace in exactly the same way so long hand typing the namespace was not an option here either.
If I have time, I'm more than happy to consider taking this work on myself and contributing it if someone can point me in the right direction to do so and how widely scoped the change should really apply to (It's not worth applying it across untestable statics for example).

 

I also logged a different but related issue here - http://issues.umbraco.org/issue/U4-4572 regarding the use of extension methods on IPublishedContent - These can't be mocked and I have proposed moving these methods down to a base class instead.

Details (to save link jumping)...

Extension methods and internal constructors in parts of the codebase has made mocking and testing practically impossible in some cases.
For example, since upgrading to U7 I now find that every time .HasProperty() is called on an item of content it calls through to the ContentType object like this...
/// <summary>
/// Gets a value indicating whether the content has a property identified by its alias.
/// </summary>
/// <param name="content">The content.</param>
/// <param name="alias">The property alias.</param>
/// <returns>A value indicating whether the content has the property identified by the alias.</returns>
/// <remarks>The content may have a property, and that property may not have a value.</remarks>
public static bool HasProperty(this IPublishedContent content, string alias)
{
 return content.ContentType.GetPropertyType(alias)
!= null;
}
This is not testable architecture. Since IPublishedContent is something you could expect to need mocking on a fairly regular basis, content should inherit from a base class which implements the methods currently registered as extension methods and the methods should themselves be included in the IPublishedContent interface declaration if at all possible.
This approach would allow all methods to be accurately mocked and prevent unexpected and complex test failures.
This applies at a larger level to all of the Extensions statics that are set up on the Interface infrastructure, attaching extension methods to interfaces is bad design. I've fallen into this trap myself on a project before, which was adapted and had no TDD. I'd written tons of the things before I started to implement testing and suddenly realised that although it looked good it created a ton of test headaches.
Looking through the codebase this would be a significant change, however, it shouldn't be a breaking change as the external interfaces should not change at all in terms of how they are viewed by the external code.

I'd really like to get involved in fixing these issues if I can, when time allows, so I guess I'm looking for more guidance as to what to do next really.

[Aaron Powell]

With your point on extension methods, is that really a problem? If you’re generating a mock of IPublishedContent surely you can control what is returned by the ContentType property, so I’m failing to see the problem with that example.

 

Regarding overall unit testing against Umbraco I echo the frustration that it is simply so difficult to test components which were inherently designed for testability, especially Controllers.

 

I’ve previously asked about changing UmbracoContext.Current to expose an interface, allowing you to pass in an interface to represent it, but the problem with doing that is that UmbracoContext has a lot of internal members on it which are used by Umbraco internally (duh!). By making it an interface you no longer have the ability to create these internal members which means that Umbraco would have to either expose these currently internal members or change something about the way it’s designed.

 

I think this is actually a symptom of a larger challenge, the reliance on a Service Locator pattern. There are a lot of these ‘service bases’, UmbracoContext and ApplicationContext are examples, and these expose dependencies which are either directly consumed from the Service Locator, or in some cases they are passed into the component. This means you need to set up the Service Locator before you can consume the dependencies because sometimes you can pass in your dependencies to constructors and sometimes it’ll internally resolve them.

I’ve hit this problem a few times in Chauffeur, which is using the API in a way it was never intended, as I don’t use the CoreBootManager which does a lot of this initialisation for you.

 

I don’t really have a solution for how to fix unit testing easily as there’s a lot of touch points you’d need to hit (which you found out). Umbraco 5 showed us how over architecting a problem will make things just as bad, if not worse, and we want to avoid repeating history.

Instead this was just a collection of my random thoughts that can go along with the work Keith has undertaken.

--
You received this message because you are subscribed to the Google Groups "Umbraco development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to umbraco-dev...@googlegroups.com.
To post to this group, send email to umbra...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/umbraco-dev/de28816a-986b-47db-a54a-8b9449f8406b%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Keith Jackson]

In the IPublishedContent example, say I have a method that I want to write a unit test for and that method takes some content (any content - type itself irrelevant at that point) and that method does various value checks on the content - Looks for a specific property, checks if it has a value and then uses the result to populate a model or pass to a logical operating class of some kind (In my case I'm wrapping optional Avatar images and summary content and changing the source of data if the optional elements aren't available).

In my test I want to mock the behaviour of methods on the IPublishedContent item like 'HasValue' (I may have the exact method names wrong as I'm writing from memory so please excuse me) but many of these methods are extension methods so they can't be mocked. This means that my method can't use them at all if it needs to be testable. Moving these declarations to a base content class would allow for them to be mocked out easily.

The issues with *.Current are an even bigger headache. I think that's probably the next step down the line from implementing an IUmbracoContext to pass into the inherently testable elements of the system. One option (although nasty to some degree) solution would be a step by step change that introduces a 'CurrentInternal' property that mirrors 'Current' - transition the internal code to use 'CurrentInternal', then change the exposed type of 'Current' to 'IUmbracoContext'

--
You received this message because you are subscribed to a topic in the Google Groups "Umbraco development" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/umbraco-dev/BKgGSgeKEug/unsubscribe.
To unsubscribe from this group and all its topics, send an email to umbraco-dev...@googlegroups.com.

To post to this group, send email to umbra...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/umbraco-dev/BLU402-EAS7764D022116E65FEEBA7A3EE530%40phx.gbl.

For more options, visit https://groups.google.com/d/optout.

--
Keith Jackson

[Andy Butland]

Hi Keith

Without wanting to divert the thread too much, just on the specific point about testing extension methods on IPublishedContent, I was working on this recently looking to write some tests for the Umbraco Mapper package.  In there I wanted to be able to spin up an instance of IPublishedContent and check the mapping functions work as expected.

Like you found, you can't mock the extension methods on this, or stub them.  However I found using Microsoft Fakes you could shim them.  This was my first use of this technology so can't say too much about it, other than it solved the problem I had here - and might help with what you are trying to do.

I wrote up some notes here and you can see an example test here.

Cheers

Andy

[Lars-Erik Aabech]

Basically, “internalizing” and tight coupling has to stop (completely!) and get refactored before we can unit-test everything. J

 

The core is developed with a philosophy of making things internal until the interface has been through a proof of concept phase and the team is sure the interface(s) won’t change. Sometimes, stuff will be made public when enough people ask for it.

However, sometimes the poc phase just never ends.

 

I for one wish everything would be public, mostly virtual and have interfaces. I’d rather go fix stuff that gets broken in updates, or even leave the sites with old versions alone.

(For one, I’m upgrading a project just now due to the Newtonsoft.Json upgrade. Finally!)

 

But I’m sure this won’t get better until version 9 or 10.

I’m hoping for a session about this at codegarden.

 

Lars-Erik

To view this discussion on the web visit https://groups.google.com/d/msgid/umbraco-dev/BLU402-EAS7764D022116E65FEEBA7A3EE530%40phx.gbl.

[Keith Jackson]

Is Ms Fakes available to normal subscribers now? I thought it was for ultimate only

--
You received this message because you are subscribed to a topic in the Google Groups "Umbraco development" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/umbraco-dev/BKgGSgeKEug/unsubscribe.
To unsubscribe from this group and all its topics, send an email to umbraco-dev...@googlegroups.com.

To post to this group, send email to umbra...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/umbraco-dev/57f0b811-3b68-4ac4-9d7d-da0df294a196%40googlegroups.com.

[Andy Butland]

It's in premium (which is what I have) and up.  Must admit hadn't realised it wasn't in the professional version... so not so good for an open source package on reflection!  Glad I posted now to find out.

Andy

[Shannon Deminick]

Some of this has been discussed previously, I am all for making testability easier but no we cannot 'just' expose everything publicly, there are several reasons for this. Most of it has to do with dealing with and slowly phasing out the very very old legacy code and architecture whilst still being able to evolve the codebase at a reasonable pace and not constantly introducing breaking changes.

You can create UmbracoContext and ApplicationContext instances and also set the 'Current' which should help you test much of this, see

https://github.com/umbraco/Umbraco-CMS/blob/7.1.2/src/Umbraco.Tests/MockTests.cs

In the above tests, you can create an UmbracoContext, you can mock everything that is part of the ApplicationContext or you can just create a blank one. I know this isn't the perfect ideal solution like interfacing these contexts but we cannot do that just yet. Once the remaining legacy bits of code are phased out and once the 'new cache' is a reality, then this can be possible - but will also be a breaking change so would have to wait for a major version release. So the alternative in the meantime is to keep evolving UmbracoContext to be able to instantiate a new one with mockable elements.

As far as extension methods, I'm sure there are some better examples of mockable issues than 'HasValue' - you can just mock the GetProperty method to return what you want since the HasValue method just calls this method. But yes there are other extension methods that have reliance on contexts, etc... You'd have to jump through some hoops to wire everything up to have them working (which is possible, but I realize not perfectly ideal). Unfortunately some of those methods just evolved that way but I agree that the important ones that have reliance on other contexts should be part of a base class or similar.

The MS fakes stuff looks awesome as a temporary work around, nice find! 

[Keith Jackson]

Thanks for jumping in on this Shannon.

REGARDING CONTEXTS

I see where you're coming from here and this sounds like quite an involved long term strategy type solution, although the code in MockTests.cs does give me some ideas. I may run up a test support project for Umbraco on github and push it to nuget that offers things like a ContextBuilder to generate context instances you can pass around - Basic to start with and then people can add flexibility as they need it case by case. It's not an ideal solution but it should help. Certainly the big issue this presents for me at the moment is that I can't unit test my controllers. If I can generate a fake UmbracoContext that works then that solves my problems for now. The goal of the package would be to maximise test code readability but to fill the holes left by untestable parts of the codebase.

Would there be much demand for such a package do you think? (I'll probably get around to it at some point anyway for my own use)

REGARDING EXTENSION METHODS

Agreed, 'HasValue' is a really bad example, it just so happened to be the place where I noticed the problem on the site I was upgrading. I believe in the end I worked around the problem by using GetProperty instead and adding the relevant null checks. My point is that I spent some fair time looking through the Umbraco source to try and figure out what I could do while, as a user of the product, it would have been much easier to just mock the HasValue return.

There are several variations on IPublishedContent, looking through the codebase in more detail that make this a bit more involved but, if necessary the methods could be duplicated or call through to a private instance of what is currently PublishedContentExtensions (in fact DynamicPublishedContent already does this or provides it's own implementations looking at the code) but rename the class to something more relevant to it's new purpose (I'd personally go for reducing the duplication and having the current extensions in an internal class within the various implementations of the interface). All the methods would then be declared on the interface and would be completely mockable. Obviously a base class would be ideal but, looking at some of the method dependencies, I'm not sure that approach would get very far.

This is definitely not a change to be taken lightly though, the extensions class has a massive amount of methods attached to it.

--
Keith Jackson

[Shannon Deminick]

Certainly an interim project for aiding in umbraco testability would be welcome! That said, there's been plans to re-organize, re-factor the test projects in the umbraco core itself and to release this type of assembly but of course hasn't been done yet. The initial plan is to separate the test project into: Unit Tests, Integration Tests and a 3rd Test project that has all the underlying 'helper' functionality for testing which is what we'd release. Not sure when we'll get the time to do all of that ... If this is something that interests you then we of course appreciate any and all core help! :) 

The plan is that the test helper assembly isn't publicly supported, meaning that we wouldn't consider breaking changes in that assembly as 'breaking', using the assembly would really be at developers own 'risk' and of course should only ever be used for testing.

The refactoring stuff for IPublishedContent is a different sort of beast and best discussed with Stephen since there are a few things in motion regarding the 'new cache' which all of this may directly affect. Definitely agree that we could approach many of the extension methods, if not all of them in a different way as you suggest so they are easily testable/mockable/etc... but breaking changes is a huge concern for us so something to keep in mind.

Cheers!