Testing underlying classes with MOQ - input requested

Question

I'm working on a sync engine. I have a main engine class that gets two lists of PickLists from two providers (objects passed in as constructors). The method SyncPickLists() calls methods on the underlying objects (the two providers + a logger), an internal method to get a list of everything to do, and then does it as such:

  public class SyncEngine {
    public virtual ILoggingProvider Logger { get; private set; }
    public virtual ICrmProvider CrmProvider { get; private set; }
    public virtual ICacheProvider CacheProvider { get; private set; }

    public SyncEngine(ILoggingProvider loggingProvider, ICrmProvider crmProvider, ICacheProvider cacheProvider) {
      Logger = loggingProvider;
      CrmProvider = crmProvider;
      CacheProvider = cacheProvider;
    }

    public virtual void SyncPickLists() {
      Logger.LogBeginPicklistSync();

      // get all the pick lists from the local cache
      var localCachePickLists = CacheProvider.GetPickLists().ToList();
      // get all the pick lists from the remote system
      var crmPickLists = CrmProvider.GetPickLists().ToList();

      // build a sync plan
      var changes = BuildPickListUpdatePlan(localCachePickLists, crmPickLists).ToList();

      // run the sync
      RunPickListSync(changes);

      Logger.LogEndPicklistSync();
    }

    private IEnumerable<PickListAction> BuildPickListUpdatePlan(List<PickList> localCachePickLists, List<PickList> crmPickLists) {
      ...
    }
  }

I'm trying to use Moq to mock and test this starting with the sync engine, but running into issues checking ot see methods on the logger are called and evaluating the results that are generated from the BuildPickListUpdatePlan() method. I think this is because I'm trying to prime the providers and just call SyncPickLists()... which could be the wrong way to do this. Maybe I should make the BuildPickListUpdatePlan() method public/internal (and decoate the assembly appropriately) and just test that? Looking for input here as I'm new to mocking and not sure i"m going about this the right way.

Here's what my test looks like so far, but it isn't complete, correct or doing what I need it to do...

[TestMethod]
public void TestPickListSync() {
  // assign
  var _fakeCrmProvider = new Mock<ICrmProvider>().Object;
  var _fakeCacheProvider = new Mock<ICacheProvider>().Object;
  var _fakeLoggingProvider = new Mock<ILoggingProvider>().Object;
  var _fakeSyncEngine = new Mock<CustomerSyncEngine>(_fakeLoggingProvider, _fakeCrmProvider, _fakeCacheProvider).Object;

  // set picklists for CRM provider
  var crmList1 = new List<string>() { "AAA", "CCC", "DDD" };
  var crmList2 = new List<string>() { "WWW", "XXX", "YYY" };
  var crmPickLists = new List<PickList>() {
    new PickList(){ InternalName = "PickList1", DisplayName = "PickList1", Values = crmList1 },
    new PickList(){ InternalName = "PickList2", DisplayName = "PickList2", Values = crmList2 }
  };
  Mock.Get(_fakeCrmProvider).Setup(x => x.GetPickLists()).Returns(crmPickLists);
  Mock.Get(_fakeSyncEngine).SetupGet(x => x.CrmProvider).Returns(_fakeCrmProvider);

  // set picklists for cache provider
  var cacheList1 = new List<string>() { "AAA", "BBB", "CCC" };
  var cacheList2 = new List<string>() { "WWW", "XXX", "ZZZ" };
  var cachePickLists = new List<PickList>() {
    new PickList(){ InternalName = "PickList1", DisplayName = "PickList1", Values = cacheList1 },
    new PickList(){ InternalName = "PickList2", DisplayName = "PickList2", Values = cacheList2 }
  };
  Mock.Get(_fakeCacheProvider).Setup(x => x.GetPickLists()).Returns(cachePickLists);
  Mock.Get(_fakeSyncEngine).SetupGet(x => x.CacheProvider).Returns(_fakeCacheProvider);

  // act
  _fakeSyncEngine.SyncPickLists();

  // assert
  Mock.Get(_fakeLoggingProvider).Verify(x => x.LogBeginPicklistSync(), Times.Once());
  Mock.Get(_fakeLoggingProvider).Verify(x => x.LogEndPicklistSync(), Times.Once());
}

Answer 1

First of all you never mock the class under test. In this case you are testing SyncEngine, so you should not mock it.

If everything is mocked, you aren't testing anything. Mocks are there to abstract away dependencies in your tests and to facilitate writing tests.

You abstract away dependencies to:

• Concentrate your tests on the code you want to test
• Abstract away dependencies on things like databases, file systems, other processes, etc.
• Simplify tests by reducing the code needed to test a particular feature or by easily getting a desired value from an object.

Secondly, always mock things for a reason, always consider if each class should or should not be mocked.

Third, it is considered bad practice by some (including myself) to test private methods. BuildPickListUpdatePlan is private, you should rather test the public methods of your class that use BuildPickListUpdatePlan.

Testing private methods will often require "tricks" to test correctly and result in tests that are very brittle. From my experience, these tests are more prone to needing to be changed rather than tests on the public interface or API of your class. Unless you have private methods that aren't used (in which case they are dead code and be deleted rather than tested), all your private methods will get called by your public methods. Concentrate on testing these.

I do not know the details of your code, so my proposition might be off, but here is what I would propose your test code could look like:

[TestMethod]
public void TestPickListSync() {
  // Consider if each of these should be mocked, in this case the answer seems to be yes
  var _fakeCrmProvider = new Mock<ICrmProvider>();
  var _fakeCacheProvider = new Mock<ICacheProvider>();
  var _fakeLoggingProvider = new Mock<ILoggingProvider>();

  // Don't mock the class under test
  var syncEngine = SyncEngine(_fakeLoggingProvider.Object, _fakeCrmProvider.Object, _fakeCacheProvider.Object);

  // 
  var crmList1 = new List<string>() { "AAA", "CCC", "DDD" };
  var crmList2 = new List<string>() { "WWW", "XXX", "YYY" };
  var crmPickLists = new List<PickList>() {
    new PickList(){ InternalName = "PickList1", DisplayName = "PickList1", Values = crmList1 },
    new PickList(){ InternalName = "PickList2", DisplayName = "PickList2", Values = crmList2 }
  };

  // Since your class under test is already using the mocks, there 
  // is no need to "mock.get", just mock the methods that will be called on the mocked objects
  _fakeCrmProvider.Setup(x => x.GetPickLists()).Returns(crmPickLists);

  // set picklists for cache provider
  var cacheList1 = new List<string>() { "AAA", "BBB", "CCC" };
  var cacheList2 = new List<string>() { "WWW", "XXX", "ZZZ" };
  var cachePickLists = new List<PickList>() {
    new PickList(){ InternalName = "PickList1", DisplayName = "PickList1", Values = cacheList1 },
    new PickList(){ InternalName = "PickList2", DisplayName = "PickList2", Values = cacheList2 }
  };

  // Since your class under test is already using the mocks, there 
  // is no need to "mock.get", just mock the methods that will be called on the mocked objects
  _fakeCacheProvider.Setup(x => x.GetPickLists()).Returns(cachePickLists);


  // act
  _syncEngine.SyncPickLists();

  // You can do your asserts like this
  _fakeLoggingProvider.Verify(x => x.LogBeginPicklistSync(), Times.Once());
  _fakeLoggingProvider.Verify(x => x.LogEndPicklistSync(), Times.Once());
}

I don't have VS at hand, so I couldn't check the syntax, but it should give you some ideas. I have replaced your comments with explanations.

Here is some simple code that show a basic example of mocking a dependency:

[TestMethod]
public void SampleTestMethod() {
    // Arrange
    var mockMyLoggingClass = new Mock<IMyLogging>();

    var classUnderTest = new ClassUnderTest(mockMyLoggingClass.Object);

    mockMyLoggingClass.Setup(mock = > mock.MethodThatWillGetCalled()).Returns(someValue)

    // Act
    var result = classUnderTest.MethodThatWillCallLoggingClass();

    // Assert
    Assert.AreEqual(expectedValue, result); // we can still do normal asserts apart from the verify
    mockMyLoggingClass.Verify(mock => mock.MethodThatWillGetCalled(), Times.Once());
}