4
\$\begingroup\$

In writing unit tests for c# functions, there is a good case made for testing properties. In testing those properties, I felt there should be a few goals.

  • Provide a wide range of inputs
  • Test additional features like PropertyChanged
  • Don't write a ton of very repetitive test code

So I came up with this function for a specific class (although it could even be generalized with a templated method for all classes) that essentially gives a property name, a value to set the property to, the expected value to receive out, and a list of PropertyChanged names that should be fired as a result. This is accomplished by taking advantage of MSTestv2's DataTestMethod and DataRow attributes.

Looking for feedback on the quality/durability/usefulness of this code.

[TestMethod()]
[DataTestMethod]
[DataRow("Name", "A normal name", "A normal name", new string[] { "Name" })]
[DataRow("Name", null, null, new string[] { "Name" })]
[DataRow("Name", "", "", new string[] { "Name" })]
[DataRow("Name", "A normal name", "A normal name", new string[] { "Name" })]
[DataRow("Name", "A really very super duper long string to test the overall allowed length", "A really very super duper long string to test the overall allowed length", new string[] { "Name" })]
[DataRow("Address", 0, 0, new string[] { "Address", "AbsAddress"})]
[DataRow("Address", -1, null, new string[] { "Address", "AbsAddress" })]
[DataRow("Address", 300, 300, new string[] { "Address", "AbsAddress" })]
[DataRow("Address", int.MaxValue, int.MaxValue, new string[] { "Address", "AbsAddress" })]
public void ConfigItem_AllProperties_Test(string propName, object set, object expected, string[] propChangedNames)
{
    var item = new ConfigItem();
    var propsChanged = new Dictionary<string, bool>();
    foreach (var s in propChangedNames)
    {
        propsChanged.Add(s, false);
    }
    item.PropertyChanged += (sender, args) =>
    {
        if (propsChanged.ContainsKey(args.PropertyName))
        {
            propsChanged[args.PropertyName] = true;
        }
    };
    typeof(ConfigItem).GetProperty(propName).SetValue(item, set);
    object actual = typeof(ConfigItem).GetProperty(propName).GetValue(item);
    Assert.AreEqual(expected, actual);
    foreach (var pair in propsChanged)
    {
        Assert.IsTrue(pair.Value, pair.Key);
    }
}
\$\endgroup\$
5
\$\begingroup\$

too vague & hiding test cases

[DataRow("Name", "A normal name", "A normal name", new string[] { "Name" })]
[DataRow("Name", null, null, new string[] { "Name" })]
[DataRow("Name", "", "", new string[] { "Name" })]
[DataRow("Name", "A normal name", "A normal name", new string[] { "Name" })]
[DataRow("Name", "A really very super duper long string to test the overall allowed length", "A really very super duper long string to test the overall allowed length", new string[] { "Name" })]

Tests should be testing very specific things. This test however isn't doing that. I looks like it's not simply testing the PropertyChanged event. If it was, then a single DataRow would be enough to tell whether the event fires or not.

It seems to also test some conditions that might prevent it from firering. Such tests should be done separately because here it's not clear what conditions you are testing.


too many hardcoded strings

Technically I find it's ok but to my taste there are too many hardcoded strings. I wouldn't say anything if it was just data but since they are property names I don't like them being strings.

You could use nameof to replace them with something more dynamic but then the test would not be very readable anymore.


extend Assert & use expressions

Instead it'd be a better idea to write an extension for the Assert.That property that you could use for testing the event in this way:

Assert.That.PropertyChanged(
    new Customer(), 
    x => x.FirstName = "John", 
    x => x.FirstName,
    x => x.FullName
);

where the signature of the method is:

public static void PropertyChanged<T>(
    this Assert assert, 
    T obj, 
    Action<T> setProperty,
    params Expression<Func<T>>[] changedProperties
)
{
    ...
}

now you can do the reflection magic here.


This is just an idea but you could of course have a couple of such extensions. One very gneric and another one that e.g. assumes that the property being set always fires an event for it.


confusing naming

foreach (var s in propChangedNames)

If you must give variables single letter names then let them at least be derived from the main name. Here p or pcn would be appropriate. s does not stand for anything. The only exception is x in lambda expressions which stands for current-item.


misses the point

although it could even be generalized with a templated method for all classes

Then do this because otherwise you failed to reach the third goal which is:

Don't write a ton of very repetitive test code

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank for your response. In my defense of accomplishing the third goal, I was going from a function for each property with lots of repetitive code, so I was already eliminating a lot of repeating code. I knew I could Type generalize the function, so that wasn't the part I was concerned about. Should I then have a test that uses this PropertyChanged event on the property to test the PropertyChanged and then a separate test to check that if I set property a to value x, that when I call the "get", I receive value x? Or can I combine my test of both aspects of that property into a single test? \$\endgroup\$ – StephenH Dec 13 '17 at 14:41
  • 1
    \$\begingroup\$ @StephenH I would make it a single test but if your properties contain any business logic then I'd consider them separate tests (well, or just asserts) that verify each condition. This needs to be properly documented by each test/assert so that if anything fails you instantly would know what exactly went wrong without having to debug the code first. \$\endgroup\$ – t3chb0t Dec 13 '17 at 20:01
1
\$\begingroup\$

Test-Driven Development

In Test-Driven Development (TDD) you write your new code and with accompanying unit test. You might even want to create the unit test upfront, causing it to fail to test your test harness. After implementation of the new code, running the test should now succeed. With this practice, you should not have the need to create generic unit tests. Just test each piece of new code you implement.

Legacy and Boiler-Plate Code

That being said, perhaps you are working with legacy code for which you want to create boiler-plate tests in retrospect. I do see a use case for your generic unit tests when the scope and body of these classes / methods under test is very strict and clear.

auto-generate unit tests

But even then, you don't want to write unit tests that cover an entire class. Ideally, you don't even want to write these tests, but prefer to have them generated automatically. The way I would tackle this problem is to use Text-Templating and Code Generation to automatically generate unit tests at design time with the granularity of a unit test for each property you test. As a final note, you should not create these tests with the purpose of increasing code coverage. Instead they should be able to find properties where the expected pattern was incorrectly implemented or even forgotten.

\$\endgroup\$
  • 1
    \$\begingroup\$ What a surprise ;-P I modified the TDD to be FDD where I first implement features (usually with interfaces only) and then write the actual code. FDD can be followed by TDD but I find starting with TDD is not practical. I also think FDT (feature-driven-testing) is the most efficient and reliable way of testing. \$\endgroup\$ – t3chb0t Jul 27 at 12:30
  • 1
    \$\begingroup\$ I agree that TDD to the core is more academic than anything else. I do like it when you sit together with another developer to define the interfaces. Then one dev implements the API while the other writes the unit tests. But perhaps this is more FDD than TDD :p \$\endgroup\$ – dfhwze Jul 27 at 12:34

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.