Struggling with how test a method with several steps

Question

I have a MVC web site with a user registration function and I have one layer that I can't wrap my head around how to test. Basically the method does this...

1) Checks the database to see if the user is already registered

2) Maps a view model into an entity framework model

3) Saves the user to the database

4) Send the user a confirmation email

5) Performs a web service post to a 3rd party API

6) Updates the user (created in step #3) with a value returned from the 3rd party

I am struggling with how or should I test this. I have abstracted all the steps into separate services and I have tests for those so really tests for this method would be testing the flow. Is that valid?

In a TDD world, and I'm trying to think that way, should I have a method like this? Or is there a design issue I am not seeing?

I can write the tests and I understand how to mock but when I write the test for step #6 I have mock setups that return data for steps #1, #2 and #5 to ensure the code gets that far and to ensure the object saved in step #6 has the correct state. My test setups get long quickly.

If this is how it's supposed to be then great! But I feel like I am missing my light bulb moment.

My Lightbulb Moment I liked Keith Payne's answer and looking at his interfaces made me see things from a new perspective. I also watched a TDD Play by Play course (http://www.pluralsight.com/courses/play-by-play-wilson-tdd) and that really helped me understand the process. I was thinking about the process from the inside out and not from the outside in.

This is definitely a new way of thinking about software development.

Answer 1

Difficult test setups are a code-smell and I think you see this already. The answer is more cowbell (abstraction).

This is a common fault in controller methods acting as both controller of the UI and orchestrating the business process. Steps 5 & 6 probably belong together, steps 1, 3 & 4 similarly should be abstracted away to another method. Really the only thing a controller method should do is to receive data from the view, hand it off to an application- or business- layer service, and compile the results into a new view to display back to the user (mapping).

EDIT:

The AccountManager class that you mention in the comment is a good step towards good abstraction. It lives in the same namespace as the rest of the MVC code, which unfortunately makes it easier to crisscross the dependencies. For instance, passing a view model to the AccountManager is a dependency in the "wrong" direction.

Imagine this idealized architecture for a web application:

Application Layer

1. UI (JavaScript/HTML/CSS)
2. Model-View-Controller (Razor/ViewModel/Navigation)
3. Application Services (Orchestration/Application Logic)

Business Layer

1. Domain Services (Domain [EF] Models/Unit Of Work/Transactions)
2. WCF/Third Party API's (Adapters/Client Proxies/Messages)

Data Layer

1. Database

In this architecture, each item references the item below it.

Inferring some things about your code, AccountManager is at highest an application service (in the referencing hierarchy). I do not think that it is logically part of the MVC or UI components. Now, if those architecture items were in different dlls, the IDE would not allow you to pass a view model into a method of the AccountManager. It would cause a circular dependency.

Outside of the architectural concerns, it is also apparent that the view model is not appropriate to pass along because it will invariably include data that support view rendering that is useless to the AccountManager. It also means that AccountManager must have knowledge of the meaning of properties in the view model. Both the view model class and AccountManager are now dependent on each other. This introduces unnecessary brittleness and fragility to the code.

A better option is to pass along simple parameters, or if you prefer, to package them into a new Data-Transfer-Object (DTO) which would be defined by contract in the same place as the AccountManager.

Some example interfaces:

namespace MyApp.Application.Services
{
    // This component lives in the Application Service layer and is responsible for orchestrating calls into the
    // business layer services and anything else that is specific to the application but not the overall business domain.

    // For instance, sending of a confirmation email is probably a requirement in some application process flows, but not
    // necessarily applicable to every instance of adding a user to the system from every source. Perhaps there is an admin back-end
    // application which may or may not send the email when an administrator registers a new user. So that back-end 
    // application would have a different orchestration component that included a parameter to indicate whether to 
    // send the email, or to send it to more than one recipient, etc.

    interface IAccountManager
    {
        bool RegisterNewUser(string username, string password, string confirmationEmailAddress, ...);
    }
}

namespace MyApp.Domain.Services
{
    // This is the business-layer component for registering a new user. It will orchestrate the
    // mapping to EF models, calling into the database, and calls out to the third-party API.

    // This is the public-facing interface. Implementation of this interface will make calls
    // to a INewUserRegistrator and IExternalNewUserRegistrator components.

    public interface IUserRegistrationService
    {
        NewUserRegistrationResult RegisterNewUser(string username, string password, ...);
    }

    public class NewUserRegistrationResult
    {
        public bool IsUserRegistered { get; set; }
        public int? NewUserId { get; set; }

        // Add additional properties for data that is available after
        // the user is registered. This includes all available relevant information
        // which serves a distinctly different purpose than that of the data returned
        // from the adapter (see below).
    }

    internal interface INewUserRegistrator
    {
        // The implementation of this interface will add the user to the database (or DbContext)
        // Alternatively, this could be a repository 
        User RegisterNewUser(User newUser) ;
    }

    internal interface IExternalNewUserRegistrator
    {
        // Call the adapter for the API and update the user registration (steps 5 & 6)
        // Replace the return type with a class if more detailed information is required

        bool UpdateUserRegistrationFromExternalSystem(User newUser);
    }

    // Note: This is an adapter, the purpose of which is to isolate details of the third-party API
    // from yor application. This means that what comes out from the adapter is determined not by what
    // is provided by the third party API but rather what is needed by the consumer. Oftentimes these
    // are similar.

    // An example of a difference can be some mundance detail. For instance, say that the API
    // returns -1 for some non-nullable int value when the intent is to indicate lack of a match.
    // The adapter would protect the application from that detail by using some logic to interpret
    // the -1 value and set a bool to indicate that no match was found, and to use int?
    // with a null value instead of propagating the magic number (-1) throughout your application.

    internal interface IThirdPartyUserRegistrationAdapter
    {
        // Call the API and interpret the response from the API.
        // Also perform any logging, exception handling, etc.
        AdapterResult RegisterUser(...);
    }

    internal class AdapterResult
    {
        public bool IsSuccessful { get; set; }

        // Additional properties for the response data that is needed by your application only.
        // Do not include data provided by the API response that is not used.
    }
}

Something to keep in mind is that this kind of design-all-at-once is the opposite of TDD. In TDD, the need for these abstractions become apparent as you test and write code from the outside-in. What I've done here is to skip all of that and jump directly to designing the innerworks based on what I picture in my head. In almost all cases this leads to over-design and over-abstraction, which TDD naturally prevents.

Answer 2

In a TDD world, and I'm trying to think that way, should I have a method like this? Or is there a design issue I am not seeing?

Your method is fine and TDD has little to say here. It's more about design. After writing single responsibility-oriented components (as you seem to have done) there comes a point where you have to use them together to implement an use case. Doing that you'll often end up with facade classes just like yours (but they should be in minority).

As for testing, there is no easy way around (if at all). Your setups might be longer than usual. It often helps to distinguish which dependencies will serve as stubs (providing data for tested method - setup) and which as mocks (which you'll assert against). As you noticed, steps 1, 2, 5 will be used for setups only.

To make your work easier and tests more readable, consider wrapping certain setup configurations in methods:

[Test] public void UserIsSavedToDatabase()
{
    UserIsNotRegistered();
    ViewModelIsMappedToEntity();

    ...
}

Answer 3

In my opinion you are thinking things right. Although you encapsulate all different tasks on separated modules you'll need a piece of code to coordinate all that stuff.

These tests on charge of evaluating complex flows are really a nightmare since you end up having a bunch of mocks and setups. I don't think you have many ways to escape.

Since testing behavior is very fragil provided it heavily relies on internal implementation, my advice is not to spend too much time writing tests for this method.

When I face this situation I try to add tests for the more relevant scenarios and omit the obvious to reduce test suit complexity. Avoid having 100 tests for this since you will likely need to change the flow at some point and that will cause 100 complex tests to be changed.

It's not ideal but I believe is a trade off decision.