Unit test for Unique Position Point

Question

Here is one of my msTest Unit Tests and possibly one of the worst "looking" tests.

[TestMethod]
public void UniquePositionPoint_CreateUPP_UPPExistsInPPField()
{
    //Setup the values to be used by this test.
    CIM_Network network = new CIM_Network();
    string x = "5";
    string y = "10";
    string expectedMRID = String.Format("_UPP{0}", (x + y).GetHashCode());
    //Carry out the test
    CIM_PositionPoint pp = new CIM_PositionPoint(network, mRID: "pp", fields: new Dictionary<string, object>()
        {
                    {"cim:PositionPoint.CoordinateSystem",new CIM_CoordinateSystem(network, mRID: "coordSys")},
                    {"cim:PositionPoint.xPosition", x},
                    {"cim:PositionPoint.yPosition", y},
        }, addBackReferences:true);
    //Checks if the test was carried out correctly.
    Assert.AreEqual(2, pp.Fields.Count(), 
        String.Format("ERROR: There should only be 2 fields in the position point as the x and y values should of been moved out to the UPP. Currently the fields in the pp are {0}", 
        string.Join(":", pp.Fields.Select(kvp => (kvp.Key + ":" + kvp.Value.ToString())))));
    Assert.IsNotNull(pp.Fields.ContainsKey("uniquePositionPoint"), "ERROR: The field 'uniquePositionPoint:' does not exist in the position point fields.");
    Assert.IsInstanceOfType(pp.getField("uniquePositionPoint"), typeof(CIM_UniquePositionPoint),
        String.Format("ERROR: Expected the object at field 'uniquePositionPoint' to be of type CIM_UniquePositionPoint however it was of type {0}", 
        pp.getField("uniquePositionPoint").GetType().Name));
    CIM_UniquePositionPoint uPP = (CIM_UniquePositionPoint)pp.getField("uniquePositionPoint");
    Assert.AreEqual(expectedMRID, uPP.MRID, 
        String.Format("ERROR: Expected the mRID {0} to be the UPP's mRID but instead it was {1}", expectedMRID, uPP.MRID));
    Assert.IsTrue(uPP.BackRefs.Contains(pp), "ERROR: The position point was not found within the upp.");
}

What the code does is add a "position point" into a network. When this happens a few things also happen as a result: 1. A UniquePositionPoint (upp for short) is created. 2. That upp will contain all of the coordinates of the position point. 3. The coordinates will be removed from the position point.

What I mainly want to know is if this (a bunch of asserts) is the correct way to test this functionality or should it be broken down at all?

Answer 1

You listed three distinct things that happen when you perform this operation. That means you should have three distinct test cases.

Clumping them all together means:

• If one of the assertions about the first thing fails, none of the insertions about the other results occur. If 1 and 3 are both failing, but 2 is succeeding, that is important information to help determine the root cause of the problem.

  With the way the code is now, the following can happen. The test case fails indicating that 1 is not happening correctly. You edit the code so that 1 happens correctly. When you re-run the test case, it fails because 3 isn't happening. Was 3 never happening? Is 3 not happening now because of the changes you made to fix 1? You don't know. By using multiple test cases, you will always know what is happening and what is not.

• The test case is hard to read. There are 10 lines dedicated to checking if the proper result was achieved. A person who hasn't seen the code before, should have to dig through all of that just to see what the the test case is checking for.

---

Ways to improve:

• One test case for one distinct result. You can have multiple assertions in a test case, but they should be verifying the same concept.

• Clear and descriptive test names. Ideally, the method name should tell you 1) What is being tested, 2) What are the inputs (context of the test case)? 3 What the expected result is? This way, you could already have an idea of what might be wrong before even seeing the code. Your test case is following this naming structure, but it is too generic. This is another indication that you might be checking two different concepts in the same test case.

• Extract more complicated assertions into a method. Just like with real code, extracting functionality into a method is a great way to improve readability. All of your assertions have very descriptive error messaged. In general, this is not a bad thing, but it might be an indication that just looking at the assertion might not clearly indicate the intent to someone reading the code. Creating a specific method for the assertion can hide away to long string.Format() call and keep the test case clean.

---

Other comments:

• Starting your error messages with "Error:" is redundant. The fact that this message is being shown indicates that there is an error.

• Use constants to clean up your test case setup. Since you will likely have many test cases with similar inputs, constructing expectedMRID for a specific point will get repetitive.

• Use variable names to indicate if input values have meaning. Are you explicitly testing 5,10 or are those just some value to represent a generic input. This could help someone else know the real intent of a test case.

• Not related to the test case, but the API you are testing seems like a real pain to work with. You have to craft this specifically formated dictionary just to create an point with a specified x and y coordinates. Then to get a value, who id happens to be related to the coordinates, you have to pass in a string key to get the value, which then has to be cast to the proper type.