Unit test styles

[James Gregory]

I don't know how many people actively read this list, but I thought I'd put this out here for anyone to comment on.

I'm pretty embarrassed about the state of unit tests in docu. Don't get me wrong, coverage is pretty good, it's the style that I'm at odds with. I'd like to decide on a style and redo the other tests to be consistent. Before I get to the examples and criticism, I'll start by saying these are not BDD style specs, they're traditional fixture-per-class style. I've not written any real BDD tests before, but I'd be open to the idea if anyone out there is willing to give me a guiding hand. Anyway, onto the examples of the various styles:

DocumentModelGeneratorTests/Remarks.cs: 

[Test]

public void ShouldHaveRemarksForType()

{

  var model = new DocumentModel(new CommentParser(), StubEventAggregator);

  var members = new[]

  {

    Type<First>(@"<member name=""T:Example.First""><remarks>First remark</remarks></member>"),

  };

  var namespaces = model.Create(members);

  var comment = new List<IComment>(namespaces[0].Types[0].Remarks.Children);

  comment.Count.ShouldEqual(1);

  ((InlineText)comment[0]).Text.ShouldEqual("First remark");

}

Not too bad, not too long, name is almost ok but still not really clear "document model generator [when parsing] remarks should have remarks for type". Mainly the body of the test is almost completely unreadable on first glance.

DocumentationGeneratorTests/Generate.cs: 

[Test]

public void should_move_untransformable_resources_to_output_dir()

{

  var resourceManager = MockRepository.GenerateMock<IUntransformableResourceManager>();

  var generator = new DocumentationGenerator(StubAssemblyLoader, StubXmlLoader, StubParser, StubWriter, resourceManager, StubEventAggregator);

  generator.SetAssemblies(new[] { "unimportant_file_path" });

  generator.SetOutputPath("output-dir");

  generator.Generate();

  resourceManager.AssertWasCalled(x => x.MoveResources(null, null),

                                  x => x.Constraints(Is.Anything(), Is.Equal("output-dir")));

}

DocumentationGenerator has too many dependencies, which makes the stub stuff a bit nasty. Other than that, if you understand how mocks work then it's pretty readable. The naming is still just ok but requires some expansion on the readers part, "documentation generator [when] generate [called] should move untransformable resources to output dir"; really I think it's the class name that lets down the name in this case.

Generation/HtmlGenerationTests.cs: 

[TestTemplate("${Namespace.Name}")]

public void ShouldOutputShortcutNamespace()

{

  Convert(new ViewData { Namespace = Namespace("Example") })

    .ShouldEqual("Example");

}

Nice and short, name not so great again. Relies on quite a bit of magic to parse the special attribute, and the Convert method that wraps the Spark code; don't know if magic is good or bad, but I like the brevity.

Generation/TemplateTransformerTests.cs: 

[Test]

public void GeneratesOutputForEachTypeFromTemplateWhenPatternUsed()

{

  var generator = MockRepository.GenerateMock<IOutputGenerator>();

  var writer = MockRepository.GenerateStub<IOutputWriter>();

  var resolver = MockRepository.GenerateStub<IPatternTemplateResolver>();

  var transformer = new PageWriter(generator, writer, resolver);

  var namespaces = Namespaces("One", "Two");

  Type<First>(namespaces[0]);

  Type<Second>(namespaces[1]);

  resolver.Stub(x => x.Resolve(null, null))

    .IgnoreArguments()

    .Return(new List<TemplateMatch>

    {

       new TemplateMatch("One.First.htm", "!type.spark", new ViewData()),

       new TemplateMatch("Two.Second.htm", "!type.spark", new ViewData())

    });

  transformer.CreatePages("!type.spark", "", namespaces);

  generator.AssertWasCalled(

    x => x.Convert(null, null),

    x => x.Constraints(Is.Equal("!type.spark"), Is.Anything())

    .Repeat.Twice());

}

Wow, just wow :) Too many dependencies again. Also, not so great that in a fixture-per-class design the fixture name doesn't reflect the class under test.

PatternTemplateResolverTests/single_template.cs: 

[Test]

public void should_have_no_specific_type()

{

  resolve("template.spark");

  type_of(first_result).ShouldBeNull();

}

Name is pretty good this time, "single template should have no specific type". The body is nice and simple again, but relies on a lot of magic; again, not sure if this is good or bad. It results in simple tests, and means there's only one point that breaks for interface changes, but it does obscure what is actually being tested.

UI/HtmlOutputFormatterTests.cs: 

[Test]

public void OutputsEventReferenceLink()

{

  var formatter = new HtmlOutputFormatter();

  var method = Event<Second>("AnEvent");

  formatter.FormatReferencable(method)

    .ShouldEqual("<a href=\"Example/Second.htm#AnEvent\">AnEvent</a>");

}

Again, name is just ok, body is pretty readable too; relies on a bit of magic, but not much.

I think that'll do for the examples. Anybody care to offer some opinions? Bash away. Should I go with a BDD style? Should I lose the magic, or is it good? Are any of the examples better than others, or are they all bad in different ways?

[Chris Missal]

I was playing around in there last night actually and I was able to pick it up pretty quick as to what the tests are doing (tests like the first example you provided). I think as long as the tests reveal their intent, are atomic, fast, and order independent, it's all good. The only other thing is that the format consistency you mentioned.

The one problem I have with PatternTemplateResolverTests/single_template.cs:  is that it looks a bit harder to see what's going on if I wanted to contribute. Whereas, in the first test, it's pretty straight forward what is being tested.

--
Chris Missal
http://chrismissal.lostechies.com/

[Justin Bozonier]

Hey James, 

I personally am a bdd guy but I agree with chris that your tests aren't as bad ad you think. I actually learned a good trick from your tests to help me in my bdd.

Having said that I'd be happy to show you how I'm managing my mocks and dependencies in a clean and readable way in Alloy. When I get to work I'll send you a link to one of my test classes on github. 

The main difference is that I take my mock/stubs and centralize them as extension methods. All of these I call assumptions and he language works beautifully IMHO. Ex. Foo.assume_will_receive_a_message() etc.

Anyway I'll send you a link when I get in. Maybe you'll be able to glean something off it.

Sent from my iPhone

[Justin Bozonier]

Here is the sample I wanted to show you. I am in the process of
refactoring my tests and this is the model upon which I am basing
those refactorings (If you keep in mind that Alloy is a Twitter and
GTalk messaging client the tests *should* make perfect sense if I've
done an adequate job, feedback is of course welcome!):

http://github.com/jcbozonier/alloy/blob/99e43b8d4025560c72a5fed96d4cc8014fca512d/IronTwit/Alloy/Alloy.Specs/sending_message_specs/Sending_a_message_in_general.cs

On May 8, 6:43 am, Justin Bozonier <darkxant...@gmail.com> wrote:
> Hey James,
>
> I personally am a bdd guy but I agree with chris that your tests  
> aren't as bad ad you think. I actually learned a good trick from your  
> tests to help me in my bdd.
>
> Having said that I'd be happy to show you how I'm managing my mocks  
> and dependencies in a clean and readable way in Alloy. When I get to  
> work I'll send you a link to one of my test classes on github.
>
> The main difference is that I take my mock/stubs and centralize them  
> as extension methods. All of these I call assumptions and he language  
> works beautifully IMHO. Ex. Foo.assume_will_receive_a_message() etc.
>
> Anyway I'll send you a link when I get in. Maybe you'll be able to  
> glean something off it.
>
> Sent from my iPhone
>

> On May 8, 2009, at 1:37 AM, James Gregory <jagregory....@gmail.com>  

[Josh Flanagan]

An AutoMocker would also help with cleaning up your test setup for classes which have a lot of dependencies. Of course, its probably re-examining the classes first to make sure they aren't doing too much and might benefit from being broken out into separate classes.

StructureMap comes with an AutoMocker for Rhino.Mocks if you want to go that route.

I have a quick intro here:

http://www.lostechies.com/blogs/joshuaflanagan/archive/2009/02/03/auto-mocking-explained.aspx

As far as base classes or fixture per context, I think that makes sense when you have a lot (maybe just more than one) of assertions for a given context. However, when you have a situation where you want to test a single result with a bunch of different inputs (context), setting up the base class or fixture per context actually ends up obscuring the test. Thing of the parsing tests - here's the input, check the expected output. I think its much more readable to be able to see the input and matching output all within the same method.