How could I refactor this factory-type method and database call to be testable?

Question

I'm trying to learn how to do Unit Testing and Mocking. I understand some of the principles of TDD and basic testing. However, I'm looking at refactoring the below code that was written without tests and am trying to understand how it needs to change in order to make it testable.

public class AgentRepository
{

public Agent Select(int agentId)
{
    Agent tmp = null;
    using (IDataReader agentInformation = GetAgentFromDatabase(agentId))
    {
        if (agentInformation.Read())
        {
            tmp = new Agent();
            tmp.AgentId = int.Parse(agentInformation["AgentId"].ToString());
            tmp.FirstName = agentInformation["FirstName"].ToString();
            tmp.LastName = agentInformation["LastName"].ToString();
            tmp.Address1 = agentInformation["Address1"].ToString();
            tmp.Address2 = agentInformation["Address2"].ToString();
            tmp.City = agentInformation["City"].ToString();
            tmp.State = agentInformation["State"].ToString();
            tmp.PostalCode = agentInformation["PostalCode"].ToString();
            tmp.PhoneNumber = agentInformation["PhoneNumber"].ToString();
        }
    }

    return tmp;
}

private IDataReader GetAgentFromDatabase(int agentId)
{
    SqlCommand cmd = new SqlCommand("SelectAgentById");
    cmd.CommandType = CommandType.StoredProcedure;

    SqlDatabase sqlDb = new SqlDatabase("MyConnectionString");
    sqlDb.AddInParameter(cmd, "AgentId", DbType.Int32, agentId);
    return sqlDb.ExecuteReader(cmd);
}

}

These two methods are in a single class. The database-related code in the GetAgentFromDatabase is related to Enterprise Libraries.

How would I be able to go about making this testable? Should I abstract out the GetAgentFromDatabase method into a different class? Should GetAgentFromDatabase return something other than an IDataReader? Any suggestions or pointers to external links would be greatly appreciated.

Answer 1

I'll start putting up some ideas and will update along the way:

• SqlDatabase sqlDb = new SqlDatabase("MyConnectionString"); - You should avoid new operators mixed up with logic. You should construct xor have logic operations; avoid them happening at the same time. Use Dependency injection to pass this database as a parameter, so you can mock it. I mean this if you want to unit test it (not going to the database, which should be done in some case later)
• IDataReader agentInformation = GetAgentFromDatabase(agentId) - Maybe you could separate the Reader retrieval to some other class, so you can mock this class while testing the factory code.

Answer 2

IMO you should normally only worry about making your public properties/methods testable. I.e. as long as Select(int agentId) works you normally don't care how it does it via GetAgentFromDatabase(int agentId).

What you have seems reasonable, as I imagine it can be tested with something like the following (assuming your class is called AgentRepository)

AgentRepository aRepo = new AgentRepository();
int agentId = 1;
Agent a = aRepo.Select(agentId);
//Check a here

As for suggested enhancements. I would recommend allowing the AgentRepository's connection string to be changed, either by public or internal access.

Answer 3

Assuming that you're trying to test the public Select method of class [NoName]..

1. Move the GetAgentFromDatabase() method into an interface say IDB_Access. Let NoName have an interface member that can be set as a ctor parameter or a property. So now you have a seam, you can change the behavior without modifying the code in the method.
2. I'd change the return type of the above method to return something more general - you seem to be using it like a hashtable. Let the production implementation of IDB_Access use the IDataReader to create the hashtable internally. It also makes it less technology dependent ; I can implement this interface using MySql or some non-MS/.net environment. private Hashtable GetAgentFromDatabase(int agentId)
3. Next for your unit test, you could work with a stub (or use something more advanced like a mock framework)

.

public MockDB_Access : IDB_Access
{
  public const string MY_NAME = "SomeName;
  public Hashtable GetAgentFromDatabase(int agentId)
  {  var hash = new Hashtable();
     hash["FirstName"] = MY_NAME; // fill other properties as well
     return hash;
  }
}

// in the unit test
var testSubject = new NoName( new MockDB_Access() );
var agent = testSubject.Select(1);
Assert.AreEqual(MockDB_Access.MY_NAME, agent.FirstName); // and so on...

Answer 4

As for my opinion the GetAgentFromDatabase() method must not be testet by an extra test, because its code is fully covered by the test of the Select() method. There are no branches the code could walk along, so no point in creating an extra test here. If the GetAgentFromDatabase() method is called from multiple methods you should test it on its own though.

Answer 5

The problem here is deciding what is SUT and what is Test. With your example you are trying to Test the Select() method and therefore want to isolate that from the database. You have several choices,

1. Virtualise the GetAgentFromDatabase() so you can provide a derived class with code to return the correct values, in this case creating an object that provides IDataReaderFunctionaity without talking to the DB i.e.

   class MyDerivedExample : YourUnnamedClass
   {
       protected override IDataReader GetAgentFromDatabase()
       {
           return new MyDataReader({"AgentId", "1"}, {"FirstName", "Fred"},
             ...);
       }
   }
   

2. As Gishu suggested instead of using IsA relationships (inheritance) use HasA (object composition) where you once again have a class that handles creating a mock IDataReader, but this time without inheriting.

   However both of these result in lots of code that simply defines a set of results that we be returned when queried. Admittedly we can keep this code in the Test code, instead of our main code, but its an effort. All you are really doing is define a result set for particular queries, and you know what’s really good at doing that... A database

3. I used LinqToSQL a while back and discovered that the DataContext objects have some very useful methods, including DeleteDatabase and CreateDatabase.

   public const string UnitTestConnection = "Data Source=.;Initial Catalog=MyAppUnitTest;Integrated Security=True";
   
   
   [FixtureSetUp()]
   public void Setup()
   {
     OARsDataContext context = new MyAppDataContext(UnitTestConnection);
   
   
     if (context.DatabaseExists())
     {
       Console.WriteLine("Removing exisitng test database");
       context.DeleteDatabase();
     }
     Console.WriteLine("Creating new test database");
     context.CreateDatabase();
   
   
     context.SubmitChanges();
   }
   

Consider it for a while. The problem with using a database for unit tests is that the data will change. Delete your database and use your tests to evolve your data that can be used in future tests.

There are two things to be careful of Make sure your tests run in the correct order. The MbUnit syntax for this is [DependsOn("NameOfPreviousTest")]. Make sure only one set of tests is running against a particular database.

Answer 6

You're correct about moving GetAgentFromDatabase() into a separate class. Here's how I redefined AgentRepository:

public class AgentRepository {
 private IAgentDataProvider m_provider;

 public AgentRepository( IAgentDataProvider provider ) {
  m_provider = provider;
 }

 public Agent GetAgent( int agentId ) {
  Agent agent = null;
  using( IDataReader agentDataReader = m_provider.GetAgent( agentId ) ) {
   if( agentDataReader.Read() ) {
    agent = new Agent();
                // set agent properties later
   }
  }
  return agent;
 }
}

where I defined the IAgentDataProvider interface as follows:

public interface IAgentDataProvider {
 IDataReader GetAgent( int agentId );
}

So, AgentRepository is the class under test. We'll mock IAgentDataProvider and inject the dependency. (I did it with Moq, but you can easily redo it with a different isolation framework).

[TestFixture]
public class AgentRepositoryTest {
 private AgentRepository m_repo;
 private Mock<IAgentDataProvider> m_mockProvider;

 [SetUp]
 public void CaseSetup() {
  m_mockProvider = new Mock<IAgentDataProvider>();
  m_repo = new AgentRepository( m_mockProvider.Object );
 }

 [TearDown]
 public void CaseTeardown() {
  m_mockProvider.Verify();
 }

 [Test]
 public void AgentFactory_OnEmptyDataReader_ShouldReturnNull() {
  m_mockProvider
   .Setup( p => p.GetAgent( It.IsAny<int>() ) )
   .Returns<int>( id => GetEmptyAgentDataReader() );
  Agent agent = m_repo.GetAgent( 1 );
  Assert.IsNull( agent );
 }

 [Test]
 public void AgentFactory_OnNonemptyDataReader_ShouldReturnAgent_WithFieldsPopulated() {
  m_mockProvider
   .Setup( p => p.GetAgent( It.IsAny<int>() ) )
   .Returns<int>( id => GetSampleNonEmptyAgentDataReader() );
  Agent agent = m_repo.GetAgent( 1 );
  Assert.IsNotNull( agent );
                    // verify more agent properties later
 }

 private IDataReader GetEmptyAgentDataReader() {
  return new FakeAgentDataReader() { ... };
 }

 private IDataReader GetSampleNonEmptyAgentDataReader() {
  return new FakeAgentDataReader() { ... };
 }
}

(I left out the implementation of class FakeAgentDataReader, which implements IDataReader and is trivial -- you only need to implement Read() and Dispose() to make the tests work.)

The purpose of AgentRepository here is to take IDataReader objects and turn them into properly formed Agent objects. You can expand the above test fixture to test more interesting cases.

After unit-testing AgentRepository in isolation from the actual database, you will need unit tests for a concrete implementation of IAgentDataProvider, but that's a topic for a separate question. HTH