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