HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thislayerupdatingunittestsensibleforobjectdataaccess

Problem

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()
                       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();
repoMock.Setup(x => x.Query()).Returns(new List(new[] {rangeToUpdate}).AsQueryable()).Verifiable();
repoMock.Setup(x => x.Commit()).Verifiable();
var factoryMock = new Mock();
factoryMock.Setup(x => x.GetRepository(It.IsAny())).Returns(repoMock.Object);
IAutoTisDal dal = new AutoTisDal(factoryMock.Object, "");

dal.UpdateRange(rangeToSave);

repoMock.VerifyAll();

Assert.AreEqual(rangeToUpdate.Id, rangeToSave.Id);
Assert.AreEqual(rangeToUpdate.Co

Solution

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

  • Get Range by id



  • Copy properties from one instance to another



  • Commit



Let's try to separate responsibilities:

Range GetRangeById(Repository repo, int id)
 {
    return repo.Query().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.

Code Snippets

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();
     }
  }

Context

StackExchange Code Review Q#18541, answer score: 5

Revisions (0)

No revisions yet.