Is this UnitTest for updating an object in data-access layer sensible?

Question

I'm still trying to learn how to write good tests (and write testable code).

This is the function I want to verify,

 void IAutoTisDal.UpdateRange(Range range)
 {
 using (var repo = _repositoryFactory.GetRepository(_baseline))
 {
 var rangeqry = from r in repo.Query<Range>()
 where r.Id == range.Id
 select r;
 var rangeToUpdate = rangeqry.SingleOrDefault();
 if (rangeToUpdate == null)
 throw new Exception(string.Format("Range ID {0} not found", range.Id));
 rangeToUpdate.Status = range.Status;
 rangeToUpdate.FirstPsn = range.FirstPsn;
 rangeToUpdate.LastPsn = range.LastPsn;
 rangeToUpdate.PageCount = (int)(range.LastPsn - range.FirstPsn);
 rangeToUpdate.CourierId = range.CourierId;
 rangeToUpdate.WorkflowServer = range.WorkflowServer;
 rangeToUpdate.DoubleFeedInd = range.DoubleFeedInd;
 repo.Commit();
 }
 }

And the unit test:

 [TestMethod]
 public void UpdateRange_RangesPropertiesAreCopied()
 {
 var rangeToUpdate = new Range()
 {
 Id = 1, CourierId = 0, Status = 20,
 WorkflowServer = null, 
 FirstPsn = -1, LastPsn = -1, PageCount = 0
 };
 var rangeToSave = new Range()
 {
 Id = 1, CourierId = 1234, Status = 40,
 WorkflowServer = new PhysicalStation(){ Id = 1, Name = "fakeserver"},
 FirstPsn = 123, LastPsn = 456, PageCount = 333
 };
 var repoMock = new Mock<IRepository>();
 repoMock.Setup(x => x.Query<Range>()).Returns(new List<Range>(new[] {rangeToUpdate}).AsQueryable()).Verifiable();
 repoMock.Setup(x => x.Commit()).Verifiable();
 var factoryMock = new Mock<IRepositoryFactory>();
 factoryMock.Setup(x => x.GetRepository(It.IsAny<string>())).Returns(repoMock.Object);
 IAutoTisDal dal = new AutoTisDal(factoryMock.Object, "");
 dal.UpdateRange(rangeToSave);
 repoMock.VerifyAll();
 Assert.AreEqual(rangeToUpdate.Id, rangeToSave.Id);
 Assert.AreEqual(rangeToUpdate.CourierId, rangeToSave.CourierId);
 Assert.AreEqual(rangeToUpdate.LastPsn, rangeToSave.LastPsn);
 Assert.AreEqual(rangeToUpdate.FirstPsn, rangeToSave.FirstPsn);
 Assert.AreEqual(rangeToUpdate.PageCount, rangeToSave.LastPsn - rangeToSave.FirstPsn);
 Assert.AreSame(rangeToUpdate.WorkflowServer, rangeToSave.WorkflowServer);
 Assert.AreEqual(rangeToUpdate.Status, rangeToSave.Status);
 }

I know I need to write another test against the expected exception if no RangeId matches.

I also realize I'm writing test after the code.

• hometoast: side topic: Would IAutoTisDal be a considered service layer or a data access layer? This is a problem-domain-specific interface that calls my IRepository. The concrete IRepository uses NHibernate to exposes Query<T>() and Commit().

Answer 1

First of all, your single method is doing multiple things:

1. Get Range by id
2. Copy properties from one instance to another
3. Commit

Let's try to separate responsibilities:

 Range GetRangeById(Repository repo, int id)
 {
 return repo.Query<Range>().SingleOrDefault(r=>r.Id == id);
 }
 void UpdateRange(Range rangeToUpdate, Range range)
 {
 // I'd even make this Range extension method
 rangeToUpdate.Status = range.Status;
 rangeToUpdate.FirstPsn = range.FirstPsn;
 rangeToUpdate.LastPsn = range.LastPsn;
 rangeToUpdate.PageCount = (int)(range.LastPsn - range.FirstPsn);
 rangeToUpdate.CourierId = range.CourierId;
 rangeToUpdate.WorkflowServer = range.WorkflowServer;
 rangeToUpdate.DoubleFeedInd = range.DoubleFeedInd;
 }
 void IAutoTisDal.UpdateRange(Range range)
 {
 using (var repo = _repositoryFactory.GetRepository(_baseline))
 {
 var rangeToUpdate = GetRangeById(range.Id);
 if (rangeToUpdate == null)
 throw new Exception(string.Format("Range ID {0} not found", range.Id));
 UpdateRange(rangeToUpdate, range);
 repo.Commit();
 }
 }

Two new methods are already easier to understand and test. And, honestly, I don't think that writing test for IAutoTisDal.UpdateRange makes much sense. It won't provide much value.

• hometoast: Thanks for the input; It does make it a bit easier on the eyes. But why wouldn't the test provide much value? I added the test after realizing I forgot to set the DoubleFeedInd property (Range class has a couple dozen properties). Perhaps I could have written the test to include only my missing property.
• Pavel Tupitsyn: I mean that after proposed refactoring this last method becomes quite simple. As I understand, tests provide value in (1) verify that newly written code works and (2) verify that it did not break after changes. In our case, last method is straightforward and relies on other (tested) methods. What can be verified there? That is throws an exception, or really updates db?
• Pavel Tupitsyn: Regarding forgotten property: your unit test won't detect the same bug when someone adds one more property to the Range entity. I think some metadata (reflection or data model or whatever - I'm not familiar with NHibernate, but I solved similar problem with Entity Framework) should be involved to verify that right properties are being copied (all public writeable except identity, probably).
• hometoast: I agree about the way to verify all properties are mapped/writable. In this case, the value of the other properties aren't important.

Answer 2

I dunno if I would use reflection for the range mapping to properties. I think this would be a great instance for Func<> lambda expressions. Or perhaps a dictionary key value implementation that can be iterated through..

• Pavel Tupitsyn: Can you please give an example how lambdas or dictionary can help to retrieve all db-mapped properties?