New to TDD, am I doing everything right? How could I improve?

Question

I'm new to TDD, never used it ever. I understand the basic concepts but I'm now working on a small project which will be the first time I've ever actually used TDD.

The code is pretty self explanatory, it's a simple user data storage class. I've written an interface, a UserData class, and then implemented the interface with SimpleUserStore which uses lists.

I first wrote the tests, and then wrote the code so that they'd fail, and then "filled in" the code so that they would pass. When I realised I needed another method, I wrote the tests, then generated the stubs, then wrote the code so that they would pass.

My question is basically, am I doing this right? Are there things I'm missing out? Big gaping holes that make you go woah, wait a second!?

All responses appreciated.

---

public interface IUserStore
{        
    bool AddUser(UserData user);
    bool DeleteUser(UserData user);
    bool UpdateUser(UserData user);

    UserData GetUserById(int id);
    UserData GetUserByName(string name);
}

public class UserData
{
    public UserData() { }

    public virtual int UserId { get; set; }
    public virtual string Username { get; set; }
    public virtual string PasswordHash { get; set; }
    public virtual string PasswordSalt { get; set; }
}

public class SimpleUserStore : IUserStore, IDisposable
{
    private List<UserData> userdata;

    public SimpleUserStore()
    {
        userdata = new List<UserData>();
    }

    public bool AddUser(UserData user)
    {
        if (user == null) throw new ArgumentNullException("user");

        if (userdata.Contains(user)) return false;

        userdata.Add(user);
        return userdata.Contains(user);
    }

    public bool DeleteUser(UserData user)
    {
        if (user == null) throw new ArgumentNullException("user");
        return userdata.Remove(user);
    }

    public bool UpdateUser(UserData user)
    {
        if (user == null) throw new ArgumentNullException("user");
        if (!userdata.Contains(user)) return false;
        UserData _user = userdata.FirstOrDefault(x => x.UserId == user.UserId);
        _user = user;
        return true;
    }

    public UserData GetUserById(int id)
    {
        if (id < 0) throw new ArgumentException("id");
        return userdata.FirstOrDefault(x => x.UserId == id);
    }

    public UserData GetUserByName(string name)
    {
        if (name == null) throw new ArgumentNullException("name");
        return userdata.FirstOrDefault(x => x.Username == name);
    }        

    public void Dispose()
    {
        userdata = null;
    }
}

I have another project for tests, and in it I have the following test helper class:

[ExcludeFromCodeCoverage]
public static class TestHelper
{
    public static void ExpectException<T>(Action action) where T : Exception
    {
        try
        {
            action();
            Assert.Fail("Expected exception " + typeof(T).Name);
        }
        catch (T)
        {
            // Expected
        }
    }
}

And the tests:

[ExcludeFromCodeCoverage]
[TestClass]    
public class SimpleUserStoreTests
{
    private SimpleUserStore simpleUserStore;
    private UserData userA, userB;

    [TestInitialize]
    public void TestInitialize()
    {
        simpleUserStore = new SimpleUserStore();
        userA = new UserData { UserId = 1, Username = "UserA", PasswordHash = "", PasswordSalt = "" };
        userB = new UserData { UserId = 2, Username = "UserB", PasswordHash = "", PasswordSalt = "" };            
    }

    [TestCleanup]
    public void TestCleanup()
    {
        userA = null;
        userB = null;
        simpleUserStore.Dispose();
    }

    [TestMethod]
    public void AddUserTest()
    {                        
        Assert.IsTrue(simpleUserStore.AddUser(userA));            
    }

    [TestMethod]
    public void AddUserAlreadyExistsTest()
    {            
        simpleUserStore.AddUser(userA);
        Assert.IsFalse(simpleUserStore.AddUser(userA));
    }

    [TestMethod]
    public void AddUserNullArgumentTest()
    {            
        TestHelper.ExpectException<ArgumentNullException>(() => simpleUserStore.AddUser(null));
    }

    [TestMethod]
    public void DeleteUserTest()
    {                        
        simpleUserStore.AddUser(userA);
        Assert.IsTrue(simpleUserStore.DeleteUser(userA));            
    }

    [TestMethod]
    public void DeleteUserNullArgumentTest()
    {            
        TestHelper.ExpectException<ArgumentNullException>(() => simpleUserStore.DeleteUser(null));
    }

    [TestMethod]
    public void UpdateUserTest()
    {                        
        simpleUserStore.AddUser(userA);
        userA.Username = "Testing";
        Assert.IsTrue(simpleUserStore.UpdateUser(userA));
    }

    [TestMethod]
    public void UpdateUserNullArgumentTest()
    {            
        TestHelper.ExpectException<ArgumentNullException>(() => simpleUserStore.UpdateUser(null));
    }

    [TestMethod]
    public void UpdateUserNonExistTest()
    {                        
        Assert.IsFalse(simpleUserStore.UpdateUser(userA));
    }

    [TestMethod]
    public void GetUserByIdTest()
    {
        simpleUserStore.AddUser(userA);
        Assert.AreEqual(userA.UserId, simpleUserStore.GetUserById(1).UserId);
    }

    [TestMethod]
    public void GetUserByIdNegativeArgumentTest()
    {
        TestHelper.ExpectException<ArgumentException>(() => simpleUserStore.GetUserById(-20));
    }

    [TestMethod]
    public void GetUserByIdNonExistTest()
    {
        simpleUserStore.AddUser(userA);
        Assert.AreEqual(null, simpleUserStore.GetUserById(12));
    }

    [TestMethod]
    public void GetUserByNameTest()
    {                        
        simpleUserStore.AddUser(userA);            
        Assert.AreEqual(userA.UserId, simpleUserStore.GetUserByName("UserA").UserId);            
    }

    [TestMethod]
    public void GetUserByNameNullArgumentTest()
    {            
        TestHelper.ExpectException<ArgumentNullException>(() => simpleUserStore.GetUserByName(null));
    }

    [TestMethod]
    public void GetUserByNameNonExistTest()
    {                        
        simpleUserStore.AddUser(userA);
        Assert.AreEqual(null, simpleUserStore.GetUserByName("Testing"));
    }        
}

Answer 1

Your tests in general suffer from trusting your code too much:

[TestMethod]
public void DeleteUserTest()
{                        
    simpleUserStore.AddUser(userA);
    Assert.IsTrue(simpleUserStore.DeleteUser(userA));            
}

You verify the return value, but you don't verify that the user has been removed from the store. The object could very well return success without actually removing the object. You should have a call to GetUser() or similiar to verify that the user isn't there anymore.

Similar with update:

[TestMethod]
public void UpdateUserTest()
{                        
    simpleUserStore.AddUser(userA);
    userA.Username = "Testing";
    Assert.IsTrue(simpleUserStore.UpdateUser(userA));
}

You don't do anything to verify that update actually did the update. You trust the return value. But you shouldn't do that. Let's actually look at the implementation.

public bool UpdateUser(UserData user)
{
    if (user == null) throw new ArgumentNullException("user");
    if (!userdata.Contains(user)) return false;
    UserData _user = userdata.FirstOrDefault(x => x.UserId == user.UserId);

You don't verify that this works. You never pass a different object then the one you originally inserted in the Store. You need to pass another object to see if that works.

    _user = user;

You don't verify that the user in the store gets updated, so you aren't testing this line either. return true; }

Truth be told, I can see two bugs in this implementation. I'll leave it to you to fix your tests and actually find them yourselves. However, its bad enough that if you wrote this in an interview I probably wouldn't hire you. One of the bugs makes it look like you are very confused about the semantics of the language you are using. Maybe its just a careless error (they happen to the best of us), but it would give me grave doubts.

Answer 2

imho you are using return values in the wrong way.

For instance, the AddUser returns false for two different reasons which will make it harder to maintain the application. imho both cases are exceptional (the developer should make sure that the user do not exist before calling AddUser and the database is not likely to fail). Hence I would probably just have a void as return signature and use exceptions instead.

Same goes for removal. Using a bool indicates that the removal may fail as often as it will succeed. And that's not the case, is it?

As for the tests, there are some patterns which are typically used.

First of all I usually divide my tests accoring to the arrange/act/assert pattern to make the tests more readable:

[TestMethod]
public void DeleteUserTest()
{      
   // arrange                   
   var userA = new UserData { UserId = 1, Username = "UserA", PasswordHash = "", PasswordSalt = "" };
    simpleUserStore.AddUser(userA);

   // act 
    var actual = simpleUserStore.DeleteUser(userA);

   //assert
    Assert.IsTrue(actual);
}

Don't use method invocations in the asserts. It makes it harder to debug failing unit tests and also makes the tests harder to read.

Many devs also name the object being tested as sut (subject under test) to make it easier to spot what's being tested.

Answer 3

Your tests names aren't doing a good job of telling what is the test supposed to do.

AddUserAlreadyExistsTest() imho isn't very descriptive.

Try AddUser_GivenAlreadyExistingUser_ReturnsFalse(). The first part is the function I'm testing followed by the scenario I'm testing then finally the expected output. This way you can build your test backwards (i.e. starting with the assert then moving back to the point where you create the system object).

The point is someone else should read the name of the test and know what is being tested without actually reading inside the test. Think if it from the point of Clean Code, function names should tell what is happening while the code itself inside it should tell How it does it.

Answer 4

Looks ok to me. I would recommend that you make your test names more descriptive of what you expect to achieve, e.g. AddUserTest -> AddUserReturnsTrueIfSuccessful.

Answer 5

As indicated in other answers, the tests should definitely be named much more descriptively. It is common to use the list of tests as a requirements list for the component under test. and so should indicate The method tested, the preconditions, and the expected outcome. I like Songo's suggested formatting.

A number of the tests provided are problematic since you are only testing with one entry in your collection: GetUserByNameTest GetUserByIdTest

This alternate (incorrect) implementation of SimpleUserStore would still pass, but only because it is lucky. (your 'not exists' tests would fail though).

public interface IUserStore
{        
    bool AddUser(UserData user);
    bool DeleteUser(UserData user);
    bool UpdateUser(UserData user);

    UserData GetUserById(int id);
    UserData GetUserByName(string name);
}

public class UserData
{
    public UserData() { }

    public virtual int UserId { get; set; }
    public virtual string Username { get; set; }
    public virtual string PasswordHash { get; set; }
    public virtual string PasswordSalt { get; set; }
}

public class SimpleUserStore : IUserStore, IDisposable
{
    private List<UserData> userdata;

    public SimpleUserStore()
    {
        userdata = new List<UserData>();
    }

    public bool AddUser(UserData user)
    {
        if (user == null) throw new ArgumentNullException("user");

        if (userdata.Contains(user)) return false;

        userdata.Add(user);
        return userdata.Contains(user);
    }

    public bool DeleteUser(UserData user)
    {
        //if (user == null) throw new ArgumentNullException("user");
        //return userdata.Remove(user);
        return true;
    }

    public bool UpdateUser(UserData user)
    {
        if (user == null) throw new ArgumentNullException("user");
        if (!userdata.Contains(user)) return false;
        UserData _user = userdata.FirstOrDefault();
        _user = user;
        return true;
    }

    public UserData GetUserById(int id)
    {
        if (id < 0) throw new ArgumentException("id");
        return userdata.FirstOrDefault();
    }

    public UserData GetUserByName(string name)
    {
        if (name == null) throw new ArgumentNullException("name");
        return userdata.FirstOrDefault();
    }        

    public void Dispose()
    {
        userdata = null;
    }
}

This isn't directly related to the TDD question, but does illustrate your tests are not picking up on problems in your code: Your use of userdata.Contains() is potentially/probably problematic. Contains checks against reference. as you are using the same two objects throughout your test, the reference will be the same. what would you expect from this test?

[TestMethod]
public void ShouldUpdateBeIdBasedOrReferenceBased()
{                        
    simpleUserStore.AddUser(userA);
    UserData userCopy = new UserData{userA.Username, userA.UserId};
    userCopy.Username = "Testing";
    Assert.IsTrue(simpleUserStore.UpdateUser(userCopy));
}

as you are holding the reference for A the whole time, UpdateUser doesn't do anything useful for you the object in your store and the object in your test are the same object.

    simpleUserStore.AddUser(userA);

    userA.UserId = 7;
    UserData testResult = simpleUserStore.GetUserById(7);
    Assert.That(testResult.UserName, Is.EqualTo(userA.Username)) //victory.

for that matter...

    ...
    simpleUserStore.AddUser(userA);
    UserData userCopy = new UserData{userA.Username, userA.UserId};
    userCopy.Username = "Testing";
    simplerUserStore.AddUser(userCopy);
    ...

You now have two users with the same id but different names.

Testing against the boolean return values of your Add/Delete/Update methods is meaningless. all you test is that the method thinks it did the right thing.

It really is ok to have more than one Assert in a test...

[TestMethod]
public void UpdateUserTest()
{                        
    simpleUserStore.AddUser(userA);
    userA.Username = "Testing";
    simpleUserStore.UpdateUser(userA); //returning true tells me nothing 

    UserData result = simpleUserStore.GetById(userA.UserId);
    Assert.IsEqual(result.UserId, userA.UserId);
    Assert.IsEqual(result.Username, userA.Username);
}