Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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"));
    }        
}
share|improve this question
2  
One thing I notice off hand is you might find it easier to use the ExpectedExceptionAttribute –  hometoast Feb 4 '13 at 15:39
    
Thanks, that's exactly the kind of feedback I am looking for! :) –  Adam K Dean Feb 4 '13 at 15:43
2  
Your test cases could be a little more robust. For example, instead of only testing that AddUser returns true, you should ensure that it actually added the user. Other than that, it sounds like you have the process down. –  Ginosaji Feb 4 '13 at 15:46
1  
@AdamKDean, don't count return values that simply indicate success towards that one assert. There is a good chance they should be exceptions anyways. –  Winston Ewert Feb 4 '13 at 16:13
1  
I would recommend using code.google.com/p/moq for easy creating moq objects –  Jocke Feb 11 '13 at 8:56
show 2 more comments

migrated from stackoverflow.com Feb 4 '13 at 15:34

This question came from our site for professional and enthusiast programmers.

5 Answers

up vote 5 down vote accepted

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.

share|improve this answer
    
Thank you for your honest appraisal/review. This is exactly what I was looking for, and the exact reason why I'm writing this here now, and not in an interview. I sincerely appreciate your feedback! :) –  Adam K Dean Feb 4 '13 at 16:21
add comment

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.

share|improve this answer
    
Thanks for the feedback, what you say makes absolute sense. Would you say I should initialise the UserData objects in each test or in the initialization class? I felt like I was repeating myself a lot with that particular line. –  Adam K Dean Feb 4 '13 at 15:50
    
And it also makes them … what? Did you forget to finish your sentence? –  svick Feb 4 '13 at 16:02
    
Sorry, I meant the initialization method. Let me try and be clearer. In my code, UserData objects userA and userB are initialized in the TestInitialize() method. However, in the above code, they are in the //arrange section of the test. Should userA be made in each test that needs it (in the arrange section) or is it okay to initialize them in the TestInitialize() method and cut down on the number of times the line is repeated? –  Adam K Dean Feb 4 '13 at 16:10
2  
Sure. You have to repeat yourself, but you also don't have to scroll like a mad man to check what the objects used have been assigned for values. Each test should be treated as an invidual unit. The class is just used to group related tests together. –  jgauffin Feb 4 '13 at 16:11
1  
I do use initialization methods too. But I try to limit the information in them to things that ALL tests use. Anything else do not belong in them. You can either use multiple classes or move the initialization code to each method. (Nothing says that there may only be one test class per class being tested) –  jgauffin Feb 4 '13 at 16:13
add comment

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.

share|improve this answer
    
This is a good way to name tests, but may I ask how you would name something that, for example, added a user but did not get a return? Lets say it adds the user, then checks the user has been added, would you call that AddUser_GivenNewUser_UserAdded? –  Adam K Dean Feb 4 '13 at 19:32
2  
@AdamKDean well you tell me :) How would you know if a user was added successfully? If you are adding him to a collection them maybe I'll call it AddUser_GivenNewUser_UserAddedToCollection that way I know my assert will check for the user in that collection and I have a way to start my test. If you are returning a boolean then I'll call it AddUser_GivenNewUser_ReturnsTrue. It all depends on how you want people to use your api. –  Songo Feb 4 '13 at 20:25
add comment

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.

share|improve this answer
    
Thanks, wasn't sure how I was supposed to name them. Your advice has been noted! :) –  Adam K Dean Feb 4 '13 at 15:47
3  
Think of your tests as documentation - you are writing your requirement before you write to code, and then the code satisfies the requirement. That should help with names. –  dskh Feb 4 '13 at 15:50
add comment

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);
}
share|improve this answer
    
Indeed, I noticed last night that there was a problem with the references, yet cloning the objects when I add/retrieve them etc causes problems, in that you can't pass the object and check it exists by reference. I wrote some new tests last night, found here: github.com/Imdsm/EasyAuth/blob/master/EasyAuth.Tests/… and the new IUserStore: github.com/Imdsm/EasyAuth/blob/master/EasyAuth/IUserStore.cs ... going to play with these tests a bit, maybe post the results later on for another review. Thanks for your time, I appreciate it! –  Adam K Dean Feb 5 '13 at 9:12
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.