Do I unit test the correct thing

Question

Thats my method to test:

 public LessonplannerRequest GetWeeklyLessonplanner(int schoolyearId) 
 { 
 Schoolyear schoolyear = _unitOfWork.SchoolyearRepository.GetById(schoolyearId);
 DayOfWeek firstDayOfWeek = schoolyear.FirstDayOfWeek;
 DateTime currentDate = DateTime.Now.Date;
 DateTime startDateOfWeek = _dateService.GetFirstDateOfWeek(currentDate, firstDayOfWeek);
 DateTime endDateOfWeek = _dateService.GetLastDateOfWeek(currentDate, firstDayOfWeek);
 var periods = _unitOfWork.PeriodRepository.GetWeeklyPeriods(schoolyearId, startDateOfWeek, endDateOfWeek );
 var weeks = _dateService.GetAllWeekStartingDays(startDateOfWeek, endDateOfWeek, firstDayOfWeek);
 var request = new LessonplannerRequest
 {
 Periods = periods,
 Weeks = weeks,
 };
 return request;
 }

Thats my unit test:

 // Arrange
 Mock<IUnitOfWork> unitOfWorkMock = new Mock<IUnitOfWork>();
 Mock<IDateService> dateServiceMock = new Mock<IDateService>();
 Mock<TLPContext> contextMock = new Mock<TLPContext>();
 Mock<IGenericRepository<Schoolyear>> schoolyearRepositoryMock = new Mock<IGenericRepository<Schoolyear>>();
 Mock<ILessonplannerRepository> lessonplannerRepoMock = new Mock<ILessonplannerRepository>();
 int schoolyearId = 1;
 DateTime start = new DateTime(2010, 1, 1);
 DateTime end = new DateTime(2010, 1, 2);
 unitOfWorkMock.Setup(s => s.SchoolyearRepository).Returns(schoolyearRepositoryMock.Object);
 unitOfWorkMock.Setup(s => s.PeriodRepository).Returns(lessonplannerRepoMock.Object);
 dateServiceMock.Setup(s => s.GetFirstDateOfWeek(DateTime.Now.Date, DayOfWeek.Sunday)).Returns(start);
 dateServiceMock.Setup(s => s.GetLastDateOfWeek(DateTime.Now.Date, DayOfWeek.Sunday)).Returns(end);
 schoolyearRepositoryMock.Setup(s => s.GetById(schoolyearId)).Returns(new Schoolyear() { FirstDayOfWeek = DayOfWeek.Sunday });
 ILessonplannerService service = new LessonplannerService(unitOfWorkMock.Object, dateServiceMock.Object); 
 // Act
 LessonplannerRequest request = service.GetWeeklyLessonplanner(schoolyearId);
 // Assert 
 dateServiceMock.Verify(d => d.GetFirstDateOfWeek(DateTime.Now.Date, DayOfWeek.Sunday));
 dateServiceMock.Verify(d => d.GetLastDateOfWeek(DateTime.Now.Date, DayOfWeek.Sunday));
 lessonplannerRepoMock.Verify(d => d.GetWeeklyPeriods(schoolyearId, start, end));
 Assert.That(request.Periods, Is.Empty);
 Assert.That(request.Weeks, Is.Empty); 

In my method to test all calls to a repository or dateService are already unit tested itself thus I am mocking them not to have an integration test...

I ask myself wether my assertion for empty lists make sense. And If not what would you test. Actually in this method everything is already tested just the creation of the LessonplannerRequest is not tested...

• TeaDrivenDev: My very first impression is that your GetWeeklyLessonPlanner() method and its class might be doing too much. How much more than the method to test is there in the class? Is it just that method, the constructor and dependency fields? Generally try and post complete classes; that will make it easier for others to get the code working and provide assistance.
• Elisa: There is still a SaveLessonplanner() method with 5 lines of code. Why do you have the impression, that my class is doing too much?
• TeaDrivenDev: Just a first thought when I saw that IUnitOfWork is actually hiding an additional dependency of your method. I'm still figuring out what's actually happening.
• Elisa: "I'm still figuring out what's actually happening" Then when did you your last mocking parade? ;P
• TeaDrivenDev: A few weeks back probably. My confusion wasn't so much about the test/mocks, but more about what the code under test actually does (and I still don't understand the greater purpose).

Answer 1

In addition to almaz' correct answer (except for using SetUp in NUnit, which is risky and can have dangerous side effects), I wouldn't test for empty collections, because those can come from anywhere and could well be the default for the properties of LessonplannerRequest.

Instead, set up your mocks (which are now actually stubs because we're not verifying the calls to their methods) to return specific objects for the correct method invocations, and then assert that those objects are returned in your request object:

[Test]
public void GetWeeklyLessonPlanner_ReturnsCorrectRequest()
{
 // Arrange
 Mock<IUnitOfWork> unitOfWorkMock = new Mock<IUnitOfWork>();
 Mock<IDateService> dateServiceMock = new Mock<IDateService>();
 Mock<IGenericRepository<Schoolyear>> schoolyearRepositoryMock = new Mock<IGenericRepository<Schoolyear>>();
 Mock<ILessonplannerRepository> lessonplannerRepoMock = new Mock<ILessonplannerRepository>();
 int schoolyearId = 1;
 DateTime start = new DateTime(2010, 1, 1);
 DateTime end = new DateTime(2010, 1, 2);
 unitOfWorkMock.Setup(s => s.SchoolyearRepository).Returns(schoolyearRepositoryMock.Object);
 unitOfWorkMock.Setup(s => s.PeriodRepository).Returns(lessonplannerRepoMock.Object);
 dateServiceMock.Setup(s => s.GetFirstDateOfWeek(DateTime.Now.Date, DayOfWeek.Sunday)).Returns(start);
 dateServiceMock.Setup(s => s.GetLastDateOfWeek(DateTime.Now.Date, DayOfWeek.Sunday)).Returns(end);
 var dayOfWeek = DayOfWeek.Sunday;
 schoolyearRepositoryMock.Setup(s => s.GetById(schoolyearId)).Returns(new Schoolyear() { FirstDayOfWeek = dayOfWeek });
 var expectedPeriods = new List<object>() { new object() };
 lessonplannerRepoMock.Setup(r => r.GetWeeklyPeriods(schoolyearId, start, end)).Returns(expectedPeriods);
 var expectedWeeks = new List<object>() { new object() };
 dateServiceMock.Setup(d => d.GetAllWeekStartingDays(start, end, dayOfWeek)).Returns(expectedWeeks);
 LessonplannerService service = new LessonplannerService(unitOfWorkMock.Object, dateServiceMock.Object);
 // Act
 LessonplannerRequest request = service.GetWeeklyLessonplanner(schoolyearId);
 // Assert
 Assert.That(request.Periods, Is.SameAs(expectedPeriods));
 Assert.That(request.Weeks, Is.SameAs(expectedWeeks));
}

This will make sure that your stub objects' methods are called with the correct parameters, because the expected objects are only returned for those specific parameter combinations, and at the same time guarantee that those values are correctly used further on, because they are particular object references that the test can explicitly recognize.

Note that the List<object> will probably have to be something else in your actual code. Your code fragments didn't contain types for Periods and Weeks, so I used IEnumerable<object> to work with.

• almaz: Can you shed a light on side effects of using SetUp? I'm using it for about 8 years now and haven't noticed anything dangerous...
• TeaDrivenDev: Then you probably have a very tidy approach that doesn't let those issues come to bear. Many people will not have that. This is from James Newkirk, who led NUnit development for years before starting xUnit.net because he became dissatisfied with NUnit: jamesnewkirk.typepad.com/posts/2007/09/why-you-should-.html
• Elisa: @GCATNM Its an IEnumerable<Period> periods... :)

Answer 2

Almost everything is perfect. Some notes:

• Do use var to avoid repeating in cases like Mock<IUnitOfWork> unitOfWorkMock = new Mock<IUnitOfWork>()
• Move mocks initialisation and wiring up to SetUp method to avoid copy-pasting in different test methods, and leave only actual setups that provide test data in test method
• It's hard to unit test the code that uses DateTime.Now. For example your test may fail when running near midnight (since you call DateTime.Now several times, and subsequent calls may return the next day). I would suggest to add the UtcNow property to your IDateService and always use it instead of DateTime.Now (I prefer to force using UTC time since it forces you to think about time zones). Then, most likely you won't need methods like GetFirstDateOfWeek in IDateService (if they depend on inputs only), they could be just extension methods to DateTime.
• You don't need to verify that GetFirstDateOfWeek and GetLastDateOfWeek were called, just make sure that result matches expected value. The reason is because the call to those methods does not guarantee that returned values were used correctly. And when all tests pass without those calls - it only shows that you should add more tests to verify that returned values were used properly

As a result you'll have something like this:

[Test]
public void WhenWeekStartOnSunday...ShouldReturnSomething
{
 const int schoolyearId = 1;
 var currentDate = new DateTime(2010, 1, 1).ToUniversalTime();
 var periods = new [] {...}; //specify test periods here
 //Arrange
 _schoolyearRepositoryMock.Setup(s => s.GetById(schoolyearId)).Returns(new Schoolyear() { FirstDayOfWeek = DayOfWeek.Sunday }); 
 _dateServiceMock.Setup(s => s.UtcNow).Returns(currentDate);
 _lessonplannerRepoMock.Setup(lp => lp.GetWeeklyPeriods(schoolyearId, new DateTime(2010, 1, 1), 
 new DateTime(2010, 1, 5))) //specify the dates expected according to "currentDate" value.
 .Returns(periods);
 var service = new LessonplannerService(_unitOfWorkMock.Object, _dateServiceMock.Object); 
 // Act
 LessonplannerRequest request = service.GetWeeklyLessonplanner(schoolyearId);
 // Assert 
 Assert.That(request.Periods, Is.SameAs(periods));
 Assert.That(request.Weeks, Is.Empty); //verify here expected list of weeks according to logic of the method
}

• Jeff Vanzella: I'd go one further and say that schoolyearId should be const.
• Elisa: @almaz "The reason is because the call to those methods does not guarantee that returned values were used correctly" But the call to those methods guarantee that the methods are called at all. What do you think?
• almaz: @Elisa what if you find the way to avoid calling those methods and yet produce proper results? Then you would have to change unit tests because they started to fail. If values returned by e.g. GetFirstDateOfWeek are actually valuable, then in certain use cases output will depend on them, and there should be a test that checks it. If there is no way to verify the proper usage of GetFirstDateOfWeek... then it does not influence results, and it means that you don't need to call it :).
• almaz: You need to verify mock method calls when it is the function being tested, e.g. when functionality under test should send an email you would write assertion as _emailSenderMock.Verify(e = > e.SendEmail(expected_parameters));
• almaz: Updated the answer to verify that periods are passed through