The testable adapter anti-pattern

Let’s say you’re writing a module to deal with Windows optional features. Following the guidance of the Ports/Adapters/Simulators design approach, you come up with a port IFeatures and an adapter WindowsFeatures. “I must make my adapter testable!” you exclaim, and after a series of coding and testing steps, you come up with something like this:

internal sealed class Program
{
    private static void Main(string[] args)
    {
        IFeatures features = CreateFeatures();
        // ...
    }

    private static IFeatures CreateFeatures()
    {
        List<string> installed = new List<string>();

        ManagementScope scope = new ManagementScope(@"\\.\ROOT\cimv2");
        ObjectQuery query = new ObjectQuery("SELECT * FROM Win32_OptionalFeature");
        using (ManagementObjectSearcher searcher = new ManagementObjectSearcher(scope, query))
        using (ManagementObjectCollection results = searcher.Get())
        {
            foreach (ManagementObject result in results)
            {
                if ((uint)result["InstallState"] == 1)
                {
                    installed.Add((string)result["Name"]);
                }
            }
        }

        return new WindowsFeatures(installed, new WindowsFeatureFactory());
    }

    private sealed class WindowsFeatureFactory : IFeatureFactory
    {
        public IFeature Create(string name)
        {
            return new WindowsFeature(name);
        }

        private sealed class WindowsFeature : IFeature
        {
            private readonly string name;

            public WindowsFeature(string name)
            {
                this.name = name;
            }

            public void Enable()
            {
                // ... implementation code goes here , probably using
                // https://msdn.microsoft.com/en-us/library/windows/desktop/hh825834.aspx ...
            }

            public void Disable()
            {
                // ... more implementation code ...
            }
        }
    }
}

The humble adapter ends up this way:

public sealed class WindowsFeatures : IFeatures
{
    private readonly HashSet<string> enabled;
    private readonly IFeatureFactory featureFactory;

    public WindowsFeatures(IEnumerable<string> enabled, IFeatureFactory featureFactory)
    {
        this.enabled = new HashSet<string>(enabled);
        this.featureFactory = featureFactory;
    }

    public void Enable(string name)
    {
        if (!this.enabled.Contains(name))
        {
            this.enabled.Add(name);
            IFeature feature = featureFactory.Create(name);
            feature.Enable();
        }
    }

    public void Disable(string name)
    {
        if (this.enabled.Contains(name))
        {
            this.enabled.Remove(name);
            IFeature feature = featureFactory.Create(name);
            feature.Disable();
        }
    }
}

And of course, some unit tests to show the logic is sound:

[TestMethod]
public void ShouldEnableTheFeatureIfNotEnabled()
{
    StubFeatureFactory featureFactory = new StubFeatureFactory();
    WindowsFeatures features = new WindowsFeatures(new string[0], featureFactory);

    features.Enable("NewFeature");

    Assert.AreEqual(1, featureFactory.Features.Count);
    Assert.IsTrue(featureFactory.Features.ContainsKey("NewFeature"));
    Assert.AreEqual(1, featureFactory.Features["NewFeature"].EnableCount);
}

[TestMethod]
public void ShouldNotEnableTheFeatureIfAlreadyEnabled()
{
    StubFeatureFactory featureFactory = new StubFeatureFactory();
    WindowsFeatures features = new WindowsFeatures(new string[] { "OldFeature" }, featureFactory);

    features.Enable("OldFeature");

    Assert.AreEqual(0, featureFactory.Features.Count);
}

[TestMethod]
public void ShouldDisableTheFeatureIfEnabled()
{
    StubFeatureFactory featureFactory = new StubFeatureFactory();
    WindowsFeatures features = new WindowsFeatures(new string[] { "OldFeature" }, featureFactory);

    features.Disable("OldFeature");

    Assert.AreEqual(1, featureFactory.Features.Count);
    Assert.IsTrue(featureFactory.Features.ContainsKey("OldFeature"));
    Assert.AreEqual(1, featureFactory.Features["OldFeature"].DisableCount);
}

[TestMethod]
public void ShouldNotDisableTheFeatureIfAlreadyDisabled()
{
    StubFeatureFactory featureFactory = new StubFeatureFactory();
    WindowsFeatures features = new WindowsFeatures(new string[0], featureFactory);

    features.Disable("NewFeature");

    Assert.AreEqual(0, featureFactory.Features.Count);
}

It’s testable and makes use of some nice dependency inversion. So what’s the problem?

Well, unfortunately, this adapter contains absolutely zero code relevant to Windows features, despite its promise. That’s the catch — in order to make it “testable,” the essence of what made it an adapter (i.e. management of external dependencies) has vanished. Even worse, two new interfaces of indirection were introduced solely to serve the tests, which themselves are essentially shallow demonstrations of an idempotence guarantee.

Making your adapters testable might seem like sound design, but you must resist the urge lest you end up with confused object models like the above. Instead, focus on making the adapter as thin and boring as possible so that you have no need to test it (or at least, not extensively beyond basic contract tests).

For instance, if the goal is to avoid calling potentially expensive code if you know the action is unnecessary, why not make smarter data structures which can clearly record the design intent instead? Here is a set which can call a post-action after successfully adding or removing an item:

public static class SetExtensions
{
    public static void Add<T>(this ISet<T> set, T item, Action ifAdded)
    {
        if (set.Add(item))
        {
            ifAdded();
        }
    }

    public static void Remove<T>(this ISet<T> set, T item, Action ifRemoved)
    {
        if (set.Remove(item))
        {
            ifRemoved();
        }
    }
}

The tests can now operate directly at this level of abstraction instead of faking that they have anything to with feature installs:

[TestMethod]
public void ShouldRunActionAfterAdd()
{
    ISet<string> set = new HashSet<string>();
    bool ran = false;

    set.Add("newItem", () => ran = true);

    Assert.IsTrue(set.Contains("newItem"));
    Assert.IsTrue(ran);
}

[TestMethod]
public void ShouldSkipActionIfAlreadyAdded()
{
    ISet<string> set = new HashSet<string>(new string[] { "existingItem" });
    bool ran = false;

    set.Add("existingItem", () => ran = true);

    Assert.IsFalse(ran);
}

[TestMethod]
public void ShouldRunActionAfterRemove()
{
    ISet<string> set = new HashSet<string>(new string[] { "existingItem" });
    bool ran = false;

    set.Remove("existingItem", () => ran = true);

    Assert.IsFalse(set.Contains("existingItem"));
    Assert.IsTrue(ran);
}

[TestMethod]
public void ShouldSkipActionIfAlreadyRemoved()
{
    ISet<string> set = new HashSet<string>();
    bool ran = false;

    set.Remove("nonexistentItem", () => ran = true);

    Assert.IsFalse(ran);
}

And the WindowsFeature code can be one step closer to trivial:

public void Enable(string name)
{
    this.enabled.Add(name, () => this.DoEnable(name));
}

public void Disable(string name)
{
    this.enabled.Remove(name, () => this.DoDisable(name));
}

private void DoEnable(string name)
{
    IFeature feature = this.featureFactory.Create(name);
    feature.Enable();
}

private void DoDisable(string name)
{
    IFeature feature = this.featureFactory.Create(name);
    feature.Disable();
}

Testability can be a helpful attribute but should not be and end in itself. Especially when dealing with the edges of your system, look for ways to get closer to the correct by construction ideal.