Unit Test for Void which copies data from one location to another

Question

Still learning and getting my head around Unit Testing, (also trying to get into TDD, though I appreciate it isn't the same thing) and in many ways it is changing/improving my code. But I come across scenarios where I am unsure how to test a piece of functionality. Which leaves me wondering whether it's my code design that is the issue, or my knowledge of unit testing (plus mocking etc). It could be either here, so I'm looking for advice how to change my code or how to test it, whichever is the right thing to do.

In this case I am writing a void which takes data from one database and loads it into another (both use EF6).

So my _manager.UpdateHissOfficersFromSource() calls the source repository, which gets the data, then calls the target repository to insert the data. The only thing I can think of that might help test it is if the Insert returned a count, which I could then check in the Test.

    public void UpdateHissOfficersFromSource()
    {
        var sourceOfficers = _sourceRepository.GetAllActiveOfficers();

        List<Officer> officers = new List<Officer>(sourceOfficers
            .Select(o => new Officer
            {
                Code = o.officer_code,
                Name = o.officer_name,
                Telephone = o.officer_telephone,
                LastUpdated = DateTime.Now
            }));

        _targetRepository.Add(officers);
    }

I'm using Mock, but I doubt that matter which I'm using.

UPDATE:

So based on the replies, this is what I've done, which passes. Is it sufficient?

public void UpdateActionOfficersDataFromConfirm()
    {
        _officerManager.UpdateHissOfficersFromConfirm();
        _sourceRepository.Verify(mock => mock.GetAllActiveOfficers(), Times.Once());
        _targetRepository.Verify(mock => mock.Add(It.IsAny<List<Officer>>()), Times.Once());
    }

Answer 1

Do you inject the target repository (i.e. are you using dependency injection)

If so, you should be able to assert that _targetRepository.Add(officers) is called by querying your mocked target repository.

This is a common problem and pattern for a solution in testing. Dependency injection helps you solve this since your test can provide the mocked repository to your test class, provide results for method calls on this mock and provide assertions on methods called on this mock.

Here's how you do this in Moq:

mockSomeClass.Verify(mock => mock.DoSomething(), Times.Once());

Returning the list that you've called _targetRepository with is all very well, but what guarantee do you have that you've called _targetRepository with that list

Answer 2

There is nothing complicated here. You just need to check if side-effect happened. Pseudocode:

PreviousOfficers = _targetRepository.GetAllOfficers()
UpdateHissOfficersFromSource()
NewOfficers = _targetRepository.GetAllOfficers()
AddedOfficers = NewOfficers except PreviousOfficers
ActiveOfficers = _sourceRepository.GetAllActiveOfficers()
Assert.AreEqual(ActiveOfficers, AddedOfficers)

This can be run both against test DB with real repositories or you can mock it out, making it independent on DB.

Answer 3

Your update asks is this sufficient to test your scenario:

_officerManager.UpdateHissOfficersFromConfirm();
_sourceRepository.Verify(mock => mock.GetAllActiveOfficers(), Times.Once());
_targetRepository.Verify(mock => mock.Add(It.IsAny<List<Officer>>()), Times.Once());

Ultimately it's up to you, however I wouldn't be happy with it as a test. All you are testing is that GetAllActiveOfficers has been called once and that Add has been called once with a List of Office.

You aren't testing that the list has anything in it, or that the contents of the list have any relationship to the source for the information.

If I was testing the method, I would be looking to test the flow of data through the method to validate that the information returned from GetAllActiveOfficers is the source of what gets passed to the Add method.

The test would look something like this (pseudo code):

var allActiveOfficers = new List<?> { /* Populate with known start data */ };
_sourceRepositoryMock.Setup(x=>x.GetAllActiveOfficers()).Returns(allActiveOfficers);

// Create list of data that is expected to be passed to Add method and verify
// that the correct information is passed using a Callback
var expectedOfficers = new List<Officer> { /* Populate with tranformed start data */};
_targetRepositoryMock.Setup(x=>x.Add(It.IsAny<List<Officer>>()))
                    .Callback((List<Officer> list)=>/* validate list==expectedOfficers */);

_officerManager.UpdateHissOfficersFromConfirm();    

// Validate add method has been called at least once.  This is so that we
// know that the Callback has been invoked to check a list.
_targetRepositoryMock.Verify(mock=>mock.Add(It.IsAny<List<Officer>>()),

Answer 4

Many of the problems with these test have already been pointed out, but allow me to pile on for a moment. The test you have here is arguably not "self verfying" - as per the FIRST guidelines. What does that mean? Well at its simplest level it means that the test should have an assertion and shouldn't require manual intervention or interpretation to verify if it's right or wrong. But I believe there are few ways that a test can have assertions and not be self-verifying. Ask yourself a couple questions:

• If I only add one officer from the select, will this fail?
• If call GetAllActiveOfficers more than once the test will fail, should it?

The answer to both of these questions is really "no". You don't care how many times GetAllActiveOfficers is called, probably, but you do care that the right officers are added to the target repository. In other words this test verifies that you wrote the implementation you said you would, but it doesn't verify that the implementation is correct!. This is a sign that you probably wrote the code first, instead of the tests, or at least decided what the code would be before you wrote anything.

Let's do this in a test-driven fashion, but with one rule: only mock one object per test, stub anything else (or use real data).

What if you started the test like this:

public void UpdateActionOfficersDataFromConfirm()
{
  _sourceRepository.stub(mock => mock.GetAllActiveOfficers().andReturn([officerOne, officerTwo]):
}

Obviously this test isn't finished yet. officerOne and officerTwo don't exist yet. What it does do is create a stub of the data coming from your repository query. Why a stub? A stub corresponds roughly to a Given. "Given that there are two active officers" might make for a good cucumber test. In this case you don't want to test the way your query is called or that it's called, you want to test what your code does when there are multiple officers in the database. You can make this test pass without using GetAllActiveOfficers of course, but it would be more difficult than actually calling the method.

Continuing:

public void UpdateActionOfficersDataFromConfirm()
{
  var officer = new Officer();
  officer.officer_code = "CODE";

  _sourceRepository.Setup(mock => mock.GetAllActiveOfficers()).Returns([officer]):

  _officerManager.UpdateHissOfficersFromConfirm();

  _targetRepository.Verify(repository => repository.Add(It.Is<Officer>(officer.officer_code == "CODE")));
}

Please note I'm writing this in markdown, so any syntax errors are accidental.

Now you'll notice I only did one property on the officer to make sure it matches. You'll also notice I switched from returning two officers to one officer. That's because this test is focused on correctly mapping the officer from the source repository to the destination repository. You probably in one test would want to make sure you get an officer with the three fields that are mapped: officer_code, officer_name and officer_telephone. That will force the code to take the first officer out of GetAllActiveOfficers and map the three fields. You'll want to write another test that verifies you map more than one officer. Something like this:

public void UpdateActionOfficersStoresAllOfficers()
{
  var officerOne = new Officer();
  officerOne.officer_code = "officer code 1";
  var officerTwo = new Officer();
  officerTwo.officer_code = "officer code 2";

  _sourceRepository.Setup(mock => mock.GetAllActiveOfficers()).Returns([officer]):

  _officerManager.UpdateHissOfficersFromConfirm();

  _targetRepository.Verify(repository => repository.Add(It.Is<Officer>(officer.officer_code == "officer code 1")));
  _targetRepository.Verify(repository => repository.Add(It.Is<Officer>(officer.officer_code == "officer code 2")));

Now in that test you don't validate every single field is mapped. Just one is enough to be certain you are storing the right officers. I'd only add those assertions again if there's actually a bug because somebody didn't map all the fields when returning more than one officer. To create that bug you'd almost have to do it on purpose, and I wouldn't worry about it yet.

Finally you'll want to make sure you're mapping the date. This seems like it's the reason you're actually writing this loop and creating cloned officers, which is why I have it as a separate test. Now you can't use DateTime.Now in a unit test directly, because "now" is constantly changing during the run of the test. There's numerous ways to extract time into an abstraction, so you can stub it, and in this case I'm going to create the officerManager with a "now" object.

// Production Code
public interface NowProvider {
  DateTime Now();
}

// Production Code
public class DateTimeNowProvider : NowProvider {
  public DateTime Now() {
    return DateTime.Now();
  }
}

// TEST CODE
public class TestNow : Now Provider {
  public DateTime now;

  public DateTime Now() {
    return now;
  }
}

public void UpdateActionOfficersDataStoresANewDate()
{
  var nowProvider = new TestNow();
  nowProvider.now = DateTime.Now() ;
  _sourceRepository.nowProvider = nowProvider;

  var officer = new Officer();
  _sourceRepository.Setup(mock => mock.GetAllActiveOfficers()).Returns([officer]):

  _officerManager.UpdateHissOfficersFromConfirm();

  _targetRepository.Verify(repository => repository.Add(It.Is<Officer>(officer.LastUpdated == nowProvider.now)));
}

Now this time interface is far from the only way you could abstract time, it's just the first one that comes to mind, and this answer has gotten longer than I expected.

Since you're learning TDD let me give you a few guidelines I used to break this down into multiple tests.

• Break down the responsibilities you need to test.

In this code there is a loop, there is a mapping of new officers, and there's the mapping to the date. You can usually identify these kinds of things as you articulate the requirement(s) you're writing. To learn this you'll need to practice writing no code without one failing unit test. When your unit test gets huge you're probably trying to test more than one thing.

• Zero/One/Many

This is just an old guideline. I skipped zero here, because it does nothing, but when testing a loop you'll want to make sure you test the cases with an empty list, one entry, and many. I've most recently heard James Grenning articulate this but I don't believe he's the originator.

• One Mock Per Test

Can't remember where I got this guideline, it might be Roy Osherove but a mock contains assertions. Just as there should usually be one (logical) assertion per test, there should be one mock object per test. Mocking every collaborator means you're really just validated you implemented what you said you'd implement. It doesn't validate any of that is even slightly correct.

In fact in general I recommend newbies avoid mocks as much as possible. The kind of tests you wrote originally can lead to systems that run tests blinding fast - and with no idea if anything works yet.

Answer 5

Why not return the List instead of void and that list to _actionOfficerRepository in the Confirm() method (I don't know what is calling this method).

When your return type is the List instead of void it will be much easier to test this + it will be very handy for reusing this method from other actions.