Unit test for a function that adds users

Question

I'm new to unit testing and I'm curious if anyone can see any problems with my approach below. Basically I want to test the Add function. To do this I am creating a new Account object, passing that to the Add function, retrieving that account and finally comparing the retrieved objects values to hard coded values.

I'm wondering if anyone can see any thing that would not be considered best practice for unit tests? Specifically, should I be using the testUser object in my asserts rather than the hard coded values? Anything else I should consider?

[TestMethod]
public void TestAddUser()
{
    AccountAccess accountActions = new AccountAccess();
    Account testUser = new Account("abe",1000m);
    accountActions.Add(testUser);
    Account retrievedUser = accountActions.GetAccount("abe");
    Assert.AreEqual("abe", retrievedUser.name, "Added user does not have corret name: 'abe' - '" + retrievedUser.name + "'");
    Assert.AreEqual(1000m, retrievedUser.limit, "Added user does not have corret limit: '1000' - '" + retrievedUser.limit + "'");
}

Answer 1

A unit test is split in 3 steps. Arrange-Act-Assert. Usually, to make it clear what is being tested, you write as a comment in which step you're at.

[TestMethod]
public void TestAddUser()
{
    //Arrange
    AccountAccess accountActions = new AccountAccess();
    Account testUser = new Account("abe",1000m);

    //Act
    accountActions.Add(testUser);

    //Assert
    Account retrievedUser = accountActions.GetAccount("abe");
    Assert.AreEqual("abe", retrievedUser.name, "Added user does not have corret name: 'abe' - '" + retrievedUser.name + "'");
    Assert.AreEqual(1000m, retrievedUser.limit, "Added user does not have corret limit: '1000' - '" + retrievedUser.limit + "'");
}

Now, to make your test as good as possible, it needs to be as small as possible but still crystal clear about what you're testing.

What are your Assert testing? If the name and limits are equals. What do you want your `Assert to test? That the same account that was added is returned. That both accounts are equal.

It's not a big difference, but what if one day the equality of an Account is based on an Id? You might need to refactor this test because the assertions would be false. Ex : If I add twice the same Account with the same name and limit, which is supposed to be the one that is returned?

What I mean is, your test should look like this :

[TestMethod]
public void AddUser()
{
    //Arrange
    AccountAccess accountActions = new AccountAccess();
    Account expected = new Account("abe",1000m);

    //Act
    accountActions.Add(new Account("abe",1000m));

    //Assert
    Account actual = accountActions.GetAccount("abe");
    Assert.AreEqual(expected, actual);
}

Now, this test might fail, if so, it's because your Account class doesn't override the Equal and GetHashCode methods. It probably should as they both define the identity of your object.

Let's get back to my last example. What is different from your test?

• Expected/Actual : This way, I know what are the expected results (I have a variable for that) and what was actually returned (I also have a variable for that!)
• Only one assertion. It's a best practice, you can't always achieve one assertion, but that should be your goal. This way, it's easy for anyone to understand what your test... tests. It's now very clear I'm testing that it's the same account that is returned, because internally Assert.AreEqual will use the Equal property of your class. And it seems reasonable that the Account class should know if it's equal to another much better than your unit test.

Now, if you don't want to override the equals, there's another solution for you. Use of constants

Ex :

[TestMethod]
public void AddUser()
{
    //Arrange
    const string accountName = "abe";
    const decimal accountLimit = 1000m;
    AccountAccess accountActions = new AccountAccess();
    Account testUser = new Account(accountName,accountLimit);

    //Act
    accountActions.Add(testUser);

    //Assert
    Account retrievedUser = accountActions.GetAccount(accountName);
    Assert.AreEqual(accountName, retrievedUser.name);
    Assert.AreEqual(accountLimit, retrievedUser.limit);
}

This way, there's no confusion about "abe" being everywhere, heck, I don't even need to know what is inside the accountName variable, I just need to know it is equals to the retrieved one because I used it to retrieve the account. Actually, the use of constants should be heavily considered in my first example too!!

Note : The string messages, aren't necessary. The default assertion message for Assert.AreEqual will be very clear that the actual value isn't equal to the expected one. And your test name shouldn't include Test. You know it's a test, there's the attribute juste above :) (You should take a look at @Konrad's comment about naming!)

Answer 2

1. Unit test should test one and only one thing

   The above test might be categorized into integration testing, not unit testing. I am assuming AccountAccess will have some external dependency and will look like this in the final version:

   public class AccountAccess
   {
   private readonly IAccountRepository _accountRepository;
   
   public AccountAccess(IAccountRepository accountRepository)
   {
       _accountRepository = accountRepository;
   }
   
   public void Add(Account account)
   {
       //TODO Write your own complex logic here
       _accountRepository.Add(account);
   }
   
   public Account GetAccount(string userName)
   {
       return _accountRepository.Get(userName);
   }
   }
   

   Now your test could look like this:

   [TestMethod]
   public void ShouldAddUser()
   {
       IAccountRepository accountRepository = Substitute.For<IAccountRepository>();
   
       AccountAccess accountActions = new AccountAccess(accountRepository);
       Account testUser = new Account("abe", 1000m);
       accountActions.Add(testUser);
   
      accountRepository.Received(1).Add(testUser);
   }
   

   Now this unit test is testing only one thing about adding user only, mocking out external dependencies IAccountRepository, and putting an assert about it.

   Same thing should be done for other methods too. I am using NSubstitute for mocking dependencies.

   You should also consider writing integration tests which will cover both methods in one go after writing your unit test (no mocking in this test).

2. Naming convention

   name and limit are not a suggested naming convention. Always use PascalCase for properties and methods. private variable should be camelCase.

3. Use TestCategory to categorize test

   [TestMethod]
   [TestCategory("Unit")]
   public void ShouldAddUser()
   {
   
   }