6
\$\begingroup\$

I wrote NUnit tests for class CharGrid from this homework assignment.

My goal here is DRY: I want to keep the testing code as short and simple as possible. In addition, want to run every test independently, as opposed to having a single method with a lot of asserts that either succeeds or fails as a whole.

I went through the NUnit documentation and found this "Parameterized TestFixture" annotation, which seemed to help fulfill my goal.

I would like to ask if there are any improvements to the testing code regarding what I just stated my goals were.

In addition, I would like to know if it is a good idea to have a separate CharGridTestFixture class or if it is preferable to directly annotate class CharGrid with a TestFixture and it's methods with TestCaseSource annotations and test them sort of in-place. This way there would be one less class.

Moreover, I used NUnit 2 without looking at any of the alternatives. Is it still a decent Unit testing framework or should I switch to NUnit 3 or something else?

using System;
using System.Collections;
using System.Collections.Generic;

using NUnit.Framework;

namespace CS108
{

This is a skeleton of the code to be tested:

            // HW1 2-d array Problems
            // CharGrid encapsulates a 2-d grid of chars and supports
            // a few operations on the grid.

            public class CharGrid {
                    private char[][] grid;

                    /**
            * Constructs a new CharGrid with the given grid.
            * Does not make a copy.
            * @param grid
            */
                    public CharGrid(char[][] grid) {
                            this.grid = grid;
                    }

                    /**
            * Returns the area for the given char in the grid. (see handout).
            * @param ch char to look for
            * @return area for given char
            */
                    public int charArea(char ch) {
                            return 0; // TODO ADD YOUR CODE HERE
                    }

                    /**
            * Returns the count of '+' figures in the grid (see handout).
            * @return number of + in grid
            */
                    public int countPlus() {
                            return 0; // TODO ADD YOUR CODE HERE
                    }

            }

This is specification of the tests:

            public class CharGridTestCase {
                    internal char [][] grid;
                    internal Dictionary<char, int> charArea;
                    internal int countPlus;
            }

            class CharGridTestCase1 : CharGridTestCase {
                    public CharGridTestCase1 () {
                            grid = new char[][] {
                                    new [] { 'c', 'a', 'x' },
                                    new [] { 'b', ' ', 'b' },
                                    new [] { ' ', ' ', 'a' }
                            };
                            charArea = new Dictionary<char, int> {
                                    {'a', 6},
                                    {'b', 3},
                                    {'c', 1},
                                    {'x', 1},
                                    {'z', 0},
                                    {' ', 4}
                            };
                            countPlus = 0;
                    }
            }

            class CharGridTestCase2 : CharGridTestCase {
                    public CharGridTestCase2 () {
                            grid = new char[][] {
                                    new [] {'a', 'b', 'c', 'd'},
                                    new [] {'a', ' ', 'c', 'b'},
                                    new [] {'x', 'b', 'c', 'a'}
                            };
                            charArea = new Dictionary<char, int> {
                                    { 'a', 12 },
                                    { 'b', 9 },
                                    { 'c', 3 },
                                    { 'd', 9 },
                                    { 'x', 1 },
                                    { ' ', 1 }
                            };
                            countPlus = 0;
                    }
            }

            class CharGridTestCase3 : CharGridTestCase {
                    public CharGridTestCase3 () {
                            grid = new char[][] {
                                    new [] { ' ', ' ', 'p', ' ', ' ', ' ', ' ', ' ', ' ' },
                                    new [] { ' ', ' ', 'p', ' ', ' ', ' ', ' ', 'x', ' ' },
                                    new [] { 'p', 'p', 'p', 'p', 'p', ' ', 'x', 'x', 'x' },
                                    new [] { ' ', ' ', 'p', ' ', ' ', 'y', ' ', 'x', ' ' },
                                    new [] { ' ', ' ', 'p', ' ', 'y', 'y', 'y', ' ', ' ' },
                                    new [] { 'z', 'z', 'z', 'z', 'z', 'y', 'z', 'z', 'z' },
                                    new [] { ' ', ' ', 'x', 'x', ' ', 'y', ' ', ' ', ' ' }
                            };
                            charArea = new Dictionary<char, int> {
                                    { 'p', 25 },
                                    { 'x', 48 },
                                    { 'y', 12 },
                                    { 'z', 9  },
                                    { ' ', 63 }
                            };
                            countPlus = 2;
                    }
            }

This is the code running the tests:

        [TestFixture(typeof(CharGridTestCase1))]
        [TestFixture(typeof(CharGridTestCase2))]
        [TestFixture(typeof(CharGridTestCase3))]
        class CharGridTestFixture<T> where T : CharGridTestCase, new() {
                T tc;
                CharGrid that;

                public CharGridTestFixture(/*Type testCaseType*/) {
                        //tc = (CharGridTestCase)Activator.CreateInstance (testCaseType);
                        tc = new T();
                        that = new CharGrid (tc.grid);
                }

                IEnumerable CharAreaSource {
                        get {
                                foreach (var e in tc.charArea) {
                                        yield return new TestCaseData (e.Key).Returns (e.Value);
                                }
                        }
                }

                [TestCaseSource("CharAreaSource")]
                public int charArea(char c) {
                                return that.charArea(c);
                }

                [Test]
                public void countPlus() {
                        Assert.AreEqual(tc.countPlus, that.countPlus());
                }
        }
}
\$\endgroup\$
1
  • 3
    \$\begingroup\$ I'm not a fan of the excessively large indent. Most people use 4 or 2 spaces for indentation. \$\endgroup\$
    – Emily L.
    Commented Jul 1, 2015 at 12:39

1 Answer 1

5
\$\begingroup\$

C# conventions

The assignment you posted from is written for Java. While the syntax is similar, C# has some slightly different conventions with naming and formatting. In particular, in C# it's normal to put opening braces on a new line, and for method names to be PascalCased rather than camelCased.

Also, you seem to have a very deep indentation. Visual Studio defaults to 4 characters, which is what seems to be conventional for C#.

As a side note, it's also usually considered poor practice to leave commented out ("zombie") code around. If you don't need it, delete it.


Naming

Some of your names don't mean much. What's "that" or "e"? These should be replaced with clear, descriptive names. Similarly, don't abbreviate names. There's no reason to write tc instead of testCase.


Simplicity

When creating complex code structures, it's useful to compare them to the alternative and ask what exactly the trade-offs are. So the pros and cons of your approach as I see them:

Pros

  • It reduces repetition in rewriting the same tests for different test cases

Cons

  • It requires a weird structure where you have subclasses differing from their parents only by data (which you'd normally just do through constructor arguments)
  • It limits you to using the same test data for all of your tests
  • It requires a relatively hard to understand construct involving generics

Given this, I'd be a little reluctant to actually use this kind of approach unless I was dead sure that a) the level of repetition for a simpler approach would be too much and, b) the rigidity of which test data I can use would not be a problem.

As it is, if you're not planning to write more tests, I think a) is false, and if you are planning to write more tests then I think b) is false until they're done.

Alternative

Even if you do want to try to cut down on repetition, you might want to look at an alternative, like the TestCase attribute. You could, for example, parameterize a test method like:

public void xxx(string grid, int expectedValue)

And take grid in a format where / represents a new line (so the grid for your first test case would be written "cax/b b/ a"), then have a helper method that translates that to your grid format.

This would give you a lot more flexibility and simplicity, and if you had repeated cases, you could assign the string representations to consts. Then your existing test class would look like:

[TestFixture]
public class CharGridTestFixture
{
    private static CharGrid CreateGrid(string grid)
    {
        //Some implementation
    }

    [TestCase("cax/b b/  a", 0)]
    //Other test cases
    public void CountPlus(string grid, int expected)
    {
        Assert.AreEqual(expected, CreateGrid(grid).countPlus());
    }
}

Types of tests

Given the relative simplicity of the problem, I'd consider the tests you're doing to be acceptance tests rather than unit tests. A unit test would generally target one very specific point of behavior, whereas your tests are more along the lines of setting up a realistic, potentially complex, situation and getting a result without worrying too much about how the code under test handles it.

Things which would be appropriate for unit tests might include:

  • Degenerate cases- e.g. a grid with 0 rows and columns, a grid containing no letters
  • Simple cases- e.g a grid with only a single cell, a grid with only a single row, a grid containing a single letter
  • As-minimal as possible complex cases. For example, when you want to test that going from a single row to multiple rows works, you might pick a 2x1 or 2x2 grid and only move to 3x3 or more if you had a reason to think it'd be qualitatively different in some way. The contents of these grids would also be as minimal as possible while trying to capture whatever it is you're trying to test in that particular case.
  • Invalid cases like non-rectangular grids (assuming these are invalid) to test error-handling

Generally these would go along with much more descriptive test method names stating the precise expectation which is being tested.

That isn't to say there's anything wrong with the kinds of tests you've done, acceptance tests have their own uses, I just wouldn't consider it unit testing.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for your comments. I am now doing HW2 (Tetris) and I started by making it a syntactically correct C# first and then running autoformatter on the code. I am also changing simple get/set methods into C# properties. Also you helped me realize that having multiple asserts in a test method is perfectly ok (unless I overdo it). \$\endgroup\$
    – user7610
    Commented Jul 3, 2015 at 20:49
  • \$\begingroup\$ @user7610 Glad to hear it was helpful. Out of interest, what part of my answer connected to that bit about multiple asserts? \$\endgroup\$ Commented Jul 3, 2015 at 23:10
  • \$\begingroup\$ Well, there is not much connection, I admit. 1) You said that it is better to keep it simple 2) the Java code given as part of the homework does contain test methods with multiple asserts 4) 3) NUnit 3 offers github.com/nunit/dev/wiki/Multiple-Asserts which is simple to use. \$\endgroup\$
    – user7610
    Commented Jul 6, 2015 at 7:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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