Are those unit tests fine?

Question

Hi!

I'm trying to grasp test driven development, and I'm wondering if those unit tests is fine. I have a interface which looks like this:

public interface IEntryRepository
{
    IEnumerable<Entry> FetchAll();
    Entry Fetch(int id);
    void Add(Entry entry);
    void Delete(Entry entry);
}

And then this class which implements that interface:

public class EntryRepository : IEntryRepository
{
    public List<Entry> Entries {get; set; }

    public EntryRepository()
    {
     Entries = new List<Entry>();
    }

    public IEnumerable<Entry> FetchAll()
    {
     throw new NotImplementedException();
    }

    public Entry Fetch(int id)
    {
     return Entries.SingleOrDefault(e => e.ID == id);
    }

    public void Add(Entry entry)
    {
     Entries.Add(entry);
    }

    public void Delete(Entry entry)
    {
     Entries.Remove(entry);
    }
}

Theese are the unit tests I have written so far, are they fine or should I do something different? Should i be mocking the EntryRepository?

[TestClass]
public class EntryRepositoryTests
{
    private EntryRepository rep;

    public EntryRepositoryTests()
    {
     rep = new EntryRepository();
    }

    [TestMethod]
    public void TestAddEntry()
    {
     Entry e = new Entry { ID = 1, Date = DateTime.Now, Task = "Testing" };
     rep.Add(e);

     Assert.AreEqual(1, rep.Entries.Count, "Add entry failed");
    }

    [TestMethod]
    public void TestRemoveEntry()
    {
     Entry e = new Entry { ID = 1, Date = DateTime.Now, Task = "Testing" };
     rep.Add(e);

     rep.Delete(e);
     Assert.AreEqual(null, rep.Entries.SingleOrDefault(i => i.ID == 1), "Delete entry failed");
    }

    [TestMethod]
    public void TestFetchEntry()
    {
     Entry e = new Entry { ID = 2, Date = DateTime.Now, Task = "Testing" };
     rep.Add(e);

     Assert.AreEqual(2, rep.Fetch(2).ID, "Fetch entry failed");
    }
}

Thanks!

Answer 1

Looks good overall. You should use transactions (or create new instance of repository in TestInitialize) to make sure the tests are trully isolated.

Also use more descriptive test methods, like When_a_new_entity_is_added_to_a_EntryRepository_the_total_count_of_objects_should_get_incremented

Answer 2

Before I answer, let me state that I am fairly new to unit testing and by no means an expert, so take everything I state with a grain of salt.

But I feel that your unit tests are largely redundant. Many of your methods are simple pass through, like your AddEntry method is simply a call to the underlying List method Add. Your not testing your code, your testing the Java library.

I would recommend only unit testing methods that contain logic that you write. Avoid testing obvious methods like getters and setters, because they operate at the most basic level. That's my philosophy, but I know some people do believe in testing obvious methods, I just happen to think it is pointless.

Answer 3

Seems fine like this. I personally like to give my tests a bit more descriptive names but that's more of a personal preference.

You can use mocking for dependencies of the class you're testing, EntryRepository is the class under test so no need to mock that, else you would end up testing the mock implementation instead of the class.

Just to give a quick example. If your EntryRepository would use a backend database to store the Entries instead of a List you could inject a mock-implementation for the data-access stuff instead of calling a real database.

Answer 4

This looks like a fine start, but you should try to test the 'borderline' cases as much as possible. Think about what might cause your methods to fail - Would passing a null Entry to Add or Delete be valid? Try to write tests that exercise every possible code path. Writing tests in this manner will make your test suite more useful in the future, should you make any changes to the code.

Also, it is useful for each test method to leave the test object in the same state as when it was invoked. I noticed your TestFetchEntry method adds an element to the EntryRepository, but never removes it. Having each method not affect test object state makes it easier to run series of tests.

Answer 5

You should not be mocking the IEntryRepository since the implementing class is the class under test. You might want to mock the List<Entry> and inject it, then just test that the methods that you invoke via your public interface are called appropriately. This would just be an alternative to the way you have it implemented and isn't necessarily better -- unless you want the class to have it's backing class injected in which case writing the tests that way would force that behavior.

You might want some more tests to make sure that when you insert an entry, the right entry is inserted. Likewise with delete -- insert a couple of entries, then delete one and make sure that the proper one has been deleted. Once you come up with tests to get the code to do what you want it to, keep thinking of ways that you could mess up writing the code and write tests to make sure those don't happen. Granted this class is pretty simple and you may be able to convince yourself that the tests you have that drive behavior is sufficient. It doesn't take much complexity, though, to make it worth testing edge cases and unexpected behavior.

Answer 6

For a TDD beginner and for this particular class, your tests are fine. +1 for making the effort.

Post another question once you get to more complex scenarios involving dependency injection and mocking. This is where things get really interesting ;).

Answer 7

Just off the top of my head...

Although your testing of add really only tests the framework:

• You've got adding 1 item, that's good
• what about adding LOTS of items (I mean, ridiculous amounts - for what value of n entries does the container add fail?)
• what about adding no items? (null entry)
• if you add items to the list, are they in a particular order? should they be?

likewise with your fetch:

• what happens in your fetch(x) if x > rep.Count ?
• what happens if x < 0?
• what happens if the rep is empty?
• does x match performance requirements (what's it's algorithmic complexity? is it within range when there's just one entry and when there's a ridiculously large amount of entries?

There's a good checklist in the book Pragmatic Unit Testing (good book, highly recommended)

• Are the results right?
• Are all the boundary conditions CORRECT
  • Conform to an expected format
  • Ordered correctly
  • In a reasonable range
  • Does it Reference any external dependencies
  • Is the Cardinality correct? (right number of values)
  • does it complete in the correct amount of Time (real or relative)
• Can you check inverse relationships
• Can you cross check the results with another proven method
• Can you force error conditions
• Are performance characteristics within bounds

Answer 8

Here's some thoughts:

Positive

• You're Unit Testing!
• You're following the convention Arrange, Act, Assert

Negative

• Where's the test to remove an entry when there's no entry?
• Where's the test to fetch an entry when there's no entry?
• What is supposed to happen when you add two entries and remove one? Which one should be left?
• Should Entries be public. The fact that one of your asserts calls rep.Entries.SingleOrDefault suggests to me you're not constructing the class correctly.
• Your test naming is a bit vague; typically a good pattern to follow is: {MethodName}_{Context}_{Expected Behavior} that remove the redundancy "test" redundancy.

As a beginner to TDD I found the book Test-Driven Development By Example to be a huge help. Second, Roy Osherove has some good Test Review video tutorials, check those out.