Thursday, September 08, 2011

Guided by Tests–Day Two

This post is third in a series about a group TDD experiment to build an application in 5 days using only tests.  Read the beginning here.

Today we break new ground on our application, starting with writing our first test. Today is still a teaching session, where I’ll write the first set of tests to demonstrate naming conventions and how to demonstrate how to use TDD with the rules we defined the day before. But first, we need to figure out where we should start.

Logical Flow

In order to determine where we should start it helps to draw out the logical flow of our primary use case: create a new dependency viewer from an NDepend AssembliesDependencies.xml file.  The logical flow looks something like this:

LogicalFlowDiagram

  1. User clicks “New”
  2. The user is prompted to select a file
  3. Some logical processing occurs where the file is read,
  4. …a graph is produced,
  5. …and the UI is updated.

The question on where to start is an interesting one. Given limited knowledge of what we need to build or how these components will interact, what area of the logical flow do we know the most about? What part can we reliably predict the outcome?

Starting from scratch, it seemed the most reasonable choice was to start with the part that reads our NDepend file. We know the structure of the file and we know that the contents of the file will represent our model.

Testing Constraints

When developing with a focus on testability, there are certain common problems that arise when trying to get a class under the test microscope. You learn to recognize them instantly, and I’ve jokingly referred to this as spidey-sense – you just know these are going to be problematic before you start.

While this is not a definitive list, the obvious ones are:

  • User Interface: Areas that involve the user-interface can be problematic for several reasons:
    • Some test-runners have a technical limitation and cannot launch a user-interface based on the threading model.
    • The UI may require complex configuration or additional prerequisites (style libraries, etc) and is subject to change frequently
    • The UI may unintentionally require human interaction during the tests, thereby limiting our ability to reliably automate.
  • File System: Any time we need files or folder structure, we are dependent on the environment to be setup a certain way with dummy data.
  • Database / Network: Being dependent on external services is additional overhead that we want to avoid. Not only will tests run considerably slower, but the outcome of the test is dependent on many factors that may not be under our control (service availability, database schema, user permissions, existing data).

Some of the less obvious ones are design considerations which may make it difficult to test, such as tight coupling to implementation details of other classes (static methods, use of “new”, etc).

In our case, our first test would be dependent on the file system. We will likely need to test several different scenarios, which will require many different files.  While we could go this route, working with the file system directly would only slow us down. We needed to find a way to isolate ourselves.

The team tossed around several different suggestions, including passing just xml as string. Ultimately, as this class must read the contents of the file we decided that the best way to work with Xml was an XmlReader. We could simulate many different scenarios by setting up a stream containing our test data.

Our First Test

So after deciding that the name of our class would be named NDependStreamParser, our first test looked something like this:

using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace DependencyViewer
{
    [TestClass]
    public class NDependStreamParserTests
    {
        [TestMethod]
        public void TestMethod1()
        {
            Assert.Fail();
        }
    }
}

We know very little about what we need. But at the very least, the golden rule is to ensure that all tests must fail from the very beginning. Writing “Assert.Fail();” is a good habit to establish.

In order to help identify what we need, it helps to work backward. So we start by writing our assertions first and then, working from the bottom up, fill in the missing pieces.  Our discovery followed this progression:

Realization Code Written
At the end of the tests, we’ll have some sort of results. The results should not be null.

At this point, test compiles, but it’s red.
Test Code:
object results = null;
Assert.IsNotNull( results,
           “Results were not produced.”);
Where will the results come from? We’ll need a parser. The results will come after we call Parse.

The code won’t compile because this doesn’t exist. If we use the auto-generate features of Visual Studio / Resharper the test compiles, but because of the default NotImplementedException, the test fails.
Test Code:
var parser = new NDependStreamParser(); 
object results = parser.Parse();
Assert.IsNotNull( results,
           “Results were not produced.”);
We need to make the test pass.

Do whatever we need to make it green.
Implementation:

public object Parse()
{
    // yes, it's a dirty hack. 
    // but now the test passes.
    return new object();
}
Our tests passes, but we’re clearly not done. How will we parse? The data needs to come from somewhere. We need to read from a stream.

Introducing the stream argument into the Parse method won’t compile (so the tests are red), but this is a quick fix in the implementation.
Test Code:

var parser = new NDependStreamParser();
var sr = new StringReader("");
var reader = XmlReader.Create(sr);
object results = parser.Parse(reader);
// ...

Implementation:

public object Parse(XmlReader reader) { //...
Our return type shouldn’t be “Object”. What should it be?

After a short review of the NDepend AssembliesDependencies.xml file, we decide that we should read the list of assemblies from the file into a model object which we arbitrarily decide should be called ProjectAssembly. At a minimum, Parse should return an IEnumerable<ProjectAssembly>.

There are a few minor compilation problems to address here, including the auto-generation of the ProjectAssembly class. These are all simple changes that can be made in under 60 seconds.
Test Code:

var parser = new NDependStreamParser();
var sr = new StringReader("");
var reader = XmlReader.Create(sr);
IEnumerable<ProjectAssembly> results 
    = parser.Parse(reader);
// ...

Implementation:

public IEnumerable<ProjectAssembly> Parse(
	XmlReader reader)
{
    return new List<ProjectAssembly>();
}

At this point, we’re much more informed about how we’re going to read the contents from the file. We’re also ready to make some design decisions and rename our test accordingly to reflect what we’ve learned. We decide that (for simplicity sake) the parser should always return a list of items even if the file is empty. While the implementation may be crude, the test is complete for this scenario, so we rename our test to match this decision and add an additional assertion to improve the intent of the test.

Sidenote: The naming convention for these tests is based on Roy Osherove’s naming convention, which has three parts:

  • Feature being tested
  • Scenario
  • Expected Behaviour
[TestMethod]
public void WhenParsingAStream_WithNoData_ShouldProduceEmptyContent()
{
    var parser = new NDependStreamParser();
    var sr = new StringReader("");
    var reader = XmlReader.Create(sr);

    IEnumerable<ProjectAssembly> results =
        parser.Parse(reader);

    Assert.IsNotNull( results,
        "The results were not produced." );

    Assert.AreEqual( 0, results.Count(),
        "The results should be empty." );
}

Adding Tests

We’re now ready to start adding additional tests. Based on what we know now, we can start each test with a proper name and then fill in the details.

With each test, we learn a little bit more about our model and the expected behaviour of the parser. The NDepend file contains a list of assemblies, where each assembly contains a list of assemblies that it references and a list of assemblies that it depends on. The subsequent tests we wrote:

  • WhenParsingAStream_ThatContainsAssemblies_ShouldProduceContent
  • WhenParsingAStream_ThatContainsAssembliesWithReferences_EnsureReferenceInformationIsAvailable
  • WhenParsingAStream_ThatContainsAssembliesWithDependencies_EnsureDependencyInformationIsAvailable

It’s important to note that these tests aren’t just building the implementation details of the parser, we’re building our model object as well. Properties are added to the model as needed.

Refactoring

Under the TDD mantra “Red, Green, Refactor”, “Refactor” implies that you should refactor the implementation after you’ve written the tests. However, the scope of the refactor should apply to both tests and implementation.

Within the implementation, you should be able to optimize the code freely assuming that you aren’t adding additional functionality. (My original implementation details of using the XmlParser was embarrassing, and I ended up experimenting with the reader syntax later that night until I found a clean elegant solution. The tests were invaluable for discovering what was possible.)

Within the tests, refactoring means removing as much duplication as possible without obscuring the intent of the test. By the time we started the third test, the string-concatenation to assemble our xml and plumbing code to create our XmlReader was copied and pasted several times. This plumbing logic slowly evolved into a utility class that used an XmlWriter to construct our test data.

Next: Day Three

submit to reddit

Wednesday, September 07, 2011

Guided by Tests–Day One

This post is second in a series about a group TDD experiment to build an application in 5 days using only tests.  Read the beginning here.

Today is the pitch. We'll talk about what we're going to build and how we're going to do it, but first we need to understand the motivation behind our methodology. We need to understand why we test.

Methodology

Kent Beck's TDD by Example was one of the first books I read about TDD and to this day it's still one of my favourites. I know many prefer Roy Osherove's Art of Unit Testing (which I also highly recommend) because it's newer and has .net examples, but in comparison they are very different books.  Roy's book represents the evolved practice of Unit Testing and it provides a solid path to understand testing and the modern testing frameworks that have taken shape since Kent's book was written in 2002. Kent's book is raw, the primordial ooze that walked upon land and declared itself sentient, it defines the methodology as an offshoot of extreme programming and is the basis for the frameworks Roy explains how to use. If you're looking for a book on how to start and the mechanics of TDD, this is it.

As an offshoot of extreme programming, the core philosophy of TDD is:

  • Demonstrate working software at all times
  • Improve confidence while making changes
  • Build only the software you need

My experiment is based on the strategies defined in Kent's book – the premise is to demonstrate working software and get early feedback – but in a nutshell, you make incredibly small incremental changes and compile and run tests a lot (30-50 times per hour). I sometimes refer to this rapid code/test cycle as the Kent Beck Method or Proper TDD. I don't necessarily follow this technique when I code, but it's a great way to start until you learn how to find your balance. Eventually, running tests becomes instinct or muscle memory – you just know when you should.

The Rules

For our experiment, we followed these rules:

1. Tests must be written first.

This means that before we do anything, we start by writing a test. If we need to create classes in order to get the test to compile, that is a secondary concern: create the test first, even if it doesn’t compile, then introduce what you need.

2. Code only gets written to satisfy a test.

This means that we write only the code that is needed to make a test pass. If while writing the code you're tempted to add additional functionality that you might need, don't succumb to writing that code now. Instead, focus on the current test and make a note for additional tests. This ensures that we only write the code that is needed.

3. Code must always compile (at least within 30 seconds of making a change)

This may seem like a difficult practice to adopt, but it is extremely valuable. To follow this rule, make small changes that can be corrected easily and compile frequently to provide early feedback. If you find yourself breaking a contract that takes twenty minutes to fix, you're doing it wrong.

4. Time between red/green must be very short (<1 minute, meaning rollback if you're not sure.)

This is also a very difficult rule to keep, but it forces you to recognize the consequences of your changes. You should know before you run the test whether it's going to pass or fail. Your brain will explode when you're wrong (and that’s a good thing).

The Problem

As mentioned in the first post, my current project uses NDepend to perform static analysis as part of our build process. The build generates a static image. As our solution analyzes over 80 assemblies, the text becomes illegible, and its somewhat difficult to read (image text has been blurred to hide assembly names).

Geez!

The two primary use cases I wanted the application to have:

  • Allow the user to select an NDepend AssembliesDependencies.xml file and display it as a graph
  • Once the graph has been rendered, provide an option to choose which assemblies should be displayed

The graph would be implemented using Graph#, and we could borrow much of the presentation details from Sacha Barber’s post. Using Graph# would provide a rich user-experience, all we’d need to do is implement the surrounding project framework.  Piece of cake!

I asked the team if there were any additional considerations that they would like to see. We would take some of these considerations into account when designing the solution. Some suggestions were made:

  • Multi-tab support so that more than one graph can be open at a time
  • Ability to color code the nodes
  • Ability to save a project (and load it presumably)

Getting Started

With the few remaining minutes in the lunch hour, we set up the project:

  1. Create a new WPF Project.  Solution Name: DependencyViewer, Project Name: DependencyViewer.
  2. Create a new Test Project, DependencyViewer.Tests
  3. Added a reference from the Test Project to the WPF Project.
  4. Deleted all automatically generated files:
    1. App.xaml
    2. MainWindow.xaml
    3. TestClass1.
  5. Renamed the default namespace of the Test Project to match the namespace of the WPF Application. For more info, read this detailed post which explains why you should.

While I have some preferences on third-party tools, etc – we’ll get to those as they’re needed.

Next: Day Two

submit to reddit

Tuesday, September 06, 2011

Guided By Tests

Last week I started a TDD experiment at work. I gathered some interest from management and then sought out participants with a very simple concept: build an application in 5 days, over lunch hour, using nothing but unit tests to drive development.

My next few posts will be part of a series that shares the details of that experiment.

Motivation

Over the years, I've had many conversations with colleagues about TDD. One of the most common comments made is that they don't know what to test or where to start. Another common theme is the desire to get into a project at the very beginning where all the project members have bought into the concept. I've always found this comment to be strange. It's a great comment and it makes perfect sense but -- why the beginning? Why not the middle or end of the project after the tests are put into place? I sense they're expressing two things. First they're expressing the obvious, that life for the development team would be much different if they had tests from the beginning. And secondly, they're interested in understanding how the team managed to establish and sustain the process.

The goal of my experiment would show how to start a project with TDD and to walk through the process of deconstructing requirements into testable components. The process would use tests as the delivery mechanism but would also showcase how to design software for testability using separation of concerns and SOLID OO design principles. Requirements and documentation would be deliberately vague, and the application would need to showcase common real world problems. The tricky part would be finding an application to build.

Fortunately, I was recently inspired by a post by Sacha Barber that provides a great example of how to use the open source graphing framework Graph# to build a graph application in WPF. I immediately saw how I could use this. My current project uses NDepend for static analysis and as part of the build process it produces an image of our dependency graph. With over 80 nodes and illegible text, the static image is mostly useless. However, the build process spits out the details for the graph as an XML file. With some simple xml parsing, I could use Sacha's example to build my own graph.

So rather than spending my weekend working on this application, why not turn this into a teaching exercise? (Ironically, I'm spending my weekends blogging about it)

Follow up posts:

submit to reddit

Friday, August 12, 2011

On Dependency Injection and Violating Encapsulation Concerns

For me, life is greatly simplified when dependencies are inverted and constructor injection is used to provide a clean mechanism to introduce dependencies to a class. It's explicit and easy to test.

Others however argue that some dependencies are private implementation details to the class and external callers shouldn't know about them. The argument is that exposing these dependencies through the constructor violates encapsulation and introduces coupling. This argument is usually coupled with resistance to using interfaces or classes with virtual methods.

From my experience, there are times when composition in the constructor makes sense but there's a fine line when constructor injection should be used. I want to use this post to elaborate on these arguments and provide my perspective on this debate.

Does Constructor Injection violate Encapsulation?

Does exposing the internal dependencies of a class in the constructor expose the implementation details of a class? If you are allowing callers to construct the class directly, then you are most certainly breaking encapsulation as the callers must posses the knowledge of how to construct your class.  However, regardless of the constructor arguments if callers know how to construct your class you are also coupling to a direct implementation and will undoubtedly create a test impediment elsewhere. There are some simple fixes for this (including more constructor injection or another inversion of control technique) which I'll outline later.

So if constructor injection violates encapsulation, when is it safe to use composition in the constructor?

It really depends on the complexity and relationship of the subject to its dependencies. If the internal dependencies are simple and contain no external dependencies there is likely no harm in using composition to instantiate them in the constructor (the same argument can be made for small static utility methods). For instance, a utility class that performs some translation or other processing activity with limited outcomes may work as a private implementation detail. Problems arise as soon as these dependencies take on further dependencies or produce output that influences the conditional logic of the subject under test. When this happens the argument to keep these as internally controlled dependencies becomes flawed and it may be necessary to upgrade the "private implementation detail" to an inverted dependency.

Before:

This example shows a class that has a private implementation detail that influences logic of the subject under test. The consequence of this design is that the internal details of the validator leak into the test specifications. This leads to very brittle tests as tests must concern themselves with object construction and validation rules -- any change to the object or validation logic will break the tests.

public class MyModelTranslator
{
    public MyModelTranslator()
    {
       _validator = new MyModelValidator();
    }

    public MyViewModel CreateResult(MyModelObject model)
    {
        var result = new MyViewModel();
        
        if (_validator.Validate( model ));
        {
             result.Id = model.Id;
        }
        else {
            result.State = MyResultState.Invalid;
        }
        return result;
    }
}

[Test]
public void WhenCreatingAResult_FromAVaidModelObejct_ShouldHaveSameId()
{
    var subject = new MyModelTranslator();

    // create model object with deep understanding how Id's
    // and expiry dates are related 
    var model = new MyModelObject()
    {
        Id = "S1402011"
        Expiry = Datetime.Parse("12/31/2011");
    };
    var result = subject.CreateResult( model );
    Assert.AreEqual("S1402011", result.Id);
}

After:

This example shows the above example corrected to use a constructor injected dependency that can be mocked. In doing so, the test is not encumbered with object creation or validation details and can easily simulate validate states without introducing brittleness.

public class MyModelTranslator
{
    public MyModelTranslator(MyModelValidator validator);
    {
       _validator = validator;
    }

    // ...
}

[Test]
public void WhenCreatingAResult_FromAVaidModelObejct_ShouldHaveSameId()
{
    var validatorMock = new Mock<MyModelValidator>();

    var subject = new MyModelTranslator(validatorMock.Object);
    var model = new MyModelObject()
    {
      Id = "Dummy"
    };

    // we no longer care how validation is done, 
    // but we expect validation to occur and can easily control
    // and test outcomes
    ValidatorMock.Setup( x => x.Validate( model )).Returns( true );

    var result = subject.CreateResult( model );
    Assert.AreEqual("Dummy", result.Id);
}

Combatting Coupling with Inversion of Control

As the above points out, if we expose the dependencies of a class in the constructor we move the coupling problems out of the class and into the callers. This is not good but it can easily resolved.

An example that shows that our controller class now knows about the MyModelValidator:

public class MyController
{
    public MyController()
    {
        _translator = new MyModelTranslator( new MyModelValidator() );
    }
}

More Constructor Injection

Ironically, the solution for solving coupling and construction problems is more constructor injection. This creates a Russian Doll where construction logic is deferred and pushed higher and higher up the stack resulting in a top level component which is responsible for constructing the entire object graph. While this provides an intuitive API that clearly outlines dependencies, it's tedious to instantiate by hand. This is where dependency injection  frameworks like Unity, StructureMap and others come in: they can do the heavy lifting for you. If done right, your application should only have well defined points where the dependency container is used.

public class MyController : IController
{
    public MyController(MyModelTranslator translator)
    {
        _translator = translator;
    }
}

Note that this assumes that the constructor arguments are abstractions -- either interfaces, abstract classes or classes with virtual methods. In effect, by exposing the constructor arguments we trade off the highly sealed encapsulated black box for an open for extensibility model.

Factories / Builders

In some cases, using Constructor Injection everywhere might not be a good fit. For example, if you had a very large and complex object graph, creating everything upfront using constructor injection might represent a performance problem. Likewise, not all parts of the object graph will be used immediately and you may need to defer construction until needed.  In these cases, the good ol' Gang of Four Factory pattern is a handy mechanism to lazy load components.

So rather that construct the entire object graph and pass resolved dependencies as constructor arguments, pass the factory instead and create the lower-level sub-dependencies when needed.

public class MyControllerFactory : IControllerFactory
{
    private IUnityContainer _container;
    
    public MyControllerFactory(IUnityContainer container)
    {
        _container = container;
    }

    public IController Create()
    {
        _container.Resolve<MyController>();
    }
}

Service Location

While Service Location is an effective mechanism for introducing inversion of control, I'm listing it last for a reason. Unlike Constructor Injection, Service Location couples all your code to an intermediate provider. Depending on your application and relative complexity this might be acceptable, but from personal experience it's a very slippery slope -- actually, it would be more appropriate to call it a cliff because removing or replacing the container from an existing codebase can be a massive undertaking.

I see service location as a useful tool when refactoring legacy code towards Constructor Injection. The lower "leaf-nodes" of the object graph can take advantage of constructor injection and the higher nodes can temporarily use service location to create the "leaves". As you refactor, the service locator is removed and replaced with constructor injection, which results in an easier to test and discoverable API. This also provides a pragmatic bottom up refactor approach.

Summary

  • Use "new" when dependencies have no external dependencies or don't influence conditional flow of the subject.  Conversely, use constructor injection when dependencies have additional dependencies.
  • Use an IoC container to construct objects that use constructor injection, but only use the IoC container in high level components that are responsible for construction of the object graph. If constructing the entire object graph is a performance concern, consider encapsulating the container in a Factory that can be used to resolve remaining parts of the object graph when needed.
  • To prevent violating encapsulation concerns, use inversion of control to promote using abstractions instead of concrete dependencies.

submit to reddit

Thursday, July 14, 2011

So You've Decided To Go "Full Retard"!

Congratulations! You've done something very clever! Your out of the box thinking has now retarded the development process!

black-downey-simple-jack

Oh? You're not sure what I'm talking about? Well remember that thing you did? I'm pretty sure you knew that it felt like a hack when you wrote it but then you quickly convinced yourself that you'd invented something insanely brilliant. Well, it wasn't. It was a hack. And now, somewhere, kittens are dying.

Yes, I know the compiler didn't complain but that doesn't technically make it valid code. Just because you can use operator overloading to concatenate files doesn't mean you should.  Oh yeah and remember that non-standard event signature you implemented to save time? Well it turns out that we really did need event arguments and proper error handling, so now Jimmy, who is replacing you btw, is going to have to rewrite it. kthxbai.

It's not clever, or agile or lean or whatever it is you think it is. It's sloppy. I can't think of a profession where cutting corners is okay.

So before you start bringing the protestors to your aid that "retarded" is an offensive word, I mean it for the true sense, for slowing down the development or progress of action, process, etc.  Anytime a developer squints and wonders what was going through your mind -- you've failed to help others understand your code.  And know what the funny thing is? It doesn't take much more to do it right. It might take an few extra seconds to put xml comments at the top of the method, or a few extra minutes to write a unit test.  If you consider that we spend more time trying to understand code than writing it, it really makes sense to ensure that code remains consistent.

For me, clever is often synonymous with stupid. Everyone's entitled to a few embarrassing "clever" moments a year as long as the own up to it. Going full in and swearing by it, well, …sometimes you come back empty handed.

submit to reddit

Tuesday, July 12, 2011

Visual Studio Regular Expressions for Find & Replace

Visual Studio has had support for regular expressions for Find & Replace for several versions, but I've only really used it for simple searches. I recently had a problem where I needed to introduce a set of changes to a very large object model. It occurred to me that this could be greatly simplified with some pattern matching, but I was genuinely surprised to learn that Visual Studio had their own brand of Regular Expressions.

After spending some time learning the new syntax I had a really simple expression to modify all of my property setters:

Original:

public string PropertyName
{
    get { return _propertyName; }
    set
    {
        _propertyName = value;
        RaisePropertyChanged("PropertyName");
    }
}

Goal:

public string PropertyName
{
    get { return _propertyName; }
    set
    {
        if ( value == _propertyName )
             return;            
        _propertyName = value;
        RaisePropertyChanged("PropertyName");
    }
}

Here’s a quick capture and breakdown of the pattern I used.

image

Find:

^{:Wh*}<{_:a+} = value;
  • ^ = beginning of line
  • { = start of capture group #1
  • :Wh = Any whitespace character
  • * = zero or more occurrences
  • } = end of capture group #1
  • < = beginning of word
  • { = start of capture group #2
  • _ = I want to the text to start with an underscore
  • :a = any alpha numerical character
  • + = 1 or more alpha numerical characters
  • } end of capture group #2
  • “ = value;” = exact text match

Replace:

\1(if (\2 == value)\n\1\t\return;\n\1\2 = value;

The Replace algorithm is fairly straight forward, where “\1” and “\2” represent capture groups 1 and 2.  Since capture group #1 represents the leading whitespace, I’m using it in the replace pattern to keep the original padding and to base new lines from that point.  For example, “\n\1\t” introduces a newline, the original whitespace and then a new tab.

It’s seems insane that Microsoft implemented their own regular expression engine, but there’s some interesting things in there, such as being able to match on quoted text, etc.

I know this ain’t much, but hopefully it will inspire you to write some nifty expressions.  Cheers.

submit to reddit

Wednesday, June 29, 2011

Build Server Code Analysis Settings

I've been looking at ways to improve the reporting of Code Analysis as part of our Team Build. During my research I found that the RunCodeAnalysis setting as defined in the TFSBuild.proj differs significantly from the local MSBuild project schema options (see this post for a detailed break down of project level analysis settings).

Specifically, TFSBuild defines RunCodeAnalysis as "Always", "Default" and "Never" while MSBuild defines this as a simple true/false Boolean.  To make sense of this I hunted this down to this section of the Microsoft.TeamFoundation.Build.targets file:

<Target Name="CoreCompileSolution">

  <PropertyGroup>
    <CodeAnalysisOption Condition=" '$(RunCodeAnalysis)'=='Always'">RunCodeAnalysis=true</CodeAnalysisOption>
    <CodeAnalysisOption Condition=" '$(RunCodeAnalysis)'=='Never'">RunCodeAnalysis=false</CodeAnalysisOption>
    <!-- ... -->
  </PropertyGroup>
  <!-- ... -->
</Target>

From this we can infer that "Default" setting does not provide a value to the runtime, while "Always" and "Never" map to True/False respectively.  But this raises the question, what's the difference between Default and Always, and what should I specify to effectively run Code Analysis?

To answer this question, let's take an example of a small project with two projects.  Both projects are configured in the csproj with default settings.  For the purposes of the demo, I've altered the output folder to be at a folder called Output at the root of the solution:

<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
  <DebugSymbols>true</DebugSymbols>
  <DebugType>full</DebugType>
  <Optimize>false</Optimize>
  <OutputPath>..\Output</OutputPath>
  <DefineConstants>DEBUG;TRACE</DefineConstants>
  <ErrorReport>prompt</ErrorReport>
  <WarningLevel>4</WarningLevel>
</PropertyGroup>

When we specify code analysis to run with the "Always" setting:

Msbuild.exe Example.sln /p:RunCodeAnalysis=True

The Output folder contains the following files:

CodeAnalysisExperiment1.dll 
CodeAnalysisExperiment1.dll.CodeAnalysisLog.xml 
CodeAnalysisExperiment1.dll.lastcodeanalysissucceeded 
CodeAnalysisExperiment1.pdb 
CodeAnalysisExperiment2.dll 
CodeAnalysisExperiment2.dll.CodeAnalysisLog.xml 
CodeAnalysisExperiment2.dll.lastcodeanalysissucceeded 
CodeAnalysisExperiment2.pdb

Close inspection of the console output shows that the Minimal ruleset was applied to my empty projects, despite having not configured either project for code analysis.

DefaultRuleset

If I tweak the project settings slightly such that one project explicitly declares RunCodeAnalysis as True and the other as False

<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "> 
  <DebugSymbols>true</DebugSymbols> 
  <DebugType>full</DebugType> 
  <Optimize>false</Optimize> 
  <OutputPath>..\Output</OutputPath> 
  <DefineConstants>DEBUG;TRACE</DefineConstants> 
  <ErrorReport>prompt</ErrorReport> 
  <WarningLevel>4</WarningLevel> 
  <RunCodeAnalysis>false</RunCodeAnalysis> 
</PropertyGroup>

and…

<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "> 
  <DebugSymbols>true</DebugSymbols> 
  <DebugType>full</DebugType> 
  <Optimize>false</Optimize> 
  <OutputPath>..\Output</OutputPath> 
  <DefineConstants>DEBUG;TRACE</DefineConstants> 
  <ErrorReport>prompt</ErrorReport> 
  <WarningLevel>4</WarningLevel> 
  <RunCodeAnalysis>true</RunCodeAnalysis> 
</PropertyGroup>

...and then specify the "Default" setting:

msbuild Example.sln

The Output folder now contains the following files:

CodeAnalysisExperiment1.dll 
CodeAnalysisExperiment1.dll.CodeAnalysisLog.xml 
CodeAnalysisExperiment1.dll.lastcodeanalysissucceeded 
CodeAnalysisExperiment1.pdb 
CodeAnalysisExperiment2.dll 
CodeAnalysisExperiment2.pdb

From this we can infer that the "Default" setting looks to the project settings to determine if analysis should be done, whereas the "Always" setting will override the analysis and "Never" will disable analysis altogether.

For my purposes, I have some projects (unit tests, etc) that I don't necessarily want to run code analysis on -- for me, "Always" is a bit of a deal breaker.  However, there's an interesting lesson to take away here: command line arguments passed to msbuild are carried to all targets.  Which means we can override most project settings this way:

// use 'all rules' for projects that have analysis turned on 
msbuild.exe Example.sln /p:CodeAnalysisRuleSet="Allrules.set"

// use a custom dictionary during code analysis 
msbulid.exe Example.sln /p:CodeAnalysisDictionary="..\AnalysisDictionary.xml"

// redirect output to a different folder 
msbulid.exe Example.sln /p:OutputPath="..\Output2"

Cheers!

Happy Coding.

submit to reddit

Tuesday, June 28, 2011

The Mystery of Cascading FxCop Errors

As I mentioned in my last post, I've been working through FxCop concerns on my current project. To provide some context, this is a large project with more than 500K lines of code, and FxCop was added late to the development process. Naturally, out of the gate there were thousands of errors, so we had to ease it into the development process by slowly turning on rules until we had the warnings under control.

To facilitate this, we had a dedicated team go through the entire code base and address as many of the errors as they could: implementing IDispose correctly, validating arguments, etc. In the end, we had a hand picked set of rules that applied to our scenarios and a code base that adhered to them. Once the existing violations were under control, we flipped the switch to treat new analysis warnings as errors to ensure that the violations were not introduced into source control.

There were some bumps along the way, but in the end it was manageable (royal pain in the butt, but manageable).

Recently however, my team started to see serious problems with code analysis throughout parts of the application. They found that if they made a minor change, a series of FxCop violations would appear -- even in areas that they didn't touch or had previously been reviewed and considered "warning free". The team believed that FxCop was somehow incrementally analyzing files, thereby finding new problems which had previously been skipped. While this theory fit into the symptom of new violations for existing code, it just didn't seem right: FxCop analyzes compiled assemblies, not individual files.

The problem with code analysis errors is that they're not always obvious what is causing the problem. It's easy to fall into a trap to believe that Code Analysis is only looking at the internal structure or logic of a class for correctness. There are many FxCop rules that look for correctness, such as Methods that can be private or static, arguments that can be null, etc. The really obscure errors are the ones that look at how a class is used by other classes. What may seem like a trivial change can introduce a different execution path that suddenly exposes the class and its dependencies for consideration of new FxCop violations.

Let's take a look at a contrived example. Let's say I'm using a Service Locator to wire up a ViewModel and like any good Inversion of Control junkie, I'm using an interface for that ViewModel. According to FxCop, the following example is good and life is fine.

public class CustomApp : Application 
{ 
    public override void OnStartup() 
    { 
        var view = new MainView(); 
        view.DataContext = new MainViewModel(); 
        view.Show(); 
    } 
}

internal class MainViewModel 
{ 
    public ISearchViewModel SearchViewModel { get; set;}

    public MainViewModel() 
    { 
        SearchViewModel = ServiceLocator.Get<ISearchViewModel>(); 
    } 
}

As smart developers, we decide that we don't really need the ServiceLocator to get our SearchViewModel. The concrete SearchViewModel doesn't have any complex construction logic and we probably will never swap it out with a different instance at runtime -- this is a little hit against dependency inversion but from a testing perspective, I can always introduce a Mock through property injection. Upon introducing this change we discover several new FxCop violations that weren't present before -- nearly a dozen warnings in unrelated classes about objects not being properly disposed. Out of all the warnings, only one warning is specific to the change we've made in this class but it doesn't seem obvious: CA2000: Dispose objects before losing scope.

public class MainViewModel 
{ 
    public ISearchViewModel SearchViewModel { get; set;}

    public MainViewModel() 
    { 
        // FxCop: CA2000 - Dispose object before losing scope
        SearchViewModel = new PrimarySearchViewModel();
    } 
}

This violation takes a few minutes to sort out because we're assigning the object to a property, and it isn't going out of scope. Upon further inpsection, it seems that the concrete class implements IDisposable, but the interface does not. FxCop is warning us that once the constructor goes out of scope, our concrete class will become polymorphically assigned to an interface that doesn't support IDispose and as such, we won't have an opportunity to call the Dispose method on the concrete class. Clever girl.

The easy fix is to add IDispose onto the interface and then implement the dispose pattern for the MainViewModel. All of this makes sense, and we've just fixed a memory leak that was there all along. But why did we have violations in all these other classes?

It turns out that PrimarySearchViewModel implemented IDispose improperly and it also constructed several other IDisposable types with the same problem in its constructor. These violations, like the PrimarySearchViewModel one, are legitimate. Now, if you're following along, you'll realize I haven't answered the question. If all these classes implemented IDispose improperly, why weren't these found earlier?

The answer is because we changed the usage. No one was calling the constructor previously -- the ServiceLocator used reflection to hunt down and construct our object for us, but by switching to use the constructor directly the analysis engine did it's part to highlight problems. It's hard to say whether the analysis engine should have caught this potential problem or if it's a feature that only inspects currently active code. At any rate, I hope this help you or your team -- before dismissing FxCop violations as fluke, look at how the usage of your objects has changed.

submit to reddit

Monday, June 27, 2011

Visual Studio Code Analysis Settings

I've just spent the last week or so working with Code Analysis for my Visual Studio 2010 projects.  I still haven't figured out if I'm using Visual Studio's built in Code Analysis tool or FxCop 10.0, because the two tools share a lot of the same engine and rulesets.  At any rate, one of the pain points I had to work through was ensuring that the configuration settings between my Visual Studio configurations were the same.  Strangely enough, I had more confidence in my changes when manually editing the csproj files in Notepad++ than from Visual Studio -- it seemed like each configuration had slightly different settings, and being able to see the msbuild settings seemed to make more sense.

During this process, I found it quite frustrating that none of the CodeAnalysis settings that are configurable from the csproj schema were documented online.  However, the intellisense for the schema is quite good.  The schema is located at: <Program Files>\Microsoft Visual Studio 10.0\Xml\Schemas\1033\MSBuild\Microsoft.Build.CommonTypes.xsd

Most of the settings generated by Visual Studio are often the default values and can be discarded.

For my reference, here's the documentation, lifted from the schema:

Project Element Description
CodeAnalysisAdditionalOptions Additional options to pass to the Code Analysis command line tool.
CodeAnalysisApplyLogFileXsl Indicates whether to apply the XSL style sheet specified in $(CodeAnalysisLogFileXsl) to the Code Analysis report. This report is specified in $(CodeAnalysisLogFile). The default is false.
CodeAnalysisConsoleXsl Path to the XSL style sheet that will be applied to the Code Analysis console output. The default is an empty string (''), which causes Code Analysis to use its default console output.
CodeAnalysisCulture Culture to use for Code Analysis spelling rules, for example, 'en-US' or 'en-AU'. The default is the current user interface language for Windows.
CodeAnalysisDependentAssemblyPaths Additional reference assembly paths to pass to the Code Analysis command line tool. A fully qualified path to a directory containing reference assemblies to be used by Code Analysis.
CodeAnalysisDictionary Code Analysis custom dictionaries.  Semicolon-separated list of Code Analysis custom dictionaries. Wildcards are allowed.
CodeAnalysisFailOnMissingRules Indicates whether Code Analysis should fail if a rule or rule set is missing. The default is false.
CodeAnalysisForceOutput Indicates whether Code Analysis generates a report file, even when there are no active warnings or errors. The default is true.
CodeAnalysisGenerateSuccessFile Indicates whether Code Analysis generates a '$(CodeAnalysisInputAssembly).lastcodeanalysissucceeded' file in the output folder when no build-breaking errors occur. The default is true.
CodeAnalysisIgnoreBuiltInRules Indicates whether Code Analysis will ignore the default rule directories when searching for rules. The default is false.
CodeAnalysisIgnoreBuiltInRuleSets Indicates whether Code Analysis will ignore the default rule set directories when searching for rule sets. The default is false.
CodeAnalysisIgnoreInvalidTargets Indicates whether Code Analysis should silently fail when analyzing invalid assemblies, such as those without managed code. The default is true.
CodeAnalysisIgnoreGeneratedCode Indicates whether Code Analysis should fail silently when it analyzes invalid assemblies, such as those without managed code. The default is true.
CodeAnalysisImport Code Analysis projects (*.fxcop) or reports to import. Semicolon-separated list of Code Analysis projects (*.fxcop) or reports to import. Wildcards are allowed.
CodeAnalysisInputAssembly Path to the assembly to be analyzed by Code Analysis. The default is '$(OutDir)$(TargetName)$(TargetExt)'.
CodeAnalysisLogFile Path to the output file for the Code Analysis report. The default is '$(CodeAnalysisInputAssembly).CodeAnalysisLog.xml'.
CodeAnalysisLogFileXsl Path to the XSL style sheet to reference in the Code Analysis output report. This report is specified in $(CodeAnalysisLogFile). The default is an empty string ('').
CodeAnalysisModuleSuppressionsFile Name of the file, without the path, where Code Analysis project-level suppressions are stored. The default is 'GlobalSuppressions$(DefaultLanguageSourceExtension)'.
CodeAnalysisOverrideRuleVisibilities Indicates whether to run all overridable Code Analysis rules against all targets. This will cause specific rules, such as those within the Design and Naming categories, to run against both public and internal APIs, instead of only public APIs. The default is false.
CodeAnalysisOutputToConsole Indicates whether to output Code Analysis warnings and errors to the console. The default is false.
CodeAnalysisVerbose Indicates whether to output verbose Code Analysis diagnostic info to the console. The default is false.
CodeAnalysisPath Path to the Code Analysis installation folder. The default is '$(VSINSTALLDIR)\Team Tools\Static Analysis Tools\FxCop'.
CodeAnalysisPlatformPath Path to the .NET Framework folder that contains platform assemblies, such as mscorlib.dll and System.dll. The default is an empty string ('').
CodeAnalysisProject Path to the Code Analysis project (*.fxcop) to load. The default is an empty string ('').
CodeAnalysisQuiet Indicates whether to suppress all Code Analysis console output other than errors and warnings. This applies when $(CodeAnalysisOutputToConsole) is true. The default is false.
CodeAnalysisRuleAssemblies Semicolon-separated list of paths either to Code Analysis rule assemblies or to folders that contain Code Analysis rule assemblies. The paths are in the form '[+|-][!][file|folder]', where '+' enables all rules in rule assembly, '-' disables all rules in rule assembly, and '!' causes all rules in rule assembly to be treated as errors. For example '+D:\Projects\Rules\NamingRules.dll;+!D:\Projects\Rules\SecurityRules.dll'. The default is '$(CodeAnalysisPath)\Rules'.
CodeAnalysisRuleDirectories Semicolon-separated list of directories in which to search for rules when resolving a rule set. The default is '$(CodeAnalysisPath)\Rules' unless the CodeAnalysisIgnoreBuiltInRules property is set to true.
CodeAnalysisRules Semicolon-separated list of Code Analysis rules. The rules are in the form '[+|-][!]Category#CheckId', where '+' enables the rule, '-' disables the rule, and '!' causes the rule to be treated as an error. For example, '-Microsoft.Naming#CA1700;+!Microsoft.Naming#CA1701'. The default is an empty string ('') which enables all rules.
CodeAnalysisRuleSet A .ruleset file which contains a list of rules to run during analysis. The string can be a full path, a path relative to the project file, or a file name. If a file name is specified, the CodeAnalysisRuleSetDirectories property will be searched to find the file. The default is an empty string ('').
CodeAnalysisRuleSetDirectories Semicolon-separated list of directories in which to search for rule sets. The default is '$(VSINSTALLDIR)\Team Tools\Static Analysis Tools\Rule Sets' unless the CodeAnalysisIgnoreBuiltInRuleSets property is set to true.
CodeAnalysisSaveMessagesToReport Comma-separated list of the type ('Active', 'Excluded', or 'Absent') of warnings and errors to save to the output report file. The default is 'Active'.
CodeAnalysisSearchGlobalAssemblyCache Indicates whether Code Analysis should search the Global Assembly Cache (GAC) for missing references that are encountered during analysis. The default is true.
CodeAnalysisSummary Indicates whether to output a Code Analysis summary to the console after analysis. The default is false.
CodeAnalysisTimeout The time, in seconds, that Code Analysis should wait for analysis of a single item to complete before it aborts analysis. Specify 0 to cause Code Analysis to wait indefinitely. The default is 120.
CodeAnalysisTreatWarningsAsErrors Indicates whether to treat all Code Analysis warnings as errors. The default is false.
CodeAnalysisUpdateProject Indicates whether to update the Code Analysis project (*.fxcop) specified in $(CodeAnalysisProject). This applies when there are changes during analysis. The default is false.
CodeAnalysisUseTypeNameInSuppression Indicates whether to include the name of the rule when Code Analysis emits a suppression. The default is true.
RunCodeAnalysis Indicates whether to run Code Analysis during the build.

As this has been a fun week dealing with FxCop, expect to see a few more posts related to the above settings.  Stay right here and click refresh endlessly, or subscribe to the rss feed.

Happy coding.

submit to reddit

Tuesday, May 31, 2011

The Tests are Broken, Now What?

Despite our best intentions to write durable tests, it seems inevitable that tests will break at some point. In some cases, breaking a test can be seen as a very positive thing: we've introduced a side-effect that has broken existing functionality, and our tests have helped prevent a bug. However, in some cases, tests break because of external factors -- maybe the tests weren't isolated enough, maybe a developer went rogue and forgot to run the tests, or some other poorly justified or lousy excuse. While identifying what went wrong in your development process is important, the immediate concern is that these broken tests represent a failed build and developer productivity is at risk -- what should you do with the broken tests?

If the developer looking at the tests knows a bit about the code, they should have enough information in the tests to help sort things out.  The developer should drop what they're doing, slow down and research the code and the tests and fix the broken tests. If caught early, it's very manageable. Unfortunately, this isn't the common case -- the developer introducing the breaking changes doesn't understand the tests and is ultimately left with one of two approaches:

  1. Comment out or remove the test
  2. Mark the test as Ignored

Neither of these approaches "fix" anything.  The first approach, to comment the test out, is equivalent to pissing on the requirements. The test supposedly represents a piece of functional behaviour of the application, the fact that it's broken suggests a feature is also broken. The only reason to comment out a test is if that functionality has ceased to exist or the test was invalid in the first place. (Take a look at my checklist for unit test code reviews to help qualify invalid tests)

The second alternative to mark the tests as Ignored can be dangerous, depending on whether you're able to track why the test has been disabled and able to monitor when it enters this state.  This is one part developer process, one part testing framework, one part reporting.

Within NUnit, the Ignore attribute fits the bill nicely.  The attribute accepts a string message which is used to report why the test has been disabled. It should be standard developer process to only allow use of Ignore if a message is provided. (Come to think of it, I should put in a feature request so that it is required.)  When NUnit runs, the Ignored tests are not executed but the number of ignored tests are included in the xml output, meaning that your build server can track ignored tests over time. You can also bypass the Ignored attribute within the NUnit user-interface by simply selecting the Ignored test and running it manually.

[Ignore("The xml for this test isn't serializing correctly. Please fix!")]
[Test]
public void SuperImportantTest()
{
    // snip
}

Within MSTest however, Ignore is essentially the same as commenting out the tests.  There's no ability to record why the test has been excluded (developers must leave comments), and when MSTest runs the test suite the Ignored tests are simply excluded from the test run.  Looking at the MSTest output, the TRX file does not define a status for ignored, meaning that it can't be tracked in your build reports.

<ResultSummary outcome="Completed">
  <Counters 
    total="1" 
    executed="1" 
    passed="1" 
    error="0" 
    failed="0" 
    timeout="0" 
    aborted="0" 
    inconclusive="0" 
    passedButRunAborted="0" 
    notRunnable="0" 
    notExecuted="0" 
    disconnected="0" 
    warning="0" 
    completed="0" 
    inProgress="0" 
    pending="0" />
</ResultSummary>

Though I'm not a big fan of this, I'll spare you the complaining and move on to working solutions.

The best approach within MSTest isn't to disable the test but to indicate that it isn't reliable and needs attention. In addition, we need these tests to stand out without breaking the build. This is exactly what Inconclusive is for.

[TestMethod] 
public void SuperImportantTest() 
{ 
    Assert.Inconclusive("The xml for this test isn't serializing correctly. Please fix!"); 
    
    // snip 
}

The key to using Assert.Inconclusive is to put the statement as the first code-expression in your test. When MSTest encounters the Assert.Inconclusive statement, execution of the test is immediately halted, and the test is recorded as "Inconclusive". The TRX reports this status separately, and the message appears in the xml output.

Next Steps

With the tests marked as problematic, it’s possible to check-in the tests and unblock developers from a failed build while you and the original test author figure out how to fix the problem. It’s a temporary patch and is not intended for all breaking tests, just small fires that happen during critical developer crunches.  Once the fire has been put out and the tests corrected, you really should go back and investigate why the test broke in the first place:

  • Should the developer’s changes have broken this test?  If not, could the code/test have been written differently to be more durable?
  • What could the developer have done differently? Is this a one time occurrence or a disconnect in process?

That’s all for now.

submit to reddit

Monday, April 18, 2011

Testing Asynchronous Logic

I've been doing a fair bit of asynchronous logic recently where I'm designing objects to raise events after long-running operations have completed, and naturally this creates a few challenges during testing. When a method uses the asynchronous pattern, the work is executed on a separate thread and execution returns immediately to the caller. This is a problem for the a unit test as they are by design a series of standalone sequential steps -- the test may finish or execute assertions before the event is called.

The immediate workaround is to add delays into the tests using a Thread.Sleep statement.

[Test]
public void AsynchronousTestsDontAlwaysWork()
{
    bool eventCalled = false;
    
    var subject = new MyClass();
    subject.OnComplete += (o,e) => eventCalled = true;
    
    subject.DoWorkAsync();
    
    Thread.Sleep(100);
    
    Assert.AreTrue( eventCalled ); // can fail because the event might raise after this assertion
}

This strategy will work, but it's dangerous in the long-term because the duration of the asynchronous operation is dependent on the complexity of the implementation and the environment the test is executing in.  In other words, you may need to increase sleep interval if the implementation needs to do more work or if the tests fail on slower machines. In the end, the sleep interval reflects the lowest common denominator in your environment (that slow laptop that intern uses) and the entire test run becomes painfully (and unnecessarily) slow.

The solution is to add a little extra plumbing to block execution until the event is fired.

[Test]
public void AsynchronousTestWithBlockingAlwaysWorks()
{
    var reset = new System.Threading.ManualResetEvent(false);
    
    var subject = new MyClass();
    subject.OnComplete += (o,e) => reset.Set();
    
    subject.DoWorkAsync();
    
    if (!reset.WaitOne(1000)) // block for at most 1 second
    {
        Assert.Fail("The test timed out waiting for the OnComplete event.");
    }
}

The above example uses the ManualResetEvent, which in short is a signalling mechanism. The WaitOne method blocks execution until another thread calls the Set method or the timeout value is exceeded. In the above example, I've set up an anonymous delegate for my event so that the test finishes as soon as the asynchronous logic completes.

Cleaning it up...

As I mentioned earlier, I've been doing a lot of this type of testing so the above pattern has appeared in more than one test. It would be nice to encapsulate this a bit and streamline it into a reusable utility. This post on StackOverflow had an interesting concept to wrap the blocking logic into an object that supports IDisposable. The original poster was attempting to use Expressions to access events, which sadly isn't possible from outside the class, but here's an updated example that uses reflection and a named parameter for the event:

public sealed class EventWatcher : IDisposable
{
    private readonly object _target;
    private readonly EventInfo _eventInfo;
    private readonly Delegate _listener;
    private readonly ManualResetEvent _reset;
    private readonly int _timeout;

    public static EventWatcher Create<T>(T target, string eventName, int timeout = 1000)
    {
        EventInfo eventInfo = typeof (T).GetEvent(eventName);
        if (eventInfo == null)
            throw new ArgumentException("Event not found.", "eventName");

        return new EventWatcher(target, eventInfo, timeout);
    }

    private EventWatcher(object target, EventInfo eventInfo, int timeout)
    {
        _target = target;
        _eventInfo = eventInfo;
        _timeout = timeout;

        _reset = new ManualResetEvent(false);

        // Create our event listener
        _listener = CreateEventListenerDelegate(eventInfo.EventHandlerType);

        _eventInfo.AddEventHandler(target, _listener);
    }

    public void SetEventWasRaised()
    {
        _reset.Set();
    }

    private Delegate CreateEventListenerDelegate(Type eventType)
    {
        // Create the event listener's body, setting the 'eventWasRaised_' field.
        var setMethod = typeof(EventWatcher).GetMethod("SetEventWasRaised");
        var body = Expression.Call(Expression.Constant(this), setMethod);

        // Get the event delegate's parameters from its 'Invoke' method.
        MethodInfo invokeMethod = eventType.GetMethod("Invoke");
        var parameters = invokeMethod.GetParameters()
            .Select((p) => Expression.Parameter(p.ParameterType, p.Name));

        // Create the listener.
        var listener = Expression.Lambda(eventType, body, parameters);
        return listener.Compile();
    }

    public void Dispose()
    {
        bool eventWasFired = _reset.WaitOne(_timeout);
        // Remove the event listener.
        _eventInfo.RemoveEventHandler(_target,_listener);

        if (!eventWasFired)
        {
            string message = String.Format("Test timed-out waiting for event \"{0}\" to be raised.", _eventInfo.Name);
            Assert.Fail(message);
        }
    }
}

[Test]
public void DemonstrateEventWatcherWithUsingBlockAndDisposePattern()
{
    var subject = new MyClass();
    
    using(EventWatcher.Create(subject, "OnComplete"))
    {
        subject.DoWorkAsync();
        
    } // throws Assert.Fail() if event wasn't called.
}

Some Refinements...

This solution works, but there are a few drawbacks:

  1. The using block is a very succinct mechanism for controlling scope, but unfortunately if an exception occurs in the body of the using statement it will get swallowed by the exception in our EventWatcher's Dispose method.
  2. This only validates that the event was called, and it doesn't give us access to the event argument, which might be of interest to the test.
  3. The event name is a literal string value which isn't very refactor friendly.

Preventing Hidden Exceptions from the Finally block

To address the first problem where exceptions aren't properly handled inside the using statement, we change the signature slightly to perform the test logic in an Action.

// snip...
public static void WaitForEvent<T>(T target, string eventName, Action action) 
{ 
    var watcher = Create(target, eventName);

    try 
    { 
        action(); 
    } 
    catch (Exception) 
    { 
        watcher.Clear(); 
        throw; 
    } 
    finally 
    { 
        watcher.Dispose(); 
    } 
}
// ...end snip

[Test] 
public void DemonstrateEventWatcherWithAnActionInsteadOfUsingBlock() 
{ 
    EventWatcher.WaitForEvent(subject, "OnComplete", () => subject.DoWorkAsync()); 
}

Gaining Access to the EventArgs

Addressing the second problem requires some changes to the CreateEventListenerDelegate so that the arguments of the event are recorded.

//snip
public void SetEventWasRaised(EventArgs e) 
{ 
    EventArg = e; 
    _reset.Set(); 
}

public EventArgs EventArg { get; private set; }

private Delegate CreateEventListenerDelegate(Type eventType) 
{ 
    // Get the event delegate's parameters from its 'Invoke' method. 
    MethodInfo invokeMethod = eventType.GetMethod("Invoke"); 
    var parameters = invokeMethod.GetParameters() 
        .Select((p) => Expression.Parameter(p.ParameterType, p.Name)).ToList();

    // Setup the callback to pass in the EventArgs 
    var setMethod = typeof(EventWatcher).GetMethod("SetEventWasRaised"); 
    var body = Expression.Call(Expression.Constant(this), setMethod, parameters[1]);

    // Create the listener. 
    var listener = Expression.Lambda(eventType, body, parameters); 
    return listener.Compile(); 
} // end snip

[Test] 
public void DemonstrateEventWatcherCanCaptureEventArgs() 
{ 
    EventArgs e; 
    EventWatcher.WaitForEvent(subject, "OnComplete", out e, t => t.DoWorkAsync()); 
    
    Assert.IsNotNull(e); 
} 

Making it Refactor-Friendly...

Tackling the last problem where we don't have type-safety for our event, we need a special workaround because we're unable to use an Expression to capture our event. From a language perspective, C# will only allow external callers to add or remove event handler assignments, so we can't write a lamda expression that points to an event reference. Hopefully this is something that will be added to the language in a future version, but until then accessing the event requires some special hackery.

Taking a play from the Moq source code, we can infer the event name by using a dynamic proxy and intercepting the method invocation when we perform an event assignment. It's a long and awkward way around, but it works, albeit it comes with all the caveats of using a dynamic proxy: non-sealed classes with virtual events, MarshalByRefObject, or interfaces.

A note about virtual events: I’m personally not a big fan of virtual events (Resharper warnings, etc), but if you’re comfortable putting the event declarations on an interface, you can explicitly specify the interface in the generic type argument.  EventWatcher.Create<IMyInterface>(…)

public class EventAssignmentInterceptor : Castle.DynamicProxy.IInterceptor
{
    private Castle.DynamicProxy.IInvocation _lastInvocation;

    #region IInterceptor implementation

    public void Intercept(IInvocation invocation)
    {
        _lastInvocation = invocation;
    }

    #endregion

    public string GetEventName()
    {
        string methodName = _lastInvocation.Method.Name;
        return methodName.Replace("add_", "").Replace("remove_", "");
    }
}

public class EventHelper
{
    public static string GetEventName<T>(T target, Action<T> action) where T : class
    {
        var generator = new Castle.DynamicProxy.ProxyGenerator();
        var interceptor = new EventAssignmentInterceptor();

        T proxy;

        if (typeof(T).IsInterface)
        {
            proxy = generator.CreateInterfaceProxyWithoutTarget<T>(interceptor);
        }
        else
        {
            proxy = generator.CreateClassProxy<T>(interceptor);
        }

        action(proxy);

        return interceptor.GetEventName();
    }
}

Examples

Here’s a few examples that illustrate the various syntax.  Note, for sake a brevity (this post is crazy large) the overloads that define events as literals are not shown.

Basic usage in a using block. Note that exceptions inside the using may be swallowed by the dispose, and there's no access to the event args.

using(EventWatcher.Create(subject, t => t.OnComplete += null)) 
{ 
    subject.PerformWork(); 
} 

using(EventWatcher.Create(subject, t => t.OnComplete += null, timeout:1000) 
{ 
    subject.PerformWork(); 
}

Condensed usage. Provides access to the EventArgs and avoids the dispose problem.

EventArgs e = EventWatcher.Create(subject, t=> t.OnComplete +=null, () => subject.PerformWork() );

EventArgs e; 
EventWatcher.Create(subject, t=> t.OnComplete += null, out e, ()=> subject.PerformWork() ); 

Fluent usage. A cleaner syntax that declares the event and the action separately.

EventArgs e = EventWatcher 
            .For(subject, t => t.OnComplete += null) 
            .WaitOne(() => subject.PerformWork()) 

EventArgs e; 
EventWatcher.For(subject, t=> t.OnComplete += null) 
            .WaitOne( () => subject.PerformWork(), out e);

Full Source

The full source is here.

public sealed class EventWatcher : IDisposable
{
    private readonly object _target;
    private readonly EventInfo _eventInfo;
    private readonly Delegate _listener;
    private readonly ManualResetEvent _reset;
    private readonly int _timeout;

    #region Create
    public static EventWatcher Create<T>(T target, string eventName, int timeout = 1000)
    {
        EventInfo eventInfo = typeof(T).GetEvent(eventName);
        if (eventInfo == null)
            throw new ArgumentException("Event not found.", "eventName");

        return new EventWatcher(target, eventInfo, timeout);
    }

    public static EventWatcher Create<T>(T subject, Action<T> eventAssignment, int timeout = 1000) where T : class
    {
        string eventName = EventHelper.GetEventName(subject, eventAssignment);

        return Create(subject, eventName, timeout);
    } 
    #endregion

    #region For
    public static EventWatcherSetup<T> For<T>(T subject, Action<T> eventAssignment) where T : class
    {
        string eventName = EventHelper.GetEventName(subject, eventAssignment);
        return For(subject, eventName);
    }

    public static EventWatcherSetup<T> For<T>(T subject, string eventName)
    {
        return new EventWatcherSetup<T>(subject, eventName);
    } 
    #endregion

    #region WaitForEvent
    public static EventArgs WaitForEvent<T>(T target, Action<T> eventAssignment, Action action) where T : class
    {
        EventArgs e;
        WaitForEvent(target, eventAssignment, out e, action);
        return e;
    }

    public static void WaitForEvent<T>(T target, Action<T> eventAssignment, out EventArgs e, Action action) where T : class
    {
        string eventName = EventHelper.GetEventName(target, eventAssignment);
        WaitForEvent(target, eventName, out e, action);
    }

    public static EventArgs WaitForEvent<T>(T target, string eventName, Action action)
    {
        EventArgs e;
        WaitForEvent(target, eventName, out e, action);
        return e;
    }

    public static void WaitForEvent<T>(T target, string eventName, out EventArgs e, Action action)
    {
        var watcher = Create(target, eventName);

        try
        {
            action();
        }
        catch (Exception)
        {
            watcher.Clear();
            throw;
        }
        finally
        {
            e = watcher.EventArg;
            watcher.Dispose();
        }
    } 
    #endregion

    private EventWatcher(object target, EventInfo eventInfo, int timeout)
    {
        _target = target;
        _eventInfo = eventInfo;
        _timeout = timeout;

        _reset = new ManualResetEvent(false);

        // Create our event listener
        _listener = CreateEventListenerDelegate(eventInfo.EventHandlerType);

        _eventInfo.AddEventHandler(target, _listener);
    }

    public void SetEventWasRaised(EventArgs e)
    {
        EventArg = e;
        _reset.Set();
    }

    public void Clear()
    {
        _reset.Set();
    }

    public EventArgs EventArg { get; private set; }

    private Delegate CreateEventListenerDelegate(Type eventType)
    {
        // Get the event delegate's parameters from its 'Invoke' method.
        MethodInfo invokeMethod = eventType.GetMethod("Invoke");
        var parameters = invokeMethod.GetParameters()
            .Select((p) => Expression.Parameter(p.ParameterType, p.Name)).ToList();

        var setMethod = typeof(EventWatcher).GetMethod("SetEventWasRaised");
        var body = Expression.Call(Expression.Constant(this), setMethod, parameters[1]);

        // Create the listener.
        var listener = Expression.Lambda(eventType, body, parameters);
        return listener.Compile();
    }

    public void Dispose()
    {
        bool eventWasFired = _reset.WaitOne(_timeout);
        // Remove the event listener.
        _eventInfo.RemoveEventHandler(_target,_listener);

        if (!eventWasFired)
        {
            string message = String.Format("Test timed-out waiting for event \"{0}\" to be raised.", _eventInfo.Name);
            Assert.Fail(message);
        }
    }
}

public class EventWatcherSetup<T>
{
    private readonly T _target;
    private readonly string _eventName;

    public EventWatcherSetup(T target, string eventName)
    {
        _target = target;
        _eventName = eventName;
    }

    public EventArgs WaitOne(Action action)
    {
        EventArgs e;

        WaitOne(action, out e);
        
        return e;
    }
    public void WaitOne(Action action, out EventArgs e)
    {
        EventWatcher.WaitForEvent(_target,_eventName, out e, action);
    }

}

public class EventAssignmentInterceptor : Castle.DynamicProxy.IInterceptor
{
    public Castle.DynamicProxy.IInvocation LastInvocation;

    #region Implementation of IInterceptor

    public void Intercept(IInvocation invocation)
    {
        LastInvocation = invocation;
    }

    #endregion

    public string GetEventName()
    {
        string methodName = LastInvocation.Method.Name;
        return methodName.Replace("add_", "").Replace("remove_", "");
    }
}

public class EventHelper
{
    public static string GetEventName<T>(T target, Action<T> action) where T : class
    {
        var generator = new Castle.DynamicProxy.ProxyGenerator();
        var interceptor = new EventAssignmentInterceptor();

        T proxy;

        if (typeof(T).IsInterface)
        {
            proxy = generator.CreateInterfaceProxyWithoutTarget<T>(interceptor);
        }
        else
        {
            proxy = generator.CreateClassProxy<T>(interceptor);
        }

        action(proxy);

        return interceptor.GetEventName();
    }
}

submit to reddit

Friday, March 18, 2011

Parsing Nullable Enumerations

I had an interesting challenge yesterday that involved converting a string to an enumerated value. This seems like it would be really trivial, but I had a few extra considerations that made it harder to deal with.

We're all familiar with the Enum.Parse method that has existed in the .NET Framework since version 1.1.  It's a somewhat clunky and verbose method:

MyEnum result = (MyEnum)Enum.Parse(typeof(MyEnum), "First");

Recently, the .NET Framework 4.0 introduced a new static method on the Enum type which uses generics.  It's much cleaner, if you don't mind the out parameters.

MyEnum result;

if (Enum.TryParse("First", out result))
{
    // do something with 'result'
}

Unfortunately, the above methods are constrained to work with struct value-types only and won’t work with Nullable types. After some experimenting and fussing with casting between generic types I managed to get a solution that works with both standard and nullable enumerations.

I’m sure there’s a bit extra boxing in here, so as always your feedback is welcome.  Otherwise, this free-as-in-beer extension method is going on my tool belt.

public static TResult ConvertToEnum<TResult>(this string source)
{
    // we can't get our values out of a Nullable so we need
    // to get the underlying type
    Type enumType = GetUnderlyingTypeIfNullable(typeof(TResult));
        
    // unfortunatetly, .net 4.0 doesn't have constraints for Enum
    if (!enumType.IsEnum)
        throw new NotSupportedException("Only enums can be converted here, chum.");

    if (!String.IsNullOrEmpty(source))
    {        
        object rawValue = GetRawEnumValueFromString(enumType, source);
        
        // if there was a match
        if (rawValue != null)
        {
            // having the value isn't enough, we need to
            // convert this back to an enum so that we can 
            // perform an implicit cast back to the generic type
            object enumValue = Enum.Parse(enumType, rawValue.ToString());
        
            // implicit cast back to generic type
            // if the generic type was nullable, the cast
            // from non-nullable to nullable is also implicit
            return (TResult)enumValue;
        }
    }
    
    // if no original value, or no match use the default value.
    // returns 0 for enum, null for nullable<enum>
    return default(TResult);
}

private static Type GetUnderlyingTypeIfNullable(Type type)
{
    if (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>))
    {
        // Nullable<T> only has one type argument
        return type.GetGenericArguments().First();
    }
    
    return targetType;
}

private static object GetRawEnumValueFromString(Type enumType, string source)
{
    FieldInfo[] fields = enumType.GetFields(BindingFlags.Public | BindingFlags.Static);

    // attempt to find our string value
    foreach (FieldInfo field in fields)
    {
        object value =  field.GetRawConstantValue();

        // exact match
        if (field.Name == source)
        {
            return value;
        }

        // attempt to locate in attributes
        var attribs = field.GetCustomAttributes(typeof (XmlEnumAttribute), false);
        {
            if (attribs.Cast<XmlEnumAttribute>().Any(attrib => source == attrib.Name))
            {
                return value;
            }
        }
    }

    return null;
}

Heh: “Free as in Beer”. Happy St. Patty’s, everybody.

submit to reddit

Friday, March 04, 2011

Add a Custom Toolbar for Source Control

My current project uses TFS and I spent a lot of time in and out of source control, switching between workspaces to manage defects. I found myself needing access to my workspaces and getting really frustrated with how clunky this operation is within Visual Studio.  There are two ways you can open source control.

  1. The Source Control item in the Team System tool window.  I don’t always have the Team Explorer tool window open and when I open it, it takes a few seconds to get details from the server.
  2. View –> Other Windows –> Source Control Explorer.  Useful, but there’s too much mouse movement and clicking to be accessible.

So, rather than creating a custom keyboard shortcut that I would forget I added a toolbar that is always in plain-sight. It’s so convenient that I take it for granted, and when I pair with others they comment on it. So for their convenience (and yours), here’s how it’s done.

Add a new Toolbar

From the Menubar, select “Tools –> Customize”.  It’ll pop up this dialog. 

Click on “New” and give your toolbar a name.

Toolbar_Customize_AddToolbar

Add the Commands

Switch to the Commands tab and select the name of your toolbar in the Toolbar dropdown.

Toolbar_Customize_Empty

Click on the “Add Command” button and select the following commands:

  • View : TfsSourceControlExplorer
  • View : TfsPendingChanges

Toolbar_Customize_AddCommand

Now style the button’s accordingly using the “Modify Selection” button.  I’ve set mine to use the Default styling, which is just the button icon.

Enjoy

CustomToolbar_result