How to do unit tests correctly

Question

I'm trying to write an application using the Test Driven Design approach - I'm quite new to unit tests, so I'm just wondering what the right way to test for correct inputs and exceptions is.

I have this class that's used to load a config file:

class Config
{
    private XmlDocument configfile;

    public Config()
    {
        configfile = new XmlDocument();
    }

    public void LoadConfigFile(string filename)
    {
        if(string.IsNullOrEmpty(filename)) 
            throw new System.ArgumentException("You must specify a filename");

        try
        {
            configfile.Load(filename);
        }
        catch (Exception ex)
        {
            throw new System.IO.FileNotFoundException("File could not be loaded");
        }
    }
}

So, there are 3 tests that can be performed here:

1. load an actual file and make sure there are no errors
2. try to load an invalid file (one that doesnt exist)
3. do not specify an filename argument

Is the correct way to test these, to write 3 test methods, like so?:

    /// <summary>
    ///A test for LoadConfigFile
    ///</summary>
    [TestMethod()]
    public void LoadConfigFileTest()
    {
        Config target = new Config(); // TODO: Initialize to an appropriate value
        string filename = "config.xml"; // TODO: Initialize to an appropriate value
        target.LoadConfigFile(filename);
        Assert.Inconclusive("A method that does not return a value cannot be verified.");
    }

    /// <summary>
    ///A test for LoadConfigFile
    ///</summary>
    [TestMethod()]
    [ExpectedException(typeof(System.ArgumentException))]
    public void LoadConfigFileTest1()
    {
        Config target = new Config(); // TODO: Initialize to an appropriate value
        string filename = ""; // TODO: Initialize to an appropriate value
        target.LoadConfigFile(filename);
        Assert.Inconclusive("A method that does not return a value cannot be verified.");
    }

    /// <summary>
    ///A test for LoadConfigFile
    ///</summary>
    [TestMethod()]
    [ExpectedException(typeof(System.IO.FileNotFoundException))]
    public void LoadConfigFileTest2()
    {
        Config target = new Config(); // TODO: Initialize to an appropriate value
        string filename = "blah.xml"; // TODO: Initialize to an appropriate value
        target.LoadConfigFile(filename);
        Assert.Inconclusive("A method that does not return a value cannot be verified.");
    }

Also, should all 3 of these tests have try {} catch () {} statements? As in the first test, the correctness is implied, and in the 2nd and 3rd tests, I'm checking for an exception anyway, so an exception is of no consequence to the tests.

Answer 1

You're on the right path but not quite there yet.

It is pretty rare to have a situation where you have to call Assert.Inconclusive and in your situation it isn't necessary: when you expect an exception and the exception is thrown, it will work as it should (aka: it should show up as a green result). When you expect an exception and none is thrown, it will show up as a fail (aka: red result). More on that here.

As a matter of fact, a method that returns void can be tested. Instead of returning a value it might change the state of something. In your case the variable configFile. The way to test this is by retrieving the value (for example by providing a getter) and/or by using dependency injection and substituting the variable for a fake/mock/stub (choose your jargon) in the test.

There should be no try-catch blocks: it would only hide any problems your code might have. As to your original code: don't catch the actual exception and rethrow it as a FileNotFoundException. Look at all the possible causes you are hiding.

Expanding on the comments:

I wouldn't want a develper to mess around with the configfile property directly, so should I make it public for the test, then change it back to private?

This is a good concern to have and every developer faces it when doing tests. What's important to realize is that inner workings of a unit are not something you should usually test. Implementation details are exactly what they are: implementation details. However sometimes the alternatives are just even less wanted so you have to make the comparison whether or not you want to do it after all.

This can be a legitimate usecase and luckily there is a fairly nice workaround for this! I go in more detail in it here but I would suggest to give internal access to configfile whether it it using an internal constructor, method, property or just the field. By applying the [InternalsVisibleTo] attribute you can provide access to it from your unit test project while still hiding it from the public.

with regards to using stubs to test what's in configfile, it looks like I have to change everything to be interface dependant? Doesn't this just add tons more complexity that isn't needed?

Define "what is needed". It is true that by defining interfaces and injecting these you have an extra layer of abstraction in your code but this is done for a reason: due to the loosely coupled nature of your class you can now test it more easily by injecting another implementation.

This process - Dependency Injection - is the backbone of unit testing and will also help with your first remark.