Mark’s Testblog

I was thinking about this the other day, and something struck me (and no, it wasn’t a disgruntled developer).  Automated testing is a valuable and widely accepted part of the software engineering process.  Many companies and organisations require that functionality be well tested prior to it being checked in and integrated into the main version control branch.  As a result, developers can spend hour upon hour every week writing unit tests.  In many cases, the quantity of test code can be more than double the code being tested!

If you give any programmer a problem to solve, their brains will start whirring and they will come up with numerous solutions.  They will draw upon past experiences, new technologies and articles/books they’ve read.  They will use specific parts of the language to elegantly solve the problem in a terse and expressive fashion.  When they come up with a really cool solution, others will marvel at it and learn from it.

In short, most programmers will think critically about problems before, during and after solving them.

The indifference of a unit tester

So, why is it that some folk just stop thinking when they put on their unit testing hat?  I fully understand that testing is not as glamorous or fun as banging out production code, but that’s no reason to accept sub-par test code.

Given that unit testing is part of the software development cycle, I personally think that anyone who phones in their test code is doing themselves a disservice.  If you do the minimum possible amount of work, do not think critically about what you’re doing and ultimately learn nothing from it, then that’s a lot of time that is thrown away every week.

It also doesn’t say much about you as a developer if you phone it in.  “Pff, functioning software, who needs that?”

Test code is still code.  You have to write a lot of it.  It is fundamentally entwined with the production code in that it is vital to proving the solution works.  Moreover, if you test in a brain-dead fashion and fail to draw upon any of the faculties used when writing production code, you are often making more work for yourself later on, too. I usually cringe at the phrase “work smarter, not harder“, but in this case it is often applicable.

Testing informs design

Firstly, the most obvious point is that, if it’s hard to test something due to it being at the mercy of the environment, static/global state, hard wired dependencies and such, then it’s probably a sign that your code needs refactored.  If you want to test a loop exit condition and end up having to connect to a real database to do so, you know you’ve got problems.

If you simply shrug your shoulders and plough on, you’ll probably run into all kinds of obstacles.  For example, you may have to initialise the environment to be just right for your test, then carefully negate the results of the test in the teardown.  But then what if the test order matters?  How will you know?  I’ve been in this situation before and it’s not pleasant.  It was such a battle getting the environment configured correctly for each test that I lost the will to live.  Plus the tests ran incredibly slowly.

One of the major benefits of unit testing is that it encourage you to split up functionality into discrete units.  It just so happens that the discrete units are more easily understood as a result.  I liken it to building blocks.  Instead of having a monolithic structure (or, as less kind folk would call it, a big ball of mud), you have a load of little blocks.  Not only are the blocks much easier to understand on their own, but they are much nicer to work with.  I won’t bang on about this much because it’s nothing new (see Test Driven Development).

I don’t always practice TDD, but at the very least I try to test classes/methods not long after I’ve written them.  Putting it off any longer tends to result in having to write a lot of test code at once, which sucks.  If you don’t like writing test code and you save up the tests for later, it’s like putting off doing your school homework.  It hangs over you like a cloud… and then you have to do it all at once on a Sunday night.  Why punish yourself? :)

Unit testing is programming, too

Naively attacking a testing problem may result in generating reams of test code that proves very little or, worse, has negative value.

When writing tests, I am constantly looking for the following warning signs:

• You have to configure the environment / concrete collaborators in a fashion that you barely understand yourself (how is anyone else meant to deal with this?)
• You can’t easily test certain conditions or logical branches due to tight coupling (e.g. trying to test the behaviour of a class when one of its collaborators throws an exception that could occur in production systems, such as a network connection exception)
• The test code is disproportionately large compared to the class under test due to repetition.  The DRY principle isn’t just for production code (though tests should favour readability if push comes to shove).
• The test code is convoluted.  It is not clear what is going on, the test names are poor and it doesn’t help you understand the class under test.
• The tests are brittle, causing them to frequently break, and break badly (e.g. a change in a concrete collaborator breaks tests that shouldn’t really depend on it, resulting in a debugging session rather than a simple fix…)
• The test code could be refactored to use patterns to cut down on the noise (see previous Test Data Builder post).
• The tests do not seem to prove anything.  If a test calls some methods with trivial cases and does very little verification, then what’s the point in writing the test?

If I encounter one or more of these signs, I try to think about whether the class under test or the tests themselves would benefit from refactoring.

Pointless Tests

The last point is in the list particularly salient.  Why write a test if it doesn’t prove anything? I find this to be the most insidious problem of all.

In general development, no self respecting programmer would repeatedly write code that:

• Has no purpose
• Looks like it’s doing the job correctly, but actually isn’t
• Sets a bad example to anyone else reading the code
• Sets good practices to one side (e.g. loads of repetition, unrepresentative method names)

Programmers should hold themselves to the same standards when writing tests.  Bad testing is arguably worse than no testing at all.

Here’s an example of a pointless test:

    [Test]
    public void SaveGameTest()
    {
        var game = new Game();
        var serializer = new GamePersistence();

        using(Stream stream = new MemoryStream())
        {
            serializer.Save(game, stream);
        }

        Assert.IsTrue(true);
    }

What does this prove?  Nothing.  At the very most, it proves that you won’t encounter an exception when saving a game in the most trivial of cases.

What does it do that is harmful?  Lots.  It took time to write.  It takes up space in the solution explorer / code file.  It gives impression that functionality has been tested in some capacity.  It skirts over areas of concern.  It sets a bad example to other developers who may look to existing tests for an example of testing this sort of thing.

Finally, anyone who writes test code like this will probably find that it makes them hate testing even more than before, because they got absolutely nothing out of it.  Bad tests rarely find any bugs because they’re not asking the right questions, or any questions at all.  If no bugs are found, then the testing pass feels like a waste of time, further reinforcing the dislike for testing.

In Summary

Testing is part of the software development process. It has its own language, methodologies, patterns, best practices etc.

If you apply the same critical thought processes and rigour to testing that you apply to production code, you will write better software with fewer bugs.  If you phone it in, you’ll receive a return call down the line…

Update

Since writing this, I’ve come to prefer the original  method chaining style.  While the property initialiser style works very well for simple builders, the original method chaining style works a bit better if you have nested builders.  See the link in this post for the original; it’s more versatile.

The problem

If you write automated tests, then you are bound to have come across a situation where, during the course of testing your classes, you have to configure them in different ways.  This difference may be slight (say, varying one argument passed into a constructor every time), but it is enough to make instantiating the objects a pain.  You can’t use a common setup method to make life easier, so that leaves you with two obvious options:

1. Instantiate the objects via a direct constructor call
2. Abstract the creation a little and use multiple factory methods

Drawbacks

The first option is verbose, cumbersome and brittle.  Every time the constructor changes or its rules change (e.g. “parameter string houseName must not contain spaces”), you’ll need to update the tests to use the new form.  If you have scores of calls, writing and maintaining the tests can consume a lot of time, plus it’s tedious.

The second option may seem appealing, but it also has shortcomings.  This is the so called “Object Mother” pattern.  You basically have some factory methods (or a test class) that returns objects in various configurations.  When you have method names like “CreateHouseWithKitchenSinkAndStairs”, “CreateHouseWIthKitchenSinkAndRoof” and “Create HouseWithRoof” (or equivalent overloaded methods) then you’ll recognise this.

Depending on how much method re-use you can share between tests, this may also be brittle and a lot of effort to maintain.  Both ways of doing things also lack clarity when lots of arguments are being passed around — the intent of the test (read: the interesting argument[s]) can be hidden by the noise of setting up safe defaults.  E.g. If you have 10 ‘normal’ objects and one badly formatted string that you’re using to make sure that an exception occurs, you want to focus on the argument that will cause the exception, not other parts.

Test Data Builders

So… what is this mythical Test Data Builder pattern?  It’s actually quite simple, though the syntax can look strange if you haven’t come across fluent interfaces before.

A Test Data Builder is an extension of a standard factory method.  The difference between a standard factory and a builder is the way that the builder wires things up.  A factory typically has a “Create” call which creates an instance of an object (or objects) in pre-defined configurations, or with some customisability provided via arguments in the Create call, or injected into the factory’s constructor.

A TDB varies this approach by setting up default, safe values to be used to instantiate the class under test.  When the builder’s constructor runs, it creates these defaults and stores them as fields.  There is usually a one to one correspondence of the class under test’s parameter list and the values stored in the builder.

The cool part is that anyone using the TDB can selectively modify these values by calling methods or setting properties on the TDB.  Finally, when the TDB has been configured satisfactorily for the test, its Build method is called, injecting its fields into the class under test’s constructor as arguments.

Nat Pryce posted an excellent article on Test Data Builders so I would implore you to read it before reading the rest of this post.

Terser is more gooder – C# 3.0

Once I’d got my head around the strange fluent interface syntax, I found that example to be invaluable.  It got me thinking: what if we could write these builders in an even more concise fashion?  Well, we can.  C# 3.0 can go one step further and, through the use of object initialisers, eliminate the need for method chaining.  This results in terser syntax still.

It’s simple: Instead of using a method per value change, we use a property instead.  Since object initialisers are run immediately after a constructor and are nicely grouped, it means we can create and configure our builder in one step.

The following code is provided merely to illustrate the syntax (there’s not much point in using a TDB on a class with so few parameters).  Here’s an overly simple example of a Person class to test.

    /// <summary>The class under test</summary>
    public class Person
    {
        private readonly string     m_name;
        private readonly DateTime   m_dateOfBirth;
        private readonly string     m_address;

        public Person(string _name, DateTime _dateOfBirth, string _address)
        {
            ValidateDateOfBirth(_dateOfBirth);
            ValidatePostCode(_address);        // etc.
            m_name          = _name;
            m_dateOfBirth   = _dateOfBirth;
            m_address       = _address;
        }
    }

Here’s the builder.  Notice that it has 3 auto properties that match the Person class’s constructor parameter names and types.  Default values are instantiated in the constructor, and the properties allow test writers to swap out the defaults for their own specific values.

    /// <summary>Test data builder for testing the Person class</summary>
    public class PersonTestBuilder
    {
        public string   Name        { get; set; }
        public DateTime DateOfBirth { get; set; }
        public string   Address     { get; set; }

        public PersonTestBuilder()
        {
            Name        = "DefaultName";
            Address     = "DefaultAddress";
            DateOfBirth = new DateTime(1920, 8, 15);
        }

        public Person Build()
        {
            return new Person(Name, Address, DateOfBirth);
        }
    }

Finally, here’s the test code itself.  Notice that the only arguments that are varied are the ones that are named in the test.  If the builder’s property is not set, the defaults held in the builder are used to construct the instance of the class under test.

    /// <summary>The actual tests for the person class</summary>
    [TestFixture]
    public class PersonTests
    {
        [Test]
        [ExpectedException(typeof(RidiculousAgeException), ExpectedMessage = "... etc")]
        public void Constructor_DateOfBirthInvalid_ThrowsExceptionTest()
        {
            new PersonTestBuilder
                {
                    // Notice that this is now the sole focus!
                    DateOfBirth = new DateTime(1200, 10, 15)
                }.Build();
        }

        [Test]
        public void Constructor_AddressTest()
        {
            Person testPerson = new PersonTestBuilder
            {
                Address = "10 Bigglesworth Lane",
            }.Build();

            Assert.That(testPerson.Address,
                Is.EqualTo("10 Bigglesworth Lane"));
        }
    }

If any of this is unclear, just paste the code into your IDE and step through it in your debugger to see what’s happening (you’ll have to remove the non existent methods & exception test, obviously!)

Summary

I’ve found the following advantages when using this pattern in tests with large amounts of setup and little commonality in object construction:

1. Less fragile.  Since object creation is centralised and uses safe values by default, tests are more resistant to change compared to direct constructor calls.
2. Terser test code (extremely important when setup is complicated)
3. The intent of the test is clearer.  There is less noise in test code, so you can more easily pick out the arguments/interactions of interest.

If you ever have to test a class that uses a WeakReference, or even just have to use Weak References, be very careful.  Numerous strange-looking things can occur when Weak References are involved.

If you have even a cursory understanding of the .NET Garbage Collector (GC), you will know that it keeps track of objects.  When an object is no longer strongly referenced, the GC will potentially collect it, freeing up its resources.  This causes the object to ‘disappear’. So, if you have a strong reference to an object in your program, you are generally safe in the assumption that the object will stick around.  The GC won’t pull the carpet from under you while you’re using that object.

Weak References, on the other hand, do not stop the GC from collecting the object they refer to.  In certain circumstances, it can be advantageous to use Weak References because you do want to use/observe/whatever an object, but you don’t want to stop it from being collected.  So far so good.  All obvious stuff.

The GC moves in mysterious ways

OK you say, what’s the point in this article?  The point is that GC is extremely clever.  Almost a little too clever.  So clever it may aggressively collect objects to the point where it can mess with your head, and your tests.  This ‘problem’ can manifest itself in subtle ways — certain types of tests involving Weak References will usually pass in debug mode, but may sporadically fail in release mode.  Heads will be scratched.  Bemused gurning will commence.

Obligatory Contrived Example

public class GuyWhoUsesWeakRef
{
    private WeakReference m_weakBuilder;

    public GuyWhoUsesWeakRef(StringBuilder _builder)
    {
        m_weakBuilder = new WeakReference(_builder);
    }

    public bool IsWeakRefValid { get { return m_weakBuilder.Target != null; } }
}

[Test]
public void IsWeakRefValid_WhenValidRef_ReturnsTrue_Test()
{
    var builder     = new StringBuilder();
    var usesWeakRef = new GuyWhoUsesWeakRef(builder);

    Assert.That(usesWeakRef.IsWeakRefValid, Is.True,
        "Operation relying on the weak reference should've succeeded");
}

Why does this occasionally fail?

If you look carefully, you may spot the problem.  Recall that I said that objects can be collected while still in scope.  After the builder reference has been passed to the GuyWhoUsesWeakRef constructor, it is no longer used anywhere.  The GuyWhoUsesWeakRef class doesn’t take a strong reference, so the moment the parameter is no longer used, that reference also gets discarded.

As a result, immediately after the new GuyWhoUsesWeakRef(builder) call, the GC figures out that the StringBuilder object we’ve created will never be used again.  After all, if the object is never used again, why not collect it as soon as possible?

In debug mode, this won’t throw a spanner in the works.  The test will pass because the GC is not aggressively collecting.  However, in release mode, the GC may well collect the StringBuilder when we fully expect it to still be alive for our Assert.That() call.

The main problem is that it won’t happen every time.  The GC is non-deterministic, so this test will pass and fail intermittently; it depends on the timing of the collection.  Coming from C++ where objects are destroyed as they exit scope, I found this somewhat bemusing.  In the context of the GC, it makes sense, though.  You just have to be careful.

The solution

The good folks at Microsoft they provided a very simple static method call to solve this particular problem; enter GC.KeepAlive.  Placing a call to GC.KeepAlive(builder) at the end of this test method will ensure that the object we’re referring to will not be collected until after the GC.KeepAlive call has been made.  Problem solved.

[Test]
public void IsWeakRefValid_WhenValidRef_ReturnsTrue_Test()
{
    var builder     = new StringBuilder();
    var usesWeakRef = new GuyWhoUsesWeakRef(builder);

    Assert.That(usesWeakRef.IsWeakRefValid, Is.True,
        "Operation relying on the weak reference should've succeeded");

    GC.KeepAlive(builder); // weak ref is now valid up to this point
}

Forget fancy websites, PR and advertising

When creating Fortress Forever, we generally depended on TFC players who had modding skills to get us up and running.  As a result, the moment other TF mods started threatening to spring up, we felt it was a good time to put up a website to ensure that we were visible to anyone interested in working on a Source TF mod.  Since we had a website set up, we added all of the usual content to it, got some forums and started doing interviews and media releases etc.

In hindsight, I feel that running the website, maintaining forums and dealing with the community & modding sites at such an early stage was a big error.  The feedback on the forums did give us some genuinely good ideas, but in general it was much more hassle than it was worth.  Let’s think rationally about this for a second: We were talking to people about something we had not yet built.  People were offering feedback on things that they had not yet played.  For every salient point, there was probably 100 useless posts containing noise, tattle and flames (i.e. just a normal forum).  This goes back to my previous post about design: You can talk it to death.  It’s much more useful to actually do it and then iterate.   If you spend hours a week promising the moon on a stick or justifying choices you have yet to implement, you could spend those hours doing something tangible.

After a year or so, FF’s forums had thousands of users.  Trolls, elitists and cliques build up over the years, especially if people are discussing .. nothing (or how long it has been between media releases etc.)  There were even a few hack attempts.  The point is that all of this stuff takes time to set up and maintain.  It didn’t help us get much closer to our goal of shipping the mod.

I should also mention that the FF forums also had thousands of normal, nice and helpful users :)

Another good reason for maintaining a low profile

If your mod goes public and you make a big song and dance about it, it makes you a much more appealing target for pissed off ex-members of the mod or hackers in general.  If it’s a small project and nobody has heard of it, there’s limited value in releasing source code or bitching to the masses about the mod.  However, it’s viewed as a ‘big’ mod with high coverage on community sites, you’re a much more satisfying target for a disgruntled modder.

As far as I know we weren’t ever targeted by an ex-member, but I wouldn’t have been surprised if someone had thought about it.  There’s always the risk of fallings out and friction between folk, that’s life. Some people take it particularly badly and will try to get back at you by trying to trash the mod in any way possible.

We did however get ‘hacked’ at some point.  The ‘hacker’ acquired a build of the mod from our SVN repository, but it was a beta version that had reams of the server code removed (so it would just crash if you tried to run a server).  As a result it could only connect to our official beta servers, which were themselves protected via SteamID checks.  The ‘hacker’ threatened to reverse engineer and leak the build, but ultimately failed because our precautions were good enough.  If we had quietly gone about our business, I doubt the person would’ve gone to such lengths to try and fuck with us.  I believe that Black Mesa suffered the ignominy of having their hard work leaked a few years back, so it is definitely something to be aware of.

If you insist…

If you are in a situation where you definitely do need a website of some sort (typically to attract modders to work on your project), just keep it simple.  Post an outline of what your mod is all about, a description of the modders working on it, the skills you are interested in and also some images of the work done so far (if available).

If people show an interest in what you’re doing, then you can always periodically show off some cool stuff, just try not to make a big deal out of such things until you’re actually approaching some sort of release.  Again, for every mod that makes it, there’s probably 20 that fold without shipping anything.  If you have something to release, start to think about advertising it.  Until then, forget it!