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!)

• Konrad Morawski: I'd advocate for using more verbose naming for unit tests. "AddUser" doesn't tell me much about what behavior is being tested, there's many things we expect from AddUser method - rejecting to add an invalid user, not throwing (or throwing) an exception when the same user is added twice, triggering an error on a null user etc. osherove.com/blog/2005/4/3/naming-standards-for-unit-tests.html I think that's the way to go: [UnitOfWork_StateUnderTest_ExpectedBehavior]. Unit test methods are not ordinary methods, we don't call them by hand so everyday naming guidelines do not apply here!
• IEatBagels: @KonradMorawski 100% on your side about this. I thought about adding it to my answer, but I didn't, by lack of time I guess. I'll add a small paragraph to tell OP to read your link.
• Dan: +1 for the comment about testing whether object A is equal to object B rather than checking the individual fields
• holroy: Usually multiple asserts are frowned upon, but here is a blog article argueing how and when to do it.

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

• abuzittin gillifirca: This just bakes the current implementation in. Whereas, OP's test ensures the following property of the Unit under test: "An Account Access gets me a User of a given name, provided I added that user before."
• Paritosh: @abuzittingillifirca Unit test should not be correlated, It should just test only one method not two .
• abuzittin gillifirca: +1 for other points. But think how many of this kind of tests would fail altogether if I deleted an assignment from the Account constructor. Now, that's some correlation.
• Paritosh: @abuzittingillifirca: If you are changing anything in your code base ,ideally your test must fail. That is whole point of unit testing. In above case you can create a method which return an object of the Account class , so changes will need to be made one place only
• David Arno: @paritosh, unit testing is not about "test[ing] one method not two". It's all about test isolation, ie no test having any affect on any other. Multiple unit tests must be able to run in parallel; in any order. Whether they test one method or a couple of inter-related classes isn't important. Whether they contain one or many asserts isn't important. eg see martinfowler.com/bliki/UnitTest.html