8
\$\begingroup\$

Trying to get an understanding of proper unit testing, I've read up quite a bit and found myself writing tests like the ones that follow. Based on "best practices" etc., how am I doing as far as naming the tests, ensuring that each test is correctly limited in what it does, and providing good test coverage? Following those points:

  1. I've been naming the tests rather verbosely based on the suggestion that a random person who doesn't know this code (myself in six months) should be able to figure out what each test does from just its name -- note each test has Struct in its name because there are two methods with this signature, and the other (with a nearly-identical set of tests) takes classes.
  2. I've written a few more single-line tests than I'd like and will be refactoring most of this soon, but to keep the tests limited, I'm trying hard to make sure that every test only verifies a single aspect of the interface.
  3. And last, the IsWithinRange method having been written to verify something is ... well .. "within a range"... I've gone out of my way to test the minimum and maximum accepted values, a value in between, and values just past each end, as well as checking each type of exception I expect the method to throw. Is there anything else I'm not testing and should?

The full source for the project is on GitHub if anyone wants to look for more information, but the basic idea is that this Validate class is used to check values at the beginning of a method, and all exceptions for invalid data are returned as a single ValidationException. The full concept came from an article by Rick Brewster, and I've just run with it for a bit and written up more ways to check values as I found need for them.

The unit tests currently in question:

    [TestMethod]
    public void PassingValidToStruct_IsWithinRange_Passes()
    {
        Validate.Begin().IsWithinRange((Int32?)5, "value", 0, 10).Check();
    }

    [TestMethod]
    public void PassingMinToStruct_IsWithinRange_Passes()
    {
        Validate.Begin().IsWithinRange((Int32?)0, "value", 0, 10).Check();
    }

    [TestMethod]
    public void PassingMaxToStruct_IsWithinRange_Passes()
    {
        Validate.Begin().IsWithinRange((Int32?)10, "value", 0, 10).Check();
    }

    [TestMethod]
    public void MinMaxValueSameToStruct_IsWithinRange_Passes()
    {
        Validate.Begin().IsWithinRange((Int32?)5, "value", 5, 5).Check();
    }

    [TestMethod]
    public void PassingTooSmallToStruct_IsWithinRange_Fails()
    {
        var valid = Validate.Begin().IsWithinRange((Int32?)-1, "value", 0, 10);

        ExceptionAssert.Throws<ValidationException>(() => valid.Check());
    }

    [TestMethod]
    public void PassingTooLargeToStruct_IsWithinRange_Fails()
    {
        var valid = Validate.Begin().IsWithinRange((Int32?)11, "value", 0, 10);

        ExceptionAssert.Throws<ValidationException>(() => valid.Check());
    }

    [TestMethod]
    public void PassingNullToStruct_IsWithinRange_Fails()
    {
        var valid = Validate.Begin().IsWithinRange((Int32?)null, "value", 0, 10);

        ExceptionAssert.Throws<ValidationException>(() => valid.Check());
    }

    [TestMethod]
    public void PassingNullNameToStruct_IsWithinRange_Throws()
    {
        ExceptionAssert.Throws<ValidationException>(() => Validate.Begin().IsWithinRange((Int32?)5, null, 0, 10));
    }

    [TestMethod]
    public void PassingEmptyNameToStruct_IsWithinRange_Throws()
    {
        ExceptionAssert.Throws<ValidationException>(() => Validate.Begin().IsWithinRange((Int32?)5, "", 0, 10));
    }

    [TestMethod]
    public void PassingNullMinToStruct_IsWithinRange_Throws()
    {
        ExceptionAssert.Throws<ValidationException>(() => Validate.Begin().IsWithinRange((Int32?)5, "value", null, 10));
    }

    [TestMethod]
    public void PassingNullMaxToStruct_IsWithinRange_Throws()
    {
        ExceptionAssert.Throws<ValidationException>(() => Validate.Begin().IsWithinRange((Int32?)5, "value", 0, null));
    }

    [TestMethod]
    public void PassingMinMaxBackwardsToStruct_IsWithinRange_Throws()
    {
        ExceptionAssert.Throws<ArgumentException>(() => Validate.Begin().IsWithinRange((Int32?)5, "value", 10, 0));
    }

And the method in question:

    internal const String ArgumentOutOfRange = "{0}'s value of \"{1}\" is out of range; must be a value from \"{2}\" to \"{3}\"";

    /// <summary>
    /// Checks a comparable value to ensure that it is within the specified range
    /// </summary>
    /// <typeparam name="T">Any object type which is a struct, and which implements IComparable</typeparam>
    /// <param name="validation">The validation instance to carry forward</param>
    /// <param name="value">An instance of T? to check</param>
    /// <param name="name">The name of the value</param>
    /// <param name="min">An instance of T? which acts as the minimum accepted value</param>
    /// <param name="max">An instance of T? which acts as the maximum accepted value</param>
    /// <exception cref="ArgumentException">Thrown if min is greater than max</exception>
    /// <exception cref="ValidationException">Thrown if min or max is null, or the name is an empty string</exception>
    /// <exception cref="ArgumentOutOfRange">Passed on in the return when the value is not within the expected range</exception>
    /// <exception cref="ArgumentNullException">Passed on in the return when the value is null</exception>
    public static Validation IsWithinRange<T>(this Validation validation, T? value, String name, T? min, T? max) where T : struct, IComparable
    {
        Validate.Begin()
            .IsNotNullOrEmpty(name, "name")
            .IsNotNull(min, "min")
            .IsNotNull(max, "max")
            .Check();

        if (min.Value.CompareTo(max.Value) > 0)
            throw new ArgumentException("min must be equal to or less than max");

        if (!value.HasValue)
            return validation.GetInstance().AddException(new ArgumentNullException(name, String.Format(ArgumentNull, name)));

        if (min.Value.CompareTo(value.Value) > 0 || max.Value.CompareTo(value.Value) < 0)
            return validation.GetInstance().AddException(new ArgumentOutOfRangeException(name, String.Format(ArgumentOutOfRange, name, value.Value, min.Value, max.Value)));

        return validation;
    }
\$\endgroup\$
6
\$\begingroup\$

There's not a universal answer to your question. Instead it's more a matter of finding the point along the continuum that you like and get the most benefit from. If you're testing something extremely complex, fiddly, and prone to updates, it can be well worth splitting out individual tests. If it's less complex, you might find it's conceptually easier to merge a few of your tests (in your case, perhaps merging into a Struct_IsWithinRange_Passes and a Struct_IsWithinRange_Fails. I find this helps find gaps in the testing, as in your case you test several inputs but only a single set of bounds. Thus your original tests would not catch the admittedly unlikely case of hardcoding the bounds.

[TestMethod]
Struct_IsWIthinRange_Passes()
{
     // Numbers at either end or anywhere in the middle of the range must pass
     Validate.Begin().IsWithinRange((Int32?)0, "value", 0, 10).Check();
     Validate.Begin().IsWithinRange((Int32?)5, "value", 0, 10).Check();
     Validate.Begin().IsWithinRange((Int32?)10, "value", 0, 10).Check();

     // This is even true for large ranges or negative numbers
     Validate.Begin().IsWithinRange((Int32?)Int32.MaxValue, "value", 0, Int32.MaxValue).Check();
     Validate.Begin().IsWithinRange((Int32?)Int32.MinValue, "value", In32.MinValue, -1).Check();
}

[TestMethod]
Struct_IsWithinRange_Fails()
{
    // Numbers just outside the range must fail
    var valid = Validate.Begin().IsWithinRange((Int32?)-1, "value", 0, 10);
    ExceptionAssert.Throws<ValidationException>(() => valid.Check());

    var valid = Validate.Begin().IsWithinRange((Int32?)11, "value", 0, 10);
    ExceptionAssert.Throws<ValidationException>(() => valid.Check());

    // As must null values
    var valid = Validate.Begin().IsWithinRange((Int32?)null, "value", 0, 10);
    ExceptionAssert.Throws<ValidationException>(() => valid.Check());

    // Even with extremely large ranges
    var valid = Validate.Begin().IsWithinRange((Int32?)Int32.MaxValue, "value", 0, Int32.MaxValue - 1);
    ExceptionAssert.Throws<ValidationException>(() => valid.Check());

    var valid = Validate.Begin().IsWithinRange((Int32?)Int32.MinValue, "value", Int32.MinValue + 1, -1);
    ExceptionAssert.Throws<ValidationException>(() => valid.Check());
}

I was once taught that the theoretical ideal is one in which a single bug introduced results in a single unit test failure so that by seeing the failure you can identify exactly what to go fix. I personally don't find that very feasible in most code, and am perfectly happy with being less granular.

The next step you will wish to consider is whether it's worth making this kind of unit test data-driven. Namely is it worth sacrificing the line information about which test fails to make it easier to test additional values? It would reduce the repetition greatly, especially in the failure case, but it's less clear whether it is a win.

[TestMethod]
Struct_IsWithinRange_Fails()
{
    var tests = new[] {
        new { Value = (Int32?)-1,           Min = 0, Max = 10 },
        new { Value = (Int32?)11,           Min = 0, Max = 10 },
        new { Value = (Int32?)null,         Min = 0, Max = 10 },
        new { Value = (Int32?)Int.MaxValue, Min = 0, Max = Int.MaxValue - 1},
        new { Value = (Int32?)Int.MinValue, Min = Int.MinValue + 1, Max = -1},
    };

    foreach (var test in tests)
    {
        var valid = Validate.Begin().IsWithinRange(test.Value, "value", test.Min, test.Max);
        ExceptionAssert.Throws<ValidationException>(() => valid.Check());
    }
}

As a side note, it's worth mentioning that my additional failure case checks don't themselves verify that a range other than 0-10 is being verified, and none of these test the range is well-formed.

In other contexts I've been taught that unit tests should serve as a living document of the interface you are guaranteeing to your callers. For example you describe that the validation process packages up all reasons a value fails validation. However your tests do not verify this at all. They do not differentiate between the various exceptions that IsWithinRange may add.

More concretely, you have to consider what you are accepting as valid use of this code. Is a caller allowed to look at the exact types of exception, or is it expected to just iterate over the list of exceptions and display them to a user? If the former, you might write some tests that verify the exception types. If the latter, you might write a test that generates multiple exceptions and just verifies it can iterate the list, and that the count is greater than zero.

Regardless, don't get too bogged down in the meta-game here. The purpose of unit tests is to catch unintentional side effects of changes to your code that might result in invalidating its callers assumptions. Once you catch one, you either fix it, or for intentional changes, you document them and ripple their effect throughout your codebase (or your users' codebases) as necessary. Everything else you do with unit tests is either a nice bonus, or extra work.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the supportive feedback. A handful of other tests I've written look like the data-driven test you suggested, and most of what you said goes with things I've thought; good to know I'm thinking the right thoughts. The extra points you brought up about checking the actual contents of the ValidationException objects and making sure the bounds aren't hard-coded however, I didn't think of. So thanks for that too. :) \$\endgroup\$ – Amazingant Feb 16 '14 at 3:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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