TDD: How to supersede a single system library call

This morning I read an article this morning by Karl Seguin on allowing clients to replace system calls with delegates (function pointers) for testing purposes — making the untestable testable.

It is a pattern I have used myself recently, but under a different name with a more formalized syntax. Imagine, for example, you have created an interface to decouple e-mail sending dependencies:

public interface IMailSender
{
    void Send(MailMessage message);
}

This allows me to swap out my IMailSender (via Dependency Injection) with a fake implementation for testing purposes – e.g. FakeMailSender or a mock.

Otherwise, production code uses SmtpMailSender, a concrete class I wrote that implements IMailSender via a call to System.Net.Mail.SmtpClient.Send():

public class SmtpMailSender : IMailSender
{
    // The MailSent event is raised every time an email is sent.
    public event EventHandler<MailSentEventArgs> MailSent;

    public void Send(MailMessage message)
    {
        if (message == null)
            throw new ArgumentNullException("message");

        // Send the message using System.Net.Mail.SmtpClient()
        new SmtpClient().Send(message);

        // Notify observers that we just sent a msg.
        MailSent(new MailSentEventArgs(message));
    }
}

Note this class is a little bit special — it also has an event MailSent that gets raised every time a message is sent. This lets me attach stuff like global e-mail logging very easily, but how can we write a unit test that asserts this event gets raised at the correct time? Because of the dependency on SmtpClient.Send(), my SmtpMailClient class is now facing the exact same problem IEmailSender was designed to solve.

Injecting an Action in the constructor

System.Net.Mail.SmtpClient.Send() isn’t virtual, so we can’t mock it, and I don’t really want another interface just for this situation. One solution Karl suggests is injecting an Action that does the actual sending:

public class SmtpMailSender : IMailSender
{
    // The MailSent event is raised every time an email is sent.
    public event EventHandler<MailSentEventArgs> MailSent;

    // method that actually sends the message.
    readonly Action<MailMessage> send;

    public SmtpMailSender(Action<MailMessage> send)
    {
        this.send = send;
    }

    public void Send(MailMessage message)
    {
        if (message == null)
            throw new ArgumentNullException("message");

        // Send the message using System.Net.Mail.SmtpClient()
        send(message);

        // Notify observers that we just sent a msg.
        MailSent(new MailSentEventArgs(message));
    }
}

This solves the dependency problem effectively and without creating new classes and interfaces. However, now users of the SmtpMailSender class are forced to provide the send action in the constructor.

var sender = new SmtpMailSender(new SmtpClient().Send);
sender.Send(...);

If you have a good IoC container it can take care of this, but other users may not be so lucky. There are a few other things that I didn’t like as well:

  • 99% of this class’s functionality lies in this single action. A class where 99% of it’s functionality is injected in a single parameter raises the question why it really needs to exist at all.
  • The default implementation, SmtpClient.Send(), only needs to be overriden in a few test cases. Everyone else shouldn’t have to care about it.
  • Putting random delegates in a constructor makes me feel uncomfortable. Unless it is an intention-revealing named delegate, I don’t think this is a good pattern to be promoting.

Using Supersede Instance Variable instead

In Working Effectively with Legacy Code, Michael Feathers discusses a pattern called Supercede Instance Variable. Although it is used for technical reasons (replacing global dependencies in non-virtual C++ classes, where methods cannot be overridden via subclassing), I believe the pattern fits this usage example well.

Really, the only difference is that, here, is that unless a user performs the optional step of overriding the action via a special SupersedeSendAction method, a default is used:

public class SmtpMailSender : IMailSender
{
    public event EventHandler<MailSentEventArgs> MailSent;
    Action<MailMessage> send;

    public Action<MailMessage> DefaultSend
    {
        get { return new SmtpClient().Send; }
    }

    public SmtpMailSender()
    {
        this.send = DefaultSend;
    }

    public void Send(MailMessage message)
    {
        if (message == null) throw new ArgumentNullException("message");

        this.send(message);

        MailSent(new MailSentEventArgs(message));
    }

    /// <summary>
    /// Supersede the method that is used to send a MailMessage for testing
    /// purposes (supersede static variable to break dependency on non-
    /// mockable SmtpClient.Send() method).
    /// </summary>
    public void SupersedeSendAction(Action<MailMessage> newSend)
    {
        this.send = newSend;
    }
}

It is only a small difference, but as Feathers states, using the supersede term indicates it is a rare deviation from default behaviour, and for special cases only:

One nice thing about using the word supersede as the method prefix is that it is kind of fancy and uncommon. If you ever get concerned about whether people are using the superceding methods in production code, you can do a quick search to make sure they aren’t.

Only supersede when you can’t inject

Remember though, that Supersede Instance Variable should only be used for very special cases like individual system library calls in low-level wrapper classes. Dependency Injection and intention-revealing interfaces is still a much better pattern, and should still be your primary tool in all other situations.

May 9, 2009

3 Comments

Beau Crawford on May 12, 2009 at 2:11 am.

I use this approach a lot. You might consider marking the “SupersedeSendAction” method internal and then granting access to internals for your Test assembly using the InternalsVisibleTo attribute.

Alex Hoffman on May 12, 2009 at 10:37 am.

I think your injecting infrastructure into your code, just to support testing. A dubious practice?

Not to mention that you now have code, that smells of the author having a specific intention, which would no longer be apparent to a new programmer maintaining the code 18 months from now.

Richard on May 18, 2009 at 10:42 pm.

@Alex: it is infrastructure to support testing, yes, and it isn’t that pretty, so only use these sort of methods sparingly, for difficult situations. I don’t think it is too hard to understand for future programmers though (this is something I also worry about a lot).

Leave a Reply