Joshua Flanagan

Real World Refactoring

Joshua Flanagan — Mon, 29 Jun 2009 00:35:29 GMT

If you’ve ever asked, or been asked, for an example to illustrate a software design principle, you know how frustrating it can be to work with a contrived example. There is rarely any depth to the example, and it lines up so neatly with the concept being explained that the student may have trouble recognizing similar situations in the wild. I’m going to try and relate a real world example of a recent refactoring effort, with the hope that the extra context and narrated walkthrough will help someone make a connection that has failed before.

A little background

My discussion revolves around Docu, which is an open source project for converting .NET XML comments into HTML documentation. (Note: I want to make clear is that this is not a criticism of any of the Docu code. All code is written with specific goals and constraints in mind. As the goals evolve, some designs that worked perfectly in early iterations may start to create friction, and deserve to be reconsidered.) .NET XML comments are constructed by applying a set of top-level tags to code elements such as classes, methods, or properties. Top-level tags are things like <summary/>, <remarks/>, or <param/>. Within the contents of the top-level tags, you can use embedded tags to provide additional contextual information. Embedded tags are things like <see/>, <para/>, or <paramref/>. The following example shows a top level <remarks/> tag, with embedded <paramref/> and <para/> tags:

///<remarks>Make sure the <paramref name=”maxValue” /> is a positive number.
///<para>Do not call more than once.</para></remarks>

In Docu, the job of translating the XML contents of a single top-level tag into a semantic model is handled by the CommentParser. Let’s take a look at the original implementation:

public class CommentParser : ICommentParser
{
    private readonly IDictionary<Func<XmlNode, bool>, Func<XmlNode, bool, bool, IComment>> parsers =
        new Dictionary<Func<XmlNode, bool>, Func<XmlNode, bool, bool, IComment>>();

    private readonly InlineTextCommentParser InlineText = new InlineTextCommentParser();
    private readonly InlineCodeCommentParser InlineCode = new InlineCodeCommentParser();
    private readonly MultilineCodeCommentParser MultilineCode = new MultilineCodeCommentParser();
    private readonly SeeCodeCommentParser See = new SeeCodeCommentParser();
    private readonly ParagraphCommentParser Paragraph;
    private readonly ParameterReferenceParser ParameterReference = new ParameterReferenceParser();
    private readonly InlineListCommentParser InlineList;

    public CommentParser()
    {
        Paragraph = new ParagraphCommentParser(this);
        InlineList = new InlineListCommentParser(this);
        parsers.Add(node => node is XmlText, InlineText.Parse);
        parsers.Add(node => node.Name == "c", InlineCode.Parse);
        parsers.Add(node => node.Name == "code", MultilineCode.Parse);
        parsers.Add(node => node.Name == "see", See.Parse);
        parsers.Add(node => node.Name == "para", Paragraph.Parse);
        parsers.Add(node => node.Name == "paramref", ParameterReference.Parse);
        parsers.Add(node => node.Name == "list", InlineList.Parse);
    }

    public IList<IComment> Parse(XmlNodeList nodes)
    {
        var blocks = new List<IComment>();

        int count = nodes.Count;
        for(int i = 0; i < count; i++)
        {
            XmlNode node = nodes;
            bool first = (i == 0);
            bool last = (i == (count - 1));

            foreach(var pair in parsers)
            {
                var isValid = pair.Key;
                var parser = pair.Value;

                if(!isValid(node))
                    continue;

                var block = parser(node, first, last);

                if(block != null)
                {
                    blocks.Add(block);
                    continue;
                }
            }
        }

        return blocks;
    }

You can see (lines 18-24 above) that in its constructor it builds up a collection of function (Func<>) pairs: one function that is used to identify a specific embedded tag, paired with another function that knows how to parse that tag into its model representation (an instance of IComment). The collection is used by the primary method in CommentParser, which iterates over all of the child nodes of a given chunk of XML, finds a parsing function that can handle that node, invokes the function, and collects the results.

The functions that perform the parsing of individual embedded tags are implemented in separate classes (I’ll call them “node parsers”). The CommentParser creates an instance of each node parser and stores it in a field where it can be referenced by the collection of function pairs. The benefit of moving the embedded tag parsing into separate classes is that they can be developed and tested independent of CommentParser. Unfortunately, you need to make a number of changes to CommentParser every time you add a new node parser. Since there are still quite a few embedded tags that are not yet recognized by Docu, and each embedded tag (generally) requires a new node parser, the CommentParser class will be continuously modified and unstable. The primary goal of my refactoring effort is to create a more stable CommentParser that is open for extension (support for new embedded tags can be added) but closed for modification.

High Cohesion

As stated above, the logic for determining when a specific parsing function should be applied was inside of CommentParser, while the logic to implement that function was in the individual parsing classes. These two pieces of logic are tightly related to each other. You cannot safely apply an arbitrary node parser to any node – it only makes sense to apply a parser to the type of node it was designed for. We can make the node parsers and the CommentParser more cohesive by moving both pieces of logic into the node parsing classes. I’ll add a CanParse(XmlNode) method to each node parser. The method returns true if the parser knows how to parse a given comment node. I implement them by copying the logic for identifying specific tags from the Func<> pairs in CommentParser.

Low Coupling

We still have the problem that the CommentParser creates and stores an instance of each specific node parser class (lines 6-11 above). This high coupling between the CommentParser and the node parsing classes makes it impossible to execute in isolation. You cannot use a CommentParser without bringing all of the functionality of all node parsers along. You also have to modify the CommentParser every time a new type of node parser is added (as when adding support for a new type of comment tag).

I’ll reduce coupling between CommentParser and individual node parsers by introducing an interface (ICommentNodeParser) to describe the functionality exposed by the node parsers. The CommentParser will only interact with this interface, which exposes the CanParse and Parse methods. I then change the node parsers so that they implement the new interface. Since they already had all of the needed functionality, it was just a matter of making sure the method signatures matched correctly.

Slight detour

When I started to add the interface to all of the node parsing classes, I noticed they all derived from CommentParserBase. However, none of the code in the application referred to these classes through this base class. That’s a pretty good indicator that inheritance is being used to share common functionality rather than for polymorphism. Using inheritance just to share some common functionality can lead to more inflexible designs and less cohesive classes. You are usually better off using composition instead of inheritance in these scenarios. In this case, the only shared functionality was a single helper method that had some special logic for string trimming. Since the method didn’t make use of any instance data, it was easy to move it to a separate class as an extension method on string. Now that CommentParserBase was empty, there was no reason to keep it around, so it was eliminated. The fact that no code broke when the base class was deleted (without using a refactoring tool) is a good indicator that it was the right decision.

Composition through dependency injection

At this point we have a bunch of node parsers that all implement a common interface. But the CommentParser is still tightly coupled to the various implementations because it has to create the instances. This is the perfect opportunity to use dependency injection to pass the node parser instances into the CommentParser. I just change the constructor of CommentParser to require an array of ICommentNodeParsers, and delete all the code that was creating the node parser instances. Whoever creates an instance of CommentParser will need to pass in the collection of node parser instances. CommentParser is no longer coupled to the specific node parsers and can be more easily used in isolation. You can see that the newer version is greatly simplified:

public class CommentParser : ICommentParser
   {
       private readonly ICommentNodeParser[] _parsers;

       public CommentParser(ICommentNodeParser[] parsers)
       {
           _parsers = parsers;
       }

       public IList<IComment> Parse(XmlNodeList nodes)
       {
           var blocks = new List<IComment>();

           var count = nodes.Count;
           for(var i = 0; i < count; i++)
           {
               var node = nodes;
               var first = (i == 0);
               var last = (i == (count - 1));

               var parser = _parsers.FirstOrDefault(p => p.CanParse(node));
               if (parser == null) continue;

               var block = parser.Parse(this, node, first, last);
               if (block != null)
               {
                   blocks.Add(block);
               }
           }

           return blocks;
       }

At this point you may be thinking I just “passed the buck” for creating the individual node parser instances to another class up the stack. Someone still needs to create them, and will therefore be tightly coupled to the implementations. That is true. However, if we can pass that responsibility up the stack far enough, it can be handled by code that has no other responsibility than to configure and bootstrap our application. That type of code doesn’t typically have any logic that would be re-used and doesn’t have the same concerns about designing for maintainability. Luckily, in the case of Docu, we already use StructureMap to compose our object instances so we get this functionality for free. I simply had to add a single line to the StructureMap configuration to tell it to make use of any implementation of ICommentNodeParser it finds in the Docu assembly.

public DefaultRegistry() {
  Scan(x =>  {
    x.AssemblyContainingType<DocumentationGenerator>();
    x.WithDefaultConventions();
    x.AddAllTypesOf<ICommentNodeParser>();

Now when an instance of CommentParser is requested from StructureMap, it will automatically have the array of all node parsers injected to its constructor.

Wrap up

So what did we gain? Let’s compare the stories for adding the ability to parse a new XML documentation tag. In the original implementation, we had to:

Create a class with code that parses the new tag
Create a new field in CommentParser to hold an instance of the new parsing class
Modify CommentParser’s constructor to register the new class, along with a predicate that determines when it should be used

And now we simply:

Create a class that implements ICommentNodeParser with code that recognizes and parses the new tag

Notice that you didn’t have to touch CommentParser? We just gave it new functionality without having to change (and potentially destabilize) the code. That’s the open-closed principle in action. The increased cohesion of the node parsers makes them easier to understand and helps localize any future changes to their implementation. We also saw that the use of dependency injection and a composition tool (StructureMap in this case) made it painless for us to pull code apart into separate classes. The code was made easier to maintain by applying a few established object oriented design principles. Hopefully this example helped clarify the application of these principles. Your feedback is appreciated.

For additional context, you can view the commit that contained the changes discussed above.

Integrating a custom test runner with TeamCity

Joshua Flanagan — Thu, 11 Jun 2009 00:45:15 GMT

JetBrains’ TeamCity build server provides a wealth of information about the tests that are run as part of your build. If you use one of the test runners that they support out of the box, you automatically get real-time individual test results, detailed timing reports, and historical performance metrics.

Up until recently, all of the tests for my team were run through NUnit – C# unit tests, QUnit JavaScript tests, and integration tests. Since NUnit is natively supported by TeamCity, we got used to the rich test reporting. However, we recently started to integrate our automated acceptance tests into the build. Our acceptance tests are written using StoryTeller, which are executed using its own test runner. The StoryTeller test runner has slightly different behavior than the NUnit runner. For one, a failed assertion does NOT stop execution of a test – it continues executing so that a single test can have multiple failed assertions. We also have the ability to mark individual tests with their state in the development lifecycle: they start as "acceptance” tests when they are first defined, and then are marked as “regression” once all supporting code has been correctly implemented. The idea is that you can drive your development with failing acceptance tests until they pass, at which point they become regression tests. This is relevant because a failed acceptance test does NOT fail the build – only a failed regression test will fail the build.

The StoryTeller test runner is a console application that will return an error code if any regression tests fail. It also generates an HTML report which contains the details for each test. Our initial attempt at integrating it into our build allowed us to fail the build if any test failed, but we didn’t get much feedback about why it failed. We would have to go searching through the build log messages or HTML report files for details.

I created a new StoryTeller ITestListener (TeamCityTestListener) which is used by the StoryTeller runner to log test results. It writes the test details to the console using TeamCity’s Service Message API (which I mentioned previously). TeamCity picks up these messages, and creates the nice individual test reports, complete with timing and history:

Now we know exactly which tests failed, but we don’t know which assertions in the tests failed. As I mentioned, the complete details of the test execution are recorded in an HTML report. It’d be nice if we could view individual test reports right within TeamCity. Turns out, that is pretty easy to do as well. I followed the documentation for Including Third-Party Reports in the Build Results, and added the following line to our <TEAMCITY>\.ServerConfig\.BuildServer\config\main-config.xml file:

<report-tab title="StoryTeller" basePath="st-results" startPage="st-results.htm" />

We now have a nice new StoryTeller tab next to the other build result tabs:

We configured the build to store all of the HTML reports as artifacts, and point our custom tab to the test results index HTML file generated by StoryTeller. Clicking the StoryTeller tab displays that index file with a color coded overview of all of the tests. Clicking a test brings up the report for that test so that I can see exactly which steps failed.

Let your Inversion of Control tool work for you

Joshua Flanagan — Wed, 10 Jun 2009 03:58:51 GMT

If you are just starting out exploring use of an Inversion of Control tool (IoC), its very easy to go down the wrong path, and make things harder for yourself. Today I had a conversation with a bright developer who was feeling the pain because doing it all wrong. And just a month or so ago, I had the same conversation with another bright developer who was also doing it the same, wrong, way.

The problem may be that people misinterpret the intent of an IoC tool. It is seen simply as a configuration tool – a way to swap out classes using some magical XML or a DSL. The thought is that as long as your code gets all of its dependencies out of the container, you can change the behavior just by configuring the container.

And then you end up with code that looks like this:

public class OrderService : IOrderService
{
    private IRepository _repository;
    private ISystemClock _systemClock;

    public OrderService()
    {
        _repository = Container.Resolve<IRepository>();
        _systemClock = Container.Resolve<ISystemClock>();
    }
}

public class OrderController
{
    private IOrderService _orderService;

    public OrderController()
    {
        _orderService = Container.Resolve<IOrderService>();
    }
}

The OrderService needs an IRepository and an ISystemClock, so you pull them out of the container in the constructor. The OrderController needs an IOrderService, so you pull it out of the container in the constructor. You can now swap out actual implementations using some external configuration.

Except now you’ve got this annoying dependency on the magical Container class. And maybe the Container has a dependency on an external XML file.

And you discover that when you try to unit test your OrderService, you need to make sure you have a Container available, and that it has been configured to serve up whichever instances you want to use during testing.

Your simple, plain-old CLR object (POCO) now has this deadweight Container dependency dragging it down. The Container requires extra care and feeding, in the form of configuration, and now it has infected your entire codebase.

Hopefully, at this point you are feeling enough pain that you go seeking some advice, and hopefully you find out about auto-wiring.

Any IoC tool worth using will support auto-wiring dependencies for objects retrieved from the container. This means that the tool will build up all of the necessary objects needed to satisfy a request. This allows you to rewrite the above code as:

public class OrderService : IOrderService
{
    private readonly IRepository _repository;
    private readonly ISystemClock _systemClock;

    public OrderService(IRepository repository, ISystemClock systemClock)
    {
        _repository = repository;
        _systemClock = systemClock;
    }
}

public class OrderController
{
    private readonly IOrderService _orderService;

    public OrderController(IOrderService orderService)
    {
        _orderService = orderService;
    }
}

But where did the Container go? Exactly. The majority of your code should never have to deal with the container directly. Ideally, you make a single call to the container which builds up an object graph that implements your program. For a console application, you might make a call to the container once in your static Main method. For an MVC web application, you might make a call to the container at the beginning of each request to retrieve the appropriate Controller. The rest of your code is blissfully ignorant of the container. When something asks the container for an OrderController, the container will detect the dependency on the IOrderService. So it goes to build an OrderService, and detects the dependencies on the IRepository and ISystemClock. So it builds up those instances, and any dependencies they may have, and so on.

By the way, my colleague Jeremy Miller wrote this same post just a few months ago. But he assured me it was worth re-writing, because there are new people jumping on all the time, and they’re not likely to go back reading old blog posts. If you find yourself with the problem I describe above, go get the full details by reading his post about auto-wiring in StructureMap.

One thing that I’ll add is that I’ve heard a lot of talk lately that “container” is really a misnomer for what an IoC tool does, and that it should be more appropriately referred to as a “composer”. I think this argument has merit, and wonder if the name may be contributing to people going down the wrong path described above. A “container” sounds like something that stores a bunch of stuff for you, and it is your job to get stuff out of it, hence the explicit calls to the Container littered throughout your code. A “composer” sounds like something that takes little pieces of functionality in your application and puts them together into a usable whole. The bad code example above reflects “container” thinking. The better code example reflects “composer” thinking.

Latest Docu builds available

Joshua Flanagan — Wed, 20 May 2009 02:29:44 GMT

Docu is a tool to create documentation out of your .NET XML comments, much in the same way we used to use NDoc. If legend has it right, it was created by the prolific James Gregory with the sole goal of generating the documentation for Fluent NHibernate. It won’t ever compete with Sandcastle on features or functionality, nor should it. If you just need to create developer-focused, HTML documentation for your API, docu is the perfect tool. If you need to create higher level end-user documentation in different formats, you’ll probably always be better off with something like Sandcastle.

Because of its original narrow focus, it doesn’t currently support all language elements completely, or all of the possible documentation tags. Fortunately, it has a solid codebase to build on, and there has been a lot of interest in it, as a simpler alternative to Sandcastle for simple tasks. Since it is under active development, and each change provides better and better support and stability, a single release can get outdated quickly. I thought it was important to make it easy for potential users to get the latest and greatest bits without having to get the code and compile it themselves.

I’m happy to announce that you can now do just that by visiting the awesome teamcity.codebetter.com. Login as guest (or create an account for yourself, if you’d like to monitor the build status of your favorite open source projects) and click on Docu in the list of projects. On the Docu page you will see a list of all recent builds, with the build number in the leftmost column. Click on the “View” link (or the down-arrow next to it) within the Artifacts column for the build you are interested in to see the list of files created by that build. Just grab the zip file and extract it to a folder and and you are ready to go.

UPDATE: As of build #35, you can download a zip file for each build which contains everything needed to run docu. You no longer need to download an earlier release from the project site first.

Of course, you will be running from trunk, and so all the usual risks and disclaimers apply, but I think the project is still at an early enough stage that it actually gets more stable with each commit. That said, if you do run into problems, you can report issues to the docu issue tracker, ask a question on the mailing list, or just contact me through this blog and I’ll do my best to help you out.

ASP.NET 400 Bad Request with restricted characters

Joshua Flanagan — Tue, 28 Apr 2009 03:36:00 GMT

Today I had to hunt down a reported defect that said our advanced search functionality was returning a Bad Request error. On initial inspection, I was unable to reproduce the issue. After talking to our product manager, I learned that he was trying to seed the search with the text “% %”. We have a quick search text box that lets you enter your criteria, and it has some built in rules about which fields it applies the criteria to. If you need more control over the criteria, you can enable the advanced search, and your quick search criteria will automatically be populated in the advanced search page. The way we were doing that was by passing the criteria in the URL, as in: http://localhost/dovetailcrm/contacts/query/yourbasiccriteria.

To seed the advanced search for “% %’, it would load http://localhost/dovetailcrm/contacts/query/%25%20%25 (% URI encodes as %25, space encodes as %20)

We were properly encoding the request, so what the problem? Attempting to load that URL would cause the error:

400 Bad Request
ASP.NET detected invalid characters in the URL.

Proceed at your own risk

My first instinct was to suspect URLScan, or the IIS 7.0 equivalent. After a bit of googling, it became apparent that ASP.NET really didn’t like it when you tried to pass a %, &, *, or : in the URL. The various fixes were scattered around different forum posts, but summed up nicely at Dirk.Net. Unfortunately, the only answer seemed to be “make a registry change” or “don’t pass those characters in your URL”.

That didn’t sit well with me – it didn’t seem right that there was no way to pass those characters in the URL without having to change your server configuration which could potentially expose your site to security risks.

Just because you can, doesn’t mean you should

After a little bit of experimentation, I discovered that you certainly CAN pass those characters in a URL: they just have to be passed in the query string (the part after the question mark). It suddenly started to make sense why there was not a whole lot of information on this error, and why the little information that was available seemed to be related to ASP.NET MVC. Up until ASP.NET MVC (or more accurately, System.Web.Routing), you would almost always send parameterized data in the URL as part of the query string. It wasn’t until we got Routing that we started putting parameters in the path portion of the URL.

So after making a short story long, the solution was to simply pass the information the old fashioned way, in the query string: http://localhost/dovetailcrm/contacts/query?search=%25%20%25

But don’t worry, WebForms is not going away…

Joshua Flanagan — Sun, 08 Mar 2009 22:23:40 GMT

I got a kick out of Jeremy’s eye poke at the ASP.NET WebForms “simpler” approach to web development.

In the same spirit, remember how convoluted it was to call simple JSON services from WebForms? Well, my friend and former colleague Shashank Shetty shows how it is done with ASP.NET MVC and jQuery in his inaugural blog post.

Auto mocking Explained

Joshua Flanagan — Wed, 04 Feb 2009 01:25:32 GMT

What does it do?

There is nothing fancy about an automocker. There is nothing scary about an automocker. An automocker is like one of those little classes we all write in order to quickly and easily create objects that we need for our tests. Nice, simple, helpful, non-threatening.

You tell the automocker which class you want to test, and the automocker creates an instance of that class for you. If the constructor of your class requires you to pass in other objects, the automocker will automatically create a mock for each of the necessary types and pass them in. The automocker will keep track of all the mocks it creates, and allows you to retrieve them if you need to stub a specific behavior.

Why should I fear the automocker?

Where it gets scary (for some) is when you use the name most often used to refer to them: auto mocking container. “Wait wait wait, you’re using an IOC container in your unit test?! Configuration! Brittle! No thank you!”

The name comes from AutoMockingContainer, which introduced the idea of an automocker (to me, at least) by using the Castle Windsor IOC tool to do the heavy lifting. The StructureMap IOC tool distribution also includes an automocker. But here’s the thing – the container is just an implementation detail. You, as the test writer, never have to know there is a container in play. Automocking is not an IOC container feature. Automocking is really a feature of a mocking framework, but just happens to be packaged and/or named in relation to a container.

In other words, I suspect the idea of automocking would be a lot less scary to people if it were packaged with the mocking frameworks. Suppose Rhino.Mocks added the method: MockRepository.CreateWithMocks<ClassToTest>() – would automocking still be as distasteful? Maybe it would use Castle MicroKernel under the hood, but you wouldn’t ever be exposed to it (just as Rhino.Mocks currently uses Castle DynamicProxy under the hood).

Why should I use the automocker?

Ok, so you’re over the container part. But you’re not convinced. The automocker can save you some tedious keystrokes, but code snippets solve that problem. I think the big benefit of the automocker is that it helps keep the content of your tests focused on the functionality being tested. It also helps keep your tests less brittle as your objects evolve.

Consider the following class that I need to test:

        1: public class PhotoController

       2: {

       3:     private readonly IPhotoCatalog photoCatalog;

       4:  

       5:     public PhotoController(IPhotoCatalog photoCatalog)

       6:     {

       7:         this.photoCatalog = photoCatalog;

       8:     }

       9:  

      10:     public PhotoDetails[] List()

      11:     {

      12:         return photoCatalog.FindAll();

      13:     }

      14:  

      15:     public void Save(PhotoDetails photo)

      16:     {

      17:         photoCatalog.AddPhoto(photo);

      18:     }

      19: }

A couple tests, without automocking, might look like this:

       1: [TestFixture]

       2: public class when_visiting_the_list_page

       3: {

       4:     private PhotoDetails[] Output;

       5:     private PhotoDetails[] AllPhotos;

       6:  

       7:     [SetUp]

       8:     public void Setup()

       9:     {

        AllPhotos = new[] { new PhotoDetails(1), new PhotoDetails(2) };

        IPhotoCatalog photoCatalog = MockRepository.GenerateStub<IPhotoCatalog>();

        photoCatalog.Stub(c => c.FindAll()).Return(AllPhotos);

  

        var controller = new PhotoController(photoCatalog);

        Output = controller.List();

    }

  

    [Test]

    public void should_display_all_of_the_photos_in_the_catalog()

    {

        Assert.AreEqual(AllPhotos, Output);

    }

}

  

[TestFixture]

public class when_saving_a_photo

{

    private PhotoDetails thePhoto;

    private IPhotoCatalog photoCatalog;

  

    [SetUp]

    public void Setup()

    {

        photoCatalog = MockRepository.GenerateStub<IPhotoCatalog>();

        var controller = new PhotoController(photoCatalog);

  

        thePhoto = new PhotoDetails(4);

        controller.Save(thePhoto);

    }

  

    [Test]

    public void should_store_the_photo_details_in_the_catalog()

    {

        photoCatalog.AssertWasCalled(c => c.AddPhoto(thePhoto));

    }

}

The same tests, with automocking, would look like this:

       1: [TestFixture]

       2: public class when_visiting_the_list_page

       3: {

       4:     private PhotoDetails[] Output;

       5:     private PhotoDetails[] AllPhotos;

       6:  

       7:     [SetUp]

       8:     public void Setup()

       9:     {

        AllPhotos = new[] { new PhotoDetails(1), new PhotoDetails(2) };

        var mocks = new RhinoAutoMocker<PhotoController>();

        var controller = mocks.ClassUnderTest;

        mocks.Get<IPhotoCatalog>().Stub(c => c.FindAll()).Return(AllPhotos);

  

        Output = controller.List();

    }

  

    [Test]

    public void should_display_all_of_the_photos_in_the_catalog()

    {

        Assert.AreEqual(AllPhotos, Output);

    }

}

  

[TestFixture]

public class when_saving_a_photo

{

    private PhotoDetails thePhoto;

    private RhinoAutoMocker<PhotoController> mocks;

  

    [SetUp]

    public void Setup()

    {

        mocks = new RhinoAutoMocker<PhotoController>();

        var controller = mocks.ClassUnderTest;

  

        thePhoto = new PhotoDetails(4);

        controller.Save(thePhoto);

    }

  

    [Test]

    public void should_store_the_photo_details_in_the_catalog()

    {

        mocks.Get<IPhotoCatalog>().AssertWasCalled(c => c.AddPhoto(thePhoto));

    }

}

Not a huge difference, mostly because the class under test only has a single dependency, so you are only hiding a single call to MockRepository.GenerateMock. But what about when I need to change some functionality on one of the methods? We realize we need to save a copy of the uploaded photo to the file system. We will need to change the Save method on the controller, and add a new dependency on an IFileSystem service.

We’ll update the automocked tests to include the new specification (no other changes to the tests required):

       1: [Test]

       2: public void should_store_the_photo_in_the_file_system()

       3: {

       4:     mocks.Get<IFileSystem>().AssertWasCalled(fs => fs.WriteFile(thePhoto.Filename, thePhoto.Data));

       5: }

And the corresponding code to:

       1: public class PhotoController

       2: {

       3:     private readonly IPhotoCatalog photoCatalog;

       4:     private readonly IFileSystem fileSystem;

       5:  

       6:     public PhotoController(IPhotoCatalog photoCatalog, IFileSystem fileSystem)

       7:     {

       8:         this.photoCatalog = photoCatalog;

       9:         this.fileSystem = fileSystem;

      10:     }

      11:  

      12:     public PhotoDetails[] List()

      13:     {

      14:         return photoCatalog.FindAll();

      15:     }

      16:  

      17:     public void Save(PhotoDetails photo)

      18:     {

      19:         photoCatalog.AddPhoto(photo);

      20:         fileSystem.WriteFile(photo.Filename, photo.Data);

      21:     }

      22: }

Now I can run all of my tests (for both Save and List) – they all compile and pass. Notice the only change to my tests was adding a single method that asserted the new behavior.

The tests that didn’t use the automocker? Not as lucky. I add the new method that has the new assertion,

       1: [Test]

       2: public void should_store_the_photo_in_the_file_system()

       3: {

       4:     fileSystem.AssertWasCalled(fs => fs.WriteFile(thePhoto.Filename, thePhoto.Data));

       5: }

and then I have to update the Setup to create the new mock. Not a big deal.

       1: fileSystem = MockRepository.GenerateStub<IFileSystem>();

       2: var controller = new PhotoController(photoCatalog, fileSystem);

The IDE complains because I’m trying to pass two arguments to the controller constructor, but it only takes one, so I update the controller. Still not a big deal. Try and run my test – ugh, compiler error. Looks like I need to update the tests for the List method? Why? I’m not making any changes to the List method? So I update the Setup for that test fixture as well:

       1: var fileSystem = MockRepository.GenerateStub<IFileSystem>();

       2: var controller = new PhotoController(photoCatalog, fileSystem);

Is it horrible? No. Is the automocked version nicer? I think so. The evolution of my code had minimal impact on the tests, plus I was able to eliminate “noise” code that creates all of the mocks. What does the automocked version cost me? Just an Add Reference to an assembly that I already have in the tools/lib section of my source tree.

Neither my code nor my tests have any dependency on an IOC container. There is no external dependency or configuration. Even if I don’t use StructureMap in my application code, I could still use the RhinoAutoMocker that it includes for my tests. If my code DOES use StructureMap as its IOC container, I could still use the original Windsor-based AutoMockingContainer for my tests without a problem. The key point being that the IOC container you use in your application has nothing to do with the automocking you do in your tests.

Solution for monitoring multiple TeamCity servers

Joshua Flanagan — Thu, 15 Jan 2009 15:23:51 GMT

Proving yet again that the squeaky wheel gets the grease (this issue rose up to have the second highest number of votes in the TeamCity issue tracker), and what a truly responsive vendor JetBrains is, we now have a solution for the problem of monitoring more than one TeamCity server.

Its a quick workaround, and requires you to run multiples instances of the notifier, but it satisfies the immediate need for me. Read the full details in Yegor Yarko’s comments in the issue tracker.

A pleasant surprise is that the updated notifier still works against a TeamCity 3.x server, for those of us that haven’t had the chance to upgrade to 4.x.

Raising awareness for TeamCity tray notifier issue

Joshua Flanagan — Wed, 07 Jan 2009 01:54:30 GMT

UPDATE: JetBrains has released a solution to this issue, I posted a follow up.

Rumor has it that a number of your favorite .NET open source applications (including StructureMap) will soon have a publicly available continuous integration server. The server will be running TeamCity by JetBrains. I’m a TeamCity fan, and we’ve been using it at work for a while now. Which is the rub. You see, since TeamCity allows you to easily host many projects on a single server (while delegating the building out to separate build agents), JetBrains did not foresee why anyone would ever want to monitor more than one server. This will soon be a very real scenario for followers of many OSS projects. The current tray notifier tool can only connect to a single build server. There aren’t even any command-line parameters that would allow you to run multiple instances of the notifier simultaneously.

An issue has already been logged with JetBrains, but it doesn’t seem to have much attention ~~(or priority)~~ (I misread their priority icons, it was marked as Major) yet. This post is an attempt to try and remedy that. If you currently use TeamCity, and want to follow the progress of projects like StructureMap, please go vote for Allow to watch several TeamCity servers via Tray Notifier.

Arrange Act Assert with StructureMap RhinoAutoMocker

Joshua Flanagan — Fri, 26 Sep 2008 02:03:18 GMT

Fresh on the heels of finally figuring out how to make the AAA syntax in Rhino.Mocks work, I've added support to StructureMap's RhinoAutoMocker. If you pass MockMode.AAA to the constructor of your RhinoAutoMocker, all of the mocks that it creates will be in replay mode. This is required to successfully use the AssertWasCalled extension method. By default, if you do not pass a MockMode to the constructor, mocks will be created in record mode so that you can continue to set expectations the old fashioned way.

[Test]
public void TheAutoMockerOptionallyPushesInMocksInReplayModeToAllowForAAAsyntax()
{
    var autoMocker = new RhinoAutoMocker<ConcreteClass>(MockMode.AAA);

    autoMocker.ClassUnderTest.CallService();

    autoMocker.Get<IMockedService>().AssertWasCalled(s => s.Go());
}

The code is in the trunk, and will be included in the eventual StructureMap 2.5 release.

Updated Oct 8, 2008: The MockMode enumeration to enable AAA syntax has been changed to MockMode.AAA to be more self-explanatory.

The Rhino.Mocks' AssertWasCalled method does work

Joshua Flanagan — Thu, 25 Sep 2008 14:08:28 GMT

This behavior is probably clearly specified somewhere, but somehow it has been non-obvious to the four people on our team to the point that we were leaning toward banning its use. I didn’t want to go that far, so figured I better figure out the rules.

I’m referring to the AssertWasCalled extension method in Rhino.Mocks that allows the Arrange/Act/Assert style of testing. We found that sometimes it would behave how we expected, and sometimes it would not, with no apparent explanation. I finally realized that it probably had something to do with how the mock objects were created, so I wrote this simple test:

        1: [TestFixture]

       2: public class AssertCalledTester

       3: {

       4:     private Dictionary<string, IDependency> dependencies;

       5:     private MockRepository mocks;

       6:  

       7:     [SetUp]

       8:     public void Setup()

       9:     {

        mocks = new MockRepository();

        dependencies = new Dictionary<string, IDependency>()

       {

           {"GenerateMock", MockRepository.GenerateMock<IDependency>()},

           {"GenerateStub", MockRepository.GenerateStub<IDependency>()},

           {"StrictMock", this.mocks.StrictMock<IDependency>()},

           {"DynamicMock", this.mocks.DynamicMock<IDependency>()},

           {"Stub", this.mocks.Stub<IDependency>()},

       };

    }

  

    [RowTest]

    [Row("GenerateMock")]

    [Row("GenerateStub")]

    [Row("StrictMock")]

    [Row("DynamicMock")]

    [Row("Stub")]

    public void When_does_it_work(string mockStyle)

    {

        var dependency = dependencies[mockStyle];

        var caller = new Caller(dependency);

        caller.Go();

        dependency.AssertWasCalled(d => d.DoSomething());

    }

}

  

public class Caller

{

    private readonly IDependency dependency;

  

    public Caller(IDependency dependency)

    {

        this.dependency = dependency;

    }

  

    public void Go()

    {

        dependency.DoSomething();

    }

}

  

public interface IDependency

{

    void DoSomething();

}

The results (3 out of 5 tests fail) clearly illustrated the source of our problems: AssertWasCalled only works automatically if you create your mocks using the new static MockRepository methods. Ayende’s initial announcement of the feature used these static methods, but it wasn’t explicit that they were required. They just appeared to be convenience methods. However, reading into the comments, I see that Ayende posted on 6/14/08 that the static methods are indeed special:

The static methods are returning an object that can be used without record/replay. It is already in replay mode, which is needed to use the AAA syntax.

If aren’t using the static methods (or are using something like StructureMap’s RhinoAutoMocker where the mocks are created for you), you can still utilize AssertWasCalled as long as you call ReplayAll() before the “act” phase of your test. For example, in the code above I can add “mocks.ReplayAll()” before line 31 to get 4 out of the 5 tests to pass. The only one that fails is the one that uses a StrictMock, which isn’t too worrisome.

Running jQuery QUnit tests under Continuous Integration

Joshua Flanagan — Thu, 18 Sep 2008 22:23:54 GMT

Setup

This post assumes you are already writing unit tests for your JavaScript code. If not, check out Chad's post on Getting Started with jQuery QUnit. We use jQuery and QUnit at work, so my code examples are geared toward those frameworks. However, the approach should be very easy to adapt to your JavaScript framework of choice.

Overview

A good continuous integration server will let you run your automated tests, fail the build if a test fails, and publish a report of the test results. For all of that to work, your CI tool needs to be able to run the tests from a command-line and harvest the output in a form that it understands. One problem is that JavaScript testing frameworks like QUnit require you to open an HTML page in a browser to run and view the test results. You can certainly launch an HTML page in a browser from a command line ("start mypage.htm"), but you won't be able to feed the results back to the CI server. We can get around this by using a tool like WatiN to control the browser from NUnit (or some other test framework which is supported by your CI server). WatiN allows you to spawn an instance of Internet Explorer, navigate to a URL, inspect the contents of the rendered DOM, and shutdown the browser, all from within an NUnit test.

A simple solution

Our first approach was to modify the QUnit test runner script so that it would create an element named TESTRESULTS that held the number of failed tests. We could then run the HTML page containing our tests and use WatiN to verify that TESTRESULTS contained a 0. An entire page of QUnit tests would be reported by NUnit as a single test. Either all of the QUnit tests passed making the single NUnit test pass, or any QUnit test failed and the NUnit test failed. You can see an example of this approach in my comment on Chad’s post referenced earlier. There are two problems with this approach: your total test count is inaccurate (you could have hundreds of QUnit tests which only show up as a single test in the CI test report), and more importantly, it is not immediately obvious which test failed, since it could have been any one of the many QUnit tests within a page.

Our current approach

A better approach would be to have a single NUnit test for each QUnit test. However, manually writing an NUnit test for each QUnit test sounds like a nightmare. What we need is a way to generate test cases dynamically. MbUnit supports this natively with its Factory attribute. We can get the same behavior in NUnit using the IterativeTest add-in (get the source and compile it against your version of NUnit). It allows you to specify a factory method that supplies a series of values to a single test method. When NUnit loads the class with this test method, it will create a separate test case for each value passed to the test method (if your factory method returns 3 values, you will have 3 test cases).

The full code to our test fixture is at the bottom of this post. The method GetQUnitTestResults takes an HTML page name as input and returns a series of QUnitTest instances. Each QUnitTest instance contains the details of an individual QUnit test run: the filename, the test name, the result, and any related failure message. The method RunQUnitTests is the actual factory method used by the IterativeTest add-in, and allows me to easily add new QUnit HTML pages without having to create new test methods. The gory details of parsing the DOM for the QUnit results are in grabTestResultsFromWebPage. Its not my proudest piece of code, but it gets the job done (for now). This is likely the only method you would need to change if you were using a JavaScript test framework other than QUnit.

Using this approach, we now get an accurate reflection of our test count, as each QUnit test is counted with every other NUnit test. Even better, when a test fails, we get detailed information about exactly which test failed and why. This is the output of a run that has a failing test:

------ Test started: Assembly: DovetailCRM.IntegrationTesting.dll ------

IterativeTest addin loaded.
TestCase 'DovetailCRM.IntegrationTesting.Web.JavaScript.JavaScriptTester.QUnit([tageditortester.htm] Post tag change to server module: should post the tag Name and item Id)'
failed: 
  the item that is tagged expected: 42 actual: 0
  Expected: collection containing "pass"
  But was:  < "fail" >
    D:\code\blue\source\DovetailCRM.IntegrationTesting\Web\JavaScript\JavaScriptTesters.cs(128,0): at DovetailCRM.IntegrationTesting.Web.JavaScript.QUnitTestHelpers.ShouldPass(QUnitTest theTest)
    D:\code\blue\source\DovetailCRM.IntegrationTesting\Web\JavaScript\JavaScriptTesters.cs(29,0): at DovetailCRM.IntegrationTesting.Web.JavaScript.JavaScriptTester.QUnit(Object current)


100 passed, 1 failed, 0 skipped, took 16.48 seconds.

Notes

NUnit add-ins must be compiled against the version of NUnit you are using. Make sure you have the same version of NUnit on your developer desktops and the CI server. We standardized on NUnit 2.4.7 simply because that was the latest version supported by TestDriven.NET.
The IterativeTest add-in must be in the addins sub-folder of the folder that contains your nunit-console.exe. If you want to run the tests via TestDriven.NET, you’ll also want to put a copy of the add-in in <ProgramFiles>\TestDriven.NET 2.0\NUnit\2.4\addins
While there is a way to get the TeamCity custom NUnit runner to load add-ins (using /addin:), I ran into trouble getting it all to work with WatiN as well. I discovered a JetBrains add-in for NUnit that allows nunit-console.exe to report progress to TeamCity in the same way as their custom runner. While it isn’t supposed to be available until TeamCity 4.0, I found the necessary files in this patch (see the Attachments section) which enables NUnit 2.4.7 support to TeamCity 3.1.

The Code

       1: [TestFixture]

       2: public class JavaScriptTester

       3: {

       4:     private IE _ie;

       5:  

       6:     [TestFixtureSetUp]

       7:     public void TestFixtureSetUp()

       8:     {

       9:         _ie = TestBrowser.GetInternetExplorer();

    }

   

    [IterativeTest("RunQUnitTests")]

    public void QUnit(object current)

    {

        ((QUnitTest)current).ShouldPass();

    }

  

    public IEnumerable RunQUnitTests()

    {

        return new[]

                   {

                       "ContextMenuTester.htm",

                       "FilterControlTester.htm",

                       "RepeaterControlTester.htm",

                       "FinderTester.htm",

                       "ConsoleScriptTester.htm",

                       "tageditortester.htm",

                       "CrudFormTester.htm"

                   }.SelectMany(page => GetQUnitTestResults(page));

    }

  

  

    public IEnumerable<QUnitTest> GetQUnitTestResults(string testPage)

    {

        TestFixtureSetUp();

        _ie.GoTo(string.Format("http://localhost/Content/scripts/tests/{0}", testPage));

        _ie.WaitForComplete(5);

  

        return grabTestResultsFromWebPage(testPage);

    }

  

    public IEnumerable<QUnitTest> grabTestResultsFromWebPage(string testPage)

    {

        // BEWARE: This logic is tightly coupled to the structure of the HTML generated by the QUnit testrunner

        // Also, this could probably be greatly simplified with a couple well-crafted xpath expressions

        var testOL = _ie.Elements.Filter(Find.ById("tests"))[0] as ElementsContainer;

        if (testOL == null) yield break;

        var documentRoot = XDocument.Load(new StringReader(makeXHtml(testOL.OuterHtml))).Root;

        if (documentRoot == null) yield break;

  

        foreach (var listItem in documentRoot.Elements())

        {

            var testName = listItem.Elements().First( x => x.Name.Is("strong")).Value;

            var resultClass = listItem.Attributes().First(x => x.Name.Is("class")).Value;

            var failedAssert = String.Empty;

            if (resultClass == "fail")

            {

                var specificAssertFailureListItem = listItem.Elements()

                    .First(x => x.Name.Is("ol")).Elements()

                    .First(x => x.Name.Is("li") && x.Attributes().First(a=> a.Name.Is("class")).Value == "fail");

                if (specificAssertFailureListItem != null)

                {

                    failedAssert = specificAssertFailureListItem.Value;

                }

            }

  

            yield return new QUnitTest

                             {

                                 FileName = testPage,

                                 TestName = removeAssertCounts(testName),

                                 Result = resultClass, Message = failedAssert

                             };

        }

  

    }

  

    private static string makeXHtml(string html)

    {

        return html.Replace("class=pass", "class=\"pass\"")

            .Replace("class=fail", "class=\"fail\"")

            .Replace("id=tests", "id=\"tests\"");

    }

  

  

    private static string removeAssertCounts(string fullTagText)

    {

        if (fullTagText == null) return String.Empty;

        int parenPosition = fullTagText.IndexOf('(');

        if (parenPosition > 0)

        {

            return fullTagText.Substring(0, parenPosition - 1);

        }

        return fullTagText;

    }

}

  

public class QUnitTest

{

    public string FileName { get; set; }

    public string TestName { get; set; }

    public string Result { get; set; }

    public string Message { get; set; }

  

    public override string ToString()

    {

        return string.Format("[{0}] {1}", FileName, TestName);

    }

}

  

public static class QUnitTestHelpers

{

    public static void ShouldPass(this QUnitTest theTest)

    {

        Assert.That(theTest.Result.Split(' '), Has.Member("pass"), theTest.Message);

    }

  

    public static bool Is(this XName xname, string name)

    {

        return xname.ToString().Equals(name, StringComparison.OrdinalIgnoreCase);

    }

}

It makes use of our TestBrowser class, which is just a simple wrapper around WatiN’s IE object.

       1: public static class TestBrowser

       2: {

       3:     public static readonly object LOCK_ROOT = new object();

       4:     public static IE _browser;

       5:  

       6:     public static IE GetInternetExplorer()

       7:     {

       8:         if( _browser == null )

       9:         {

      10:             lock(LOCK_ROOT)

      11:             {

      12:                 if( _browser == null )

      13:                 {

      14:                     IE.Settings.AutoMoveMousePointerToTopLeft = false;

      15:  

      16:                     _browser = (IE)BrowserFactory.Create(BrowserType.InternetExplorer);

      17:                     _browser.ShowWindow(NativeMethods.WindowShowStyle.Hide);

      18:                 }

      19:             }

      20:         }

      21:  

      22:         return _browser;

      23:     }

      24: }

Monkey patching rake for use with TeamCity

Joshua Flanagan — Wed, 10 Sep 2008 22:08:54 GMT

We use Ruby’s rake as our build automation tool. It provides a nice, XML-free way to define logical groupings of steps to perform, with dependency resolution.

We use JetBrain’s TeamCity as our continuous integration server. It has a neat feature that provides real-time reporting of build progress. TeamCity does not have any built-in support for rake*, so we use the simpleRunner build runner which allows you to kick off any process. However, when using the simpleRunner, you do not get any progress updates, since TeamCity doesn’t really know what is going on within your custom process.

Turns out, this is really easy to fix. TeamCity has a very simple API which allows a build script to interact with it. You just have to send specially formatted text messages to standard out (the console). For example, writing the following text to the console from your build script will cause the message “Starting compilation” to appear as the build status (on the project website, or the notification tray):

##teamcity[progressStart 'Starting compilation']

Now, it would be pretty annoying if we had to litter these messages throughout our build script. Fortunately, in ruby, classes are open by default. This means we can add or change behavior of existing classes, even if we do not have control over the source. In this case, I want to change the Execute method on Rake’s Task class so that it sends a message to TeamCity when a task starts and finishes. I use Jay Fields’ example of redefining a method that still needs to call its original implemention (thanks to Steven Harman for the help via twitter). I created a file [TeamCity.rb] with the following contents in the same directory as our RAKEFILE:

module TeamCity
    def teamcity_progress(task)
        teamcity_service_message 'progressStart', task
        yield
        teamcity_service_message 'progressFinish', task
    end

    def teamcity_service_message(message, message_value)
        puts "##teamcity[#{message} '#{message_value}']"
    end
end

class Rake::Task
include TeamCity
old_execute = self.instance_method(:execute)

       define_method(:execute) do |args|
         teamcity_progress("Executing #{name} rake task") { old_execute.bind(self).call(args) }
       end
end

I then conditionally include this patch (so that we do not see all of the annoying TeamCity messages when running the build locally) with the following line in our RAKEFILE:

require 'TeamCity.rb' if ENV["teamcity.dotnet.nunitlauncher"]

I just arbitrarily picked an environment variable that I know is defined when run within TeamCity, that is not defined on our local machines. There is probably a more appropriate/universally applicable environment variable which indicates “running in TeamCity”.

We now can see near real-time which rake task is currently running on the CI server. This can be nice when you are trying to fix a broken build, and you want to know if the current build has successfully passed the step that caused the failure. And yeah, it was also a fun way to explore monkey patching with ruby.

* There is actually an EAP for a Rake Build Runner, which I did not find out about until doing some research for this post. From the description, it sounds more geared toward people actually building ruby applications, which does not include us. All of our code is C#/.NET – only our build script is in ruby. However, its probably worth investigating further.

Hola Los Techies

Joshua Flanagan — Wed, 10 Sep 2008 03:05:29 GMT

I'm very happy to accept an invitation to join the Los Techies community. I've been posting at http://flimflan.com/blog for the past four years, and am looking forward to blogging more among some great company.

Subscribers to my old site should continue to get the new content, assuming they are subscribed via http://feeds.feedburner.com/JoshuaFlanagan. However, if you aren't already subscribed to the Los Techies main feed, I'd highly recommend it.

Since there is no code in this post, I'm going to keep it real short. Thanks again to the Los Techies crew for having me!