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.

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.

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..