# Unit test for a function that adds users

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]
{
AccountAccess accountActions = new AccountAccess();
Account testUser = new Account("abe",1000m);
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 + "'");
}


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]
{
//Arrange
AccountAccess accountActions = new AccountAccess();
Account testUser = new Account("abe",1000m);

//Act

//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]
{
//Arrange
AccountAccess accountActions = new AccountAccess();
Account expected = new Account("abe",1000m);

//Act

//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]
{
//Arrange
const string accountName = "abe";
const decimal accountLimit = 1000m;
AccountAccess accountActions = new AccountAccess();
Account testUser = new Account(accountName,accountLimit);

//Act

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

• 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! – Konrad Morawski Oct 26 '15 at 14:21
• +1 for the comment about testing whether object A is equal to object B rather than checking the individual fields – Dan Oct 26 '15 at 14:27
• Usually multiple asserts are frowned upon, but here is a blog article argueing how and when to do it. – holroy Dec 3 '15 at 16:58
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
{

public AccountAccess(IAccountRepository accountRepository)
{
_accountRepository = accountRepository;
}

{
//TODO Write your own complex logic here
}

{
}
}


Now your test could look like this:

[TestMethod]
{
IAccountRepository accountRepository = Substitute.For<IAccountRepository>();

AccountAccess accountActions = new AccountAccess(accountRepository);
Account testUser = new Account("abe", 1000m);

}


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")]

• +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. – abuzittin gillifirca Oct 26 '15 at 8:23