What are the possible problems with unit testing A

问题:

I've been looking at the way unit testing is done in the NuGetGallery. I observed that when controllers are tested, service classes are mocked. This makes sense to me because while testing the controller logic, I didn't want to be worried about the architectural layers below. After using this approach for a while, I noticed how often I was running around fixing my mocks all over my controller tests when my service classes changed. To solve this problem, without consulting people that are smarter than me, I started writing tests like this (don't worry, I haven't gotten that far):

public class PersonController : Controller
{
    private readonly LESRepository _repository;

    public PersonController(LESRepository repository)
    {
        _repository = repository;
    }

    public ActionResult Index(int id)
    {
        var model = _repository.GetAll<Person>()
            .FirstOrDefault(x => x.Id == id);

        var viewModel = new VMPerson(model);
        return View(viewModel);
    }
}

public class PersonControllerTests
{
    public void can_get_person()
    {
        var person = _helper.CreatePerson(username: "John");
        var controller = new PersonController(_repository);
        controller.FakeOutContext();

        var result = (ViewResult)controller.Index(person.Id);
        var model = (VMPerson)result.Model;
        Assert.IsTrue(model.Person.Username == "John");
    }
}

I guess this would be integration testing because I am using a real database (I'd prefer an inmemory one). I begin my test by putting data in my database (each test runs in a transaction and is rolled back when the test completes). Then I call my controller and I really don't care how it retrieves the data from the database (via a repository or service class) just that the Model to be sent to the view must have the record I put into the database aka my assertion. The cool thing about this approach is that a lot of times I can continue to add more layers of complexity without having to change my controller tests:

public class PersonController : Controller
{
    private readonly LESRepository _repository;
    private readonly PersonService _personService;

    public PersonController(LESRepository repository)
    {
        _repository = repository;
        _personService = new PersonService(_repository);
    }

    public ActionResult Index(int id)
    {
        var model = _personService.GetActivePerson(id);
        if(model  == null)
          return PersonNotFoundResult();

        var viewModel = new VMPerson(model);
        return View(viewModel);
    }
}

Now I realize I didn't create an interface for my PersonService and pass it into the constructor of my controller. The reason is 1) I don't plan to mock my PersonService and 2) I didn't feel I needed to inject my dependency since my PersonController for now only needs to depend on one type of PersonService.

I'm new at unit testing and I'm always happy to be shown that I'm wrong. Please point out why the way I'm testng my controllers could be a really bad idea (besides the obvious increase in the time my tests will take to run).

回答1:

Hmm. a few things here mate.

First, it looks like you're trying to test the a controller method. Great :)

So this means, that anything the controller needs, should be mocked. This is because

1. You don't want to worry about what happens inside that dependency.
2. You can verify that the dependency was called/executed.

Ok, so lets look at what you did and I'll see if i can refactor it to make it a bit more testable.

-REMEMBER- i'm testing the CONTROLLER METHOD, not the stuff the controller method calls/depends upon.

So this means I don't care about the service instance or the repository instance (which ever architectural way you decide to follow).

NOTE: I've kept things simple, so i've stripped lots of crap out, etc.

Interface

First, we need an interface for the repository. This can be implemented as a in-memory repo, an entity framework repo, etc.. You'll see why, soon.

public interface ILESRepository
{
    IQueryable<Person> GetAll();
}

Controller

Here, we use the interface. This means it's really easy and awesome to use a mock IRepository or a real instance.

public class PersonController : Controller
{
    private readonly ILESRepository _repository;

    public PersonController(ILESRepository repository)
    {
       if (repository == null)
       {
           throw new ArgumentNullException("repository");
       }
        _repository = repository;
    }

    public ActionResult Index(int id)
    {
        var model = _repository.GetAll<Person>()
            .FirstOrDefault(x => x.Id == id);

        var viewModel = new VMPerson(model);
        return View(viewModel);
    }
}

Unit Test

Ok - here's the magic money shot stuff. First, we create some Fake People. Just work with me here... I'll show you where we use this in a tick. It's just a boring, simple list of your POCO's.

public static class FakePeople()
{
    public static IList<Person> GetSomeFakePeople()
    {
        return new List<Person>
        {
            new Person { Id = 1, Name = "John" },
            new Person { Id = 2, Name = "Fred" },
            new Person { Id = 3, Name = "Sally" },
        }
    }
}

Now we have the test itself. I'm using xUnit for my testing framework and moq for my mocking. Any framework is fine, here.

public class PersonControllerTests
{
    [Fact]
    public void GivenAListOfPeople_Index_Returns1Person()
    {
        // Arrange.
        var mockRepository = new Mock<ILESRepository>();
        mockRepository.Setup(x => x.GetAll<Person>())
                                   .Returns(
                                FakePeople.GetSomeFakePeople()
                                          .AsQueryable);
        var controller = new PersonController(mockRepository);
        controller.FakeOutContext();

        // Act.
        var result = controller.Index(person.Id) as ViewResult;

        // Assert.
        Assert.NotNull(result);
        var model = result.Model as VMPerson;
        Assert.NotNull(model);
        Assert.Equal(1, model.Person.Id);
        Assert.Equal("John", model.Person.Username);

        // Make sure we actually called the GetAll<Person>() method on our mock.
        mockRepository.Verify(x => x.GetAll<Person>(), Times.Once());
    }
}

Ok, lets look at what I did.

First, I arrange my crap. I first create a mock of the ILESRepository. Then i say: If anyone ever calls the GetAll<Person>() method, well .. don't -really- hit a database or a file or whatever .. just return a list of people, which created in FakePeople.GetSomeFakePeople().

So this is what would happen in the controller ...

var model = _repository.GetAll<Person>()
                       .FirstOrDefault(x => x.Id == id);

First, we ask our mock to hit the GetAll<Person>() method. I just 'set it up' to return a list of people .. so then we have a list of 3 Person objects. Next, we then call a FirstOrDefault(...) on this list of 3 Person objects .. which returns the single object or null, depending on what the value of id is.

Tada! That's the money shot :)

Now back to the rest of the unit test.

We Act and then we Assert. Nothing hard there. For bonus points, I verify that we've actually called the GetAll<Person>() method, on the mock .. inside the Controller's Index method. This is a safety call to make sure our controller logic (we're testing for) was done right.

Sometimes, you might want to check for bad scenario's, like a person passed in bad data. This means you might never ever get to the mock methods (which is correct) so you verify that they were never called.

Ok - questions, class?

回答2:

Even when you do not plan to mock an interface, I strongly suggest you to do not hide the real dependencies of an object by creating the objects inside the constructor, you are breaking the Single Responsibility principle and you are writing un-testable code.

The most important thing to consider when writing tests is: "There is no magic key to write tests". There are a lot of tools out there to help you write tests but the real effort should be put in writing testable code rather than trying to hack our existing code to write a test which usually ends up being an integration test instead of a unit-test.

Creating a new object inside a constructor is one of the first big signals that your code is not testable.

These links helped me a lot when I was making the transition to start writing tests and let me tell you that after you start, that will become a natural part of your daily work and you will love the benefits of writing tests I can not picture myself writing code without tests anymore

Clean code guide (used in Google): http://misko.hevery.com/code-reviewers-guide/

To get more information read the following:

http://misko.hevery.com/2008/09/30/to-new-or-not-to-new/

and watch this video cast from Misko Hevery

http://www.youtube.com/watch?v=wEhu57pih5w&feature=player_embedded

Edited:

This article from Martin Fowler explain the difference between a Classical and a Mockist TDD approach

http://martinfowler.com/articles/mocksArentStubs.html

As a summary:

• Classic TDD approach: This implies to test everything you can without creating substitutes or doubles (mocks, stubs, dummies) with the exception of external services like web services or databases. The Classical testers use doubles for the external services only

  • Benefits: When you test you are actually testing the wiring logic of your application and the logic itself (not in isolation)
  • Cons: If an error occurs you will see potentially hundreds of tests failing and it will be hard to find the code responsible

• Mockist TDD approach: People following the Mockist approach will test in isolation all the code because they will create doubles for every dependency

  • Benefits: You are testing in isolation each part of your application. If an error occurs, you know exactly where it occurred because just a few tests will fail, ideally only one
  • Cons: Well you have to double all your dependencies which makes tests a little bit harder but you can use tools like AutoFixture to create doubles for the dependencies automatically

This is another great article about writing testable code

http://www.loosecouplings.com/2011/01/how-to-write-testable-code-overview.html

回答3:

There are some downsides.

First, when you have a test that depends on an external component (like a live database), that test is no longer really predictable. It can fail for any number of reasons - a network outage, a changed password on the database account, missing some DLLs, etc. So when your test suddenly fails, you cannot be immediately sure where the flaw is. Is it a database problem? Some tricky bug in your class?

When you can immediately answer that question just by knowing which test failed, you have the enviable quality of defect localization.

Secondly, if there is a database problem, all your tests that depend on it will fail at once. This might not be so severe, since you can probably realize what the cause is, but I guarantee it will slow you down to examine each one. Widespread failures can mask real problems, because you don't want to look at the exception on each of 50 tests.

And I know you want to hear about factors besides the execution time, but that really does matter. You want to run your tests as frequently as possible, and a longer runtime discourages that.

I have two projects: one with 600+ tests that run in 10 seconds, one with 40+ tests that runs in 50 seconds (this project does actually talk to a database, on purpose). I run the faster test suite much more frequently while developing. Guess which one I find easier to work with?

All of that said, there is value in testing external components. Just not when you're unit-testing. Integration tests are more brittle, and slower. That makes them more expensive.

回答4:

Accessing the database in unit tests has the following consequences:

1. Performance. Populating the database and accessing it is slow. The more tests you have, the longer the wait. If you used mocking your controller tests may run in a couple of milliseconds each, compared to seconds if it was using the database directly.
2. Complexity. For shared databases, you'll have to deal with concurrency issues where multiple agents are running tests against the same database. The database needs to be provisioned, structure needs to be created, data populated etc. It becomes rather complex.
3. Coverage. You mind find that some conditions are nearly impossible to test without mocking. Examples may include verifying what to do when the database times out. Or what to do if sending an email fails.
4. Maintenance. Changes to your database schema, especially if its frequent, will impact almost every test that uses the database. In the beginning when you have 10 tests it may not seem like much, but consider when you have 2000 tests. You may also find that changing business rules and adapting the tests to be more complex, as you'll have to modify the data populated in the database to verify the business rule.

You have to ask whether it is worth it for testing business rules. In most cases, the answer may be "no".

The approach I follow is:

1. Unit classes (controllers, service layers etc) by mocking out dependencies and simulating conditions that may occur (like database errors etc). These tests verify business logic and one aims to gain as much coverage of decision paths as possible. Use a tool like PEX to highlight any issues you never thought of. You'll be surprised how much robust (and resilient) your application would be after fixing some of the issues PEX highlights.
2. Write database tests to verify that the ORM I'm using works with the underlying database. You'll be surprised the issues EF and other ORM have with certain database engines (and versions). These tests are also useful to for tuning performance and reducing the amount of queries and data being sent to and from the database.
3. Write coded UI tests that automates the browser and verifies the system actually works. In this case I would populate the database before hand with some data. These tests simply automate the tests I would have done manually. The aim is to verify that critical pieces still work.