The goal is to test everything automatically

So it all started with the following line of code in the Startup.cs of a WPF application,

locator.GetInstance<IAuthenticationService>().LogInBasedOnGeneralSettings();

It was to be replaced with these lines of code.

#if DEBUG
    locator.GetInstance<IAuthenticationService>().LogInBasedOnGeneralSettings();
#else 
    locator.GetInstance<LoginViewModel>().Show();
#endif

Reduce startup complexity

Going by the single-responsibility principle, the startup should be responsible for starting the app but not for making decisions.

The new code makes a decision, so it should be encapsulated in a component.

The simplest (though not testable) way of reducing cognitive complexity (SonarCube) (PDF Download) is to move the logic to a function. E.g.,

private void EnsureLoggedIn(IServiceLocator locator)
{
#if DEBUG
    locator.GetInstance<IAuthenticationService>().LogInBasedOnGeneralSettings();
#else 
    locator.GetInstance<LoginViewModel>().Show();
#endif
}

Now the calling code is much simpler,

 EnsureLoggedIn(locator);

Define a login service

I also like to reduce calls to locator.GetInstance() as much as possible, so I prefer do define something like a LoginService that consumes the IAuthenticationService and the LoginViewModel, so that you have something like this:

class LoginService(
    IAuthenticationService authenticationService,
    LoginViewModel loginViewModel)
{
    public void EnsureLoggedIn()
    {
#if DEBUG
        authenticationService.LogInBasedOnGeneralSettings();
#else 
        loginViewModel.Show();
#endif
    }
}

Now we can make the method in the startup file use the LoginService instead.

locator.GetInstance<LoginService>().EnsureLoggedIn()

We don’t need to pollute the startup with the nuance of which mode we’re in.

Making it testable

A colleague responded that,

“But then you have to […] inject a ViewModel into a Service?”

I’ll include the reply to show how the sausage is made: instead of just showing the solution, I often appreciate learning how people think about problems.

So, here was my response,

I’m not trying to be pedantic; it just comes naturally. 😃

• I was going to write that injecting a ViewModel into a service isn’t bad because it’s just a view model. But then I noticed that it seems to be communicating with the view in order to show something to the user. 😃
• We’re trying to abstract away complexity and to make our logic testable.
• We need to call Show() during startup; that’s a fact. If we introduce a service, it actually makes that part mockable.
• If we wanted to test that the LogInBasedOnGeneratedSettings() is called when expected, we couldn’t do that right now, could we?
• If we make it a service, then we could think about verifying the logic with a test.
• Of course, once we want to build the test, we’d then be confronted with the need to abstract away the compiler-define. Otherwise, we wouldn’t be able to test both branches without recompiling. That’s a code smell, too.
• Which is why I usually end up with some standard settings objects like the following.

public interface ICompilerSettings
{
    public bool IsDebug { get; }
}

public class CompilerSettings : ICompilerSettings
{
    public bool IsDebug
    {
        get
        {
#if DEBUG
        return true;
#else 
        return false;
#endif
        }
    }
}

public interface ILoginServiceSettings
{
    public bool ForceLogin { get; }
}

public class LoginServiceSettings(ICompilerSettings compilerSettings) : ILoginServiceSettings
{
    public bool ForceLogin => !compilerSettings.IsDebug;
}

You might think that this is over-engineering, overkill, an excess of ceremony introduced by an architect astronaut!

I, on the other hand, think that this is a minimal solution that separates concerns and makes all branches testable.

Once we inject the ILoginServiceSettings into the LoginService, we can easily verify the behavior with tests (using fakes, mocks, etc.).

class LoginService(ILoginServiceSettings settings,
    IAuthenticationService authenticationService,
    LoginViewModel loginViewModel)
{
    public void EnsureLoggedIn()
    {
        if (!settings.ForceLogin())
        {
            authenticationService.LogInBasedOnGeneralSettings();
        }
        else
        {
            loginViewModel.Show();
        }
}

It might look like a lot of ceremony but, without it, how else can you say with confidence that the login is required in some cases but not others? We can even verify that it’s not required in DEBUG mode by mocking ICompilerSettings.

Then the only thing we have to verify without automated tests is that the CompilerSettings are implemented as expected, which is very little code to manually check. We don’t need to look at the rest. 👍

We want to test everything

My colleague very politely responded,

“Injecting ViewModels into Services is generally considered bad practice. The rest seems to depend on what you wish to test and don’t overengineer it…”

At this point, we took the conversation to meatspace, i.e., I ran over to his desk to tell him that “I always want to test everything.” I am willing to concede on time constraints, priority, and planning, but my goal is “test all the code paths” eventually. I’m patient, though, so will accept unwritten tests as technical debt. I will design my could so that it could be tested, though, when we eventually find time to do so. This

We shouldn’t just punt on tests because “it looks difficult” or “it’s not much logic”.

In the first case, the fact that it looks difficult may indicate you’re not writing your code in a testable way or may reveal architectural problems. In the second case, those are famous last words. If it’s just a little logic, then why wouldn’t you just test it instead of investing the time arguing that you don’t need a test?

If you have a code base that’s difficult to test because of some unfortunate architectural decisions, then the thing to do is not to ignore it but to slowly chip away at it.

How else would we get a higher percentage of our code covered by tests? Hint: it’s not by continuing to write more code without tests.

Addendum: A note on architectural boundaries

He’d also argued about mixing levels—injecting a ViewModel into a service—but I convinced him that this is already what was happening whether you wrap a service around it or not. The startup is already instantiating and using a view model. Is that somehow better?

I don’t think it’s a bad thing, as it’s just a way of asking the user for input in order to continue starting the application. It’s a step in the application startup. If you wrap it in a service, then you can at least test that the code does what you want. This is exactly the kind of thing that everyone is going to forget to test manually.

Should the startup be using the view? Maybe, maybe not. It currently does and it makes for a legible workflow. Being too pedantic about architectural boundaries just for the sake of it is often wasted effort and can often lead to overly complex solutions.

Ordinarily, the problem would be that introducing a dependency across a boundary weakens testability, but that’s obviously not the case here. We can just mock away the view and test all of our logic.

I wouldn’t worry about this one too much, though it’s a good rule of thumb.