Unit test Web API controller that uses a static helper class which uses app config setting

Question

I have a situation where I have a static class that reads my application configuration file for some configuration value. When unit testing Web API controllers that make use of this static class I am getting an error about the confgiration setting being null.

I could have duplicated the value of the key in my app config for the unit test project but that seemed dirty.

Instead I have tried to abstract the dependency on app settings via a configuration interface and inject the interface using Unity. This all seems to work but I've no idea if I have done this correctly (code below).

NB: I am using FakeItEasy for mocking in unit tests.

So my question is simple - is this the correct way?

Interface for accessing App config

public interface IConfigurationReader
{
 string GetAppSetting(string key);
}
public class ConfigurationReader : IConfigurationReader
{
 public string GetAppSetting(string key)
 {
 return ConfigurationManager.AppSettings[key];
 }
}

Add init method to static class

private static IConfigurationReader _config;
public static void Init(IConfigurationReader config)
{
 _config = config;
}

Register & resolve in Unity Register Types method

unityContainer.RegisterType<IConfigurationReader, ConfigurationReader>("Config");
HeaderHelper.Init(unityContainer.Resolve<IConfigurationReader>("Config"));

In my unit test project fake interface and call static init

_config = A.Fake<IConfigurationReader>();
HeaderHelper.Init(_config);

In my unit test fake call to GetAppSetting

A.CallTo(() => _config.GetAppSetting("AES")).Returns(AesKey);

• David Arno: Simple answer: if you have a helper class that needs access to mutable state (such as your app config), then it should not be static. Make it a normal class and pass the app config IConfigurationReader instance to its constructor.
• Sykomaniac: @DavidArno if I make this class non static I suppose I could then abstract the helper class and mock that out as well?
• Mathieu Guindon: Welcome to Code Review! It's a bit hard to wrap one's head around exactly what's going on and where, because you've boiled your code down to one specific question - which isn't how this site works best (from our help center: "do I want feedback on any/all aspects of the code?") - this question would be much more interesting if you included that whole static class, and a bit more context to show how it's used in your code - see Simon's checklist for posting a good question.
• Mathieu Guindon: How to get the best value out of Code Review is another good meta post to read. Cheers!

Answer 1

You're doing it wrong.

You can't do Dependency Injection and have static class dependencies scattered throughout your code: when you do Dependency Injection, you need to go all the way or not at all, else you gain the complexity and lose the benefits.

Proper DI goes like this:

1. Register: Set up your IoC container and register dependencies.
2. Resolve: Resolve the entire application's dependency graph - this is ONE method call.
3. Release: Dispose any disposables, clean up and tear down.

Your solution doesn't follow the Resolve step correctly, since you're calling .Resolve to fetch a single specific type, only to call that specific Init method on your static class: in all likelihood you're eventually going to need to do this in more than one place, and that's only because you stopped injecting dependencies at one point.

Any code that uses this HeaderHelper static class has a hidden dependency that isn't being injected, which means every single piece of code that uses HeaderHelper is tightly coupled with one specific implementation of that class.

Consider the below hypothetical MyClass; its constructor implicitly documents the type's dependencies (here ISomeDependency), but DoSomething has a dependency that isn't injected from the outside:

private readonly ISomeDependency _dependency;
public MyClass(ISomeDependency dependency)
{
 _dependency = dependency; // explicit dependency, constructor-injected
}
private void DoSomething()
{
 var foo = HeaderHelper.Foo; // HeaderHelper is a hidden dependency
 _dependency.DoSomething(foo);
}

There are several ways to fix this, but which is best for your case largely depends on context you haven't provided in your question (we know nothing of HeaderHelper other than it has a vague useless name that doesn't mean anything - you should really avoid "Helper" in class names).

---

1. Constructor Injection

You could do exactly like you did with the static ConfigurationManager class, and wrap it with an interface:

public interface IHeaderHelper // todo: rename!!!
{
 // members
}

That Init method seems rather artificial, and wouldn't be needed if the class' constructor took an IConfigurationReader dependency.

public class HeaderHelper : IHeaderHelper // notice: NOT static
{
 private readonly IConfigurationReader _configReader;
 public HeaderHelper(IConfigurationReader configReader)
 {
 _configReader = configReader;
 }
 // members
}

And then every class that needs to use a HeaderHelper should intake an IHeaderHelper constructor argument, and replace the implicit dependencies to the static class with explicit dependencies to that interface, so instead of this:

HeaderHelper.Foobar();

You would have that:

_headerHelper.Foobar();

Where _headerHelper is a private readonly IHeaderHelper instance field.

---

2. Ambient Context

Constructor injection is nice, but sometimes there are cross-cutting dependencies that are needed pretty much everywhere - it's fairly possible that configuration is one of such dependencies. Wrapping it with an interface and constructor-injecting it will work, but then you'll end up with lots of classes that intake this dependency, and when that feels like constructor-bloating, you might want to explore other possibilities.

Mark Seemann's blog describes an interesting pattern to address this problem: the Ambient Context.

This DI pattern would change this:

var foo = HeaderHelper.Foo();

Into this:

var foo = HeaderSettingsContext.Current.Foo();

The Ambient Context is still an implicit dependency though, and Constructor Injection should be preferred most of the time. But by storing the current context in thread-local storage, your tests can easily manipulate the current context, and alter what Foo() does in your application (e.g. hit the file system and read some XML value vs return a mocked-up value).

This article by Adras Nemes describes more DI patterns (the article is based on Mark Seemann's work).

• Sykomaniac: Thanks for the detailed response. I did realise this was probably wrong after I took a break. I changed it exactly to as described in option 1. Thanks for taking the time to reply though I really appreciate it