Pages

Wednesday, August 24, 2016

Catching Up on Coding - TDD Part 2

I’ve been a software developer for twenty years, but am behind in some skills. This series chronicles my self-improvement.

Taking a cue from the 70s and its badass fonts, my Fantastic Journey is traveling from skill to skill, trying to escape every software engineer’s Bermuda Triangle: obsolescence. I don’t have Jared Martin’s Sonic Energizer to focus my thoughts and manipulate matter. All I can do is listen to the groovy theme music.

Previously on CUOC: TDD Part 1
Next Episode: TDD Part 3

 

 

Teaser

It’s one thing to practice TDD from the start. It’s another to take legacy code that you wrote and create unit tests for it. My EduAgent project includes a client service that processes requests to update the registry.

Looking at my code, I right away see my first stumbling block: the purpose of the code is changing the registry. For testing, this means:

  • Affecting the registry of whatever computer the tests run on.
  • Guaranteeing each test starts with a clean environment. What does this mean for parallel testing?
  • Guaranteeing the registry returns to its original state, regardless of test results.
  • Not breaking the computer.

These are characteristics of integration/functional tests, not unit tests. Should I mock the registry classes to deal with this code?

object value = Registry.GetValue(keyName, valueName, null);

If I do—if I can—am I really testing anything? Or am I creating fake conditions that assure the test will pass? There’s a danger, in unit testing, of creating tests that don’t actually test the method: they “test” a mock that will always return the expected value.

Act 1 – What I Can Test, What I Should

The purpose of mocks is to allow the unit test to focus on what it can test, and ignore what can corrupt the test—assuming what can corrupts the test isn’t exactly what should be tested!

A classic, challenging example is methods that access a database. I don’t want to test if I can access the database. I want to test what I’m doing with the data. For example, consider this class method.

public class EmployeeInfoService
{
	IEmployeeDbContext _db;
	public EmployeeInfoService(IEmployeeDbContext db) { _db = db; }
	public List GetAverageHourlyPay(int[] employeeIds)
	{
		return from stats in _db.PayStubs
		//...a bunch of code that gets data and calculates the hourly pay by doing this:
                // stats.Hours / stats.Amount
		;
	}
}

I don’t care, in the test I’m about to write, if I can access the database, or if the database is returning the right data, or breaks when I hand it a list of EmployeeIds. All I care about is whether the method properly does the math given specific numbers. So, I’d mock the EmployeeDbContext and fabricate the numbers.

	[Fact]
	public void EmployeeInfoService_GetAverageHourlyPay_Should_Correctly_Calculate()
	{
		var db = new MockEmployeeDbContext();
		//...add a bunch of test data to the mock, then pass it to service
		var service = new EmployeeInfoService(db);
		var list = service.GetAverageHourlyPay(new int[] { 1, 2, 3 });
		Assert.Equals(23.34, list[0].AverageHourlyPay);
		Assert.Equals(17.98, list[1].AverageHourlyPay);
	}

Contrast that to this method.

	public string[] GetEmployeeNames()
	{      return _db.Employees.Select(a => a.Name).ToArray();
	}

Is there any reason to create a unit test for this method? I don’t think so. The test would look something like this.

	[Fact]
	public void EmployeeInfoService_GetEmployeeNames_Should_Return_A_List_Of_Names()
	{
		var db = new MockEmployeeDbContext();
		//...add a bunch of mock Employees with names, then pass to service
		var service = new EmployeeInfoService(db);
		var list = service.GetEmployeeNames();
		Assert.True(list.Contains("Varian"));
	}

This gets us nothing. All we did was test that our mocking framework works. So…

Lesson 1: Be sure you’re testing something that can fail.

Act 2 – The Simple Test

For my RegistryService class, I have a different problem as stated above. Can I even mock the .Net Registry class? We’ll look at that shortly. For right now, I’ll evaluate all the methods and see which ones I can test without mocking. Here’s the complete list:

  • ProcessRequest
  • GetRegistryKeyValue
  • GetRegistryValueKind
  • SetRegistryKeyValue
  • DeleteRegistryKey
  • DeleteRegistryValue
  • OpenSubKey64
  • ParseRegistryHive
  • NotifyAppsOfRegistryChange

Of all these, just one doesn’t require using registry data: ParseRegistryHive. The method’s purpose is to return the hive enum given a string.

        public static RegistryHive ParseRegistryHive(string hiveName)
        {
            if (hiveName.ToUpper() == "HKEY_CLASSES_ROOT") { return RegistryHive.ClassesRoot; }
            else if (hiveName.ToUpper() == "HKEY_CURRENT_USER") { return RegistryHive.CurrentUser; }
            else if (hiveName.ToUpper() == "HKEY_LOCAL_MACHINE") { return RegistryHive.LocalMachine; }
            else if (hiveName.ToUpper() == "HKEY_USERS") { return RegistryHive.Users; }
            else if (hiveName.ToUpper() == "HKEY_CURRENT_CONFIG") { return RegistryHive.CurrentConfig; }
            //last chance, will throw error if invalid
            else return (RegistryHive)Enum.Parse(typeof(RegistryHive), hiveName);
        }

I’m using xUnit.net for my unit testing. xUnit’s Theories are pretty nice, allowing testing sets of data. My tests verify that the method throws errors—or doesn’t—appropriately, and also that it returns the correct hive. I wrote a note to myself about that last one, summed up as:

Lesson 2: Don’t copy/paste any code from your test into the method you’re testing. What if your test has a bug?

    public class RegistryService_ParseRegistryHive_Should
    {
        [Theory]
        [InlineData("HKEY_CLASSES_ROOT")]
        [InlineData("HKEY_CURRENT_USER")]
        [InlineData("HKEY_LOCAL_MACHINE")]
        [InlineData("HKEY_USERS")]
        [InlineData("HKEY_CURRENT_CONFIG")]
        [InlineData("hkey_current_config")]
        public void NotThrowErrorWithValidHive(string name)
        {
            var hive = RegistryService.ParseRegistryHive(name);
        }

        [Theory]
        [InlineData("HKEY_CLASSES_ROOT")]
        [InlineData("HKEY_CURRENT_USER")]
        [InlineData("HKEY_LOCAL_MACHINE")]
        [InlineData("HKEY_USERS")]
        [InlineData("HKEY_CURRENT_CONFIG")]
        public void ReturnCorrectEnumWithValidHive(string name)
        {
            //This basically reproduces the method code. Need to NOT copy/paste from method, but
            //instead rewrite. In TDD, would NOT copy/paste from the test, otherwise there's a danger
            //the test duplicates a wrong condition.
            var hive = RegistryService.ParseRegistryHive(name);
            switch (name)
            {
                case "HKEY_CLASSES_ROOT":
                    Assert.True(hive == Microsoft.Win32.RegistryHive.ClassesRoot);
                    break;
                case "HKEY_CURRENT_USER":
                    Assert.True(hive == Microsoft.Win32.RegistryHive.CurrentUser);
                    break;
                case "HKEY_LOCAL_MACHINE":
                    Assert.True(hive == Microsoft.Win32.RegistryHive.LocalMachine);
                    break;
                case "HKEY_USERS":
                    Assert.True(hive == Microsoft.Win32.RegistryHive.Users);
                    break;
                case "HKEY_CURRENT_CONFIG":
                    Assert.True(hive == Microsoft.Win32.RegistryHive.CurrentConfig);
                    break;
                default:
                    Assert.True(false,name + " didn't return a valid hive. Hive enum returned was " + hive);
                    break;
            }
        }

        [Theory]
        [InlineData("fail")]
        [InlineData("")]
        public void ThrowErrorWithInvalidHive(string name)
        {
            Assert.Throws(() =>  RegistryService.ParseRegistryHive(name));
        }
    }

Act 3 – Testing the Difficult: Mock it, or . . .

I’ve been putting it off long enough. What about the rest of the methods? You know, the ones that could completely screw up my registry?

First, which methods do testable work? Some of them are just wrappers so that I can get back a better error message, for example:

        public static void SetRegistryKeyValue(string keyName, string valueName, string value, RegistryValueKind dataType = RegistryValueKind.String)
        {
            try
            {
                Registry.SetValue(keyName, valueName, value, dataType);
            }
            catch (Exception ex)
            {
                throw new Exception("Error setting registry value keyName '" + keyName + "', valueName '" + valueName + ", value '" + value + "'\r\n" + ex.GetBaseException().Message);
            }
        }

Do I need to test this? Well, I’d like to test that I get back the right exception, so…hmm. OK, let’s see if anyone has mocked the registry.

In the last bullet, I quote from an excellent Stack Overflow response on the subject. In fact, here are two comments.

The research tells me I have two choices:

  1. Mock the concrete Registry class
  2. Use a wrapper around the Registry class

I looked at the Prig example. Whew! That’s a lot of setup! And I’m not prepared to pay for TypeMock or JustMock, though I expect they’re excellent.

This means wrapping the Registry class. Before I try out SystemWrapper, I’m going to create my own wrapper. I want to understand what’s involved. (This is self-education, after all!) The methods I’m using are:

Registry.GetValue
Registry.SetValue
RegistryKey.GetValueKind
RegistryKey.DeleteSubKey
RegistryKey.DeleteValue
RegistryKey.OpenBaseKey
RegistryKey.OpenBaseKey.OpenSubKey

All right, boys! Wrap…That…Code!!!

I had to rethink my whole approach to my RegistryService class, which consumes the Microsoft.Win32.Registry. Originally I wrote it as a static class, since it didn’t need to be instantiated. But this class now could depend on different “registry providers.” I want to protect the class from being misused. A static SetRegistryProvider method won’t work, because of the danger that in production one caller sets it one way and another sets it differently. It’s the same as if a class could use more than one database back end; you wouldn’t make it static. The solution was to remove static, and make the class entirely instance-able.

Moving on, the Registry wrapper is pretty straight forward. The oddity is that I’m wrapping a static class in an instance.

    public interface IRegistryWrapper
    {
        object GetValue(string keyName, string valueName, object defaultValue);
        void SetValue(string keyName, string valueName, object value, RegistryValueKind valueKind);
    }

    public class RegistryWrapper : IRegistryWrapper
    {
        public object GetValue(string keyName, string valueName, object defaultValue)
        {
            return Registry.GetValue(keyName, valueName, defaultValue);
        }

        public void SetValue(string keyName, string valueName, object value, RegistryValueKind valueKind)
        {
            Registry.SetValue(keyName, valueName, value, valueKind);
        }
    }

The RegistryKey wrapper presented a challenge. The constructor for RegistryKey is private. You cannot create an instance of RegistryKey this way:

RegistryKey key = new RegistryKey(); //this won't compile

Instead, instances are created using the static method OpenBaseKey().

RegistryKey key = RegistryKey.OpenBaseKey(RegistryHive.CurrentUser, RegistryView.Registry64);

My abstraction needs to be clear, I want to create an interface, but interfaces can’t have static members.

A base key is required for RegistryKey to function, therefore it’s required for my wrapper to function. But the RegistryService class, which uses RegistryKey, encapsulates which base key will be used; each method could instantiate its own RegistryKey with different base keys. For example:

public void DeletePluginVersionValue(string id)
{
    //_registryKey was injected via the constructor
    RegistryKey basekey = RegistryKey.OpenBaseKey(RegistryHive.LocalMachine, RegistryView.Default);
    using (RegistryKey key = basekey.OpenSubKey(@"SOFTWARE\EduAgent\RegistryPlugin", false))
    {
        key.DeleteValue("Value");
        key.Close();
    }
}

My abstracted class is going to work similarly.

public void DeletePluginVersionValue(string id)
{
    //_registryKey was injected via the constructor
    IRegistryKeyWrapper basekey = _registryKey.OpenBaseKey(RegistryHive.LocalMachine, RegistryView.Default);
    using (IRegistryKeyWrapper key = basekey.OpenSubKey(@"SOFTWARE\EduAgent\RegistryPlugin",false))
    {
        key.DeleteValue("Value");
        key.Close();
    }
}

But for testing, I need to pass in a non-null class instance that implements an interface, but does not have a base class yet. I can’t do this:

    public interface IRegistryKeyWrapper: IDisposable
    {
        IRegistryKeyWrapper OpenSubKey(string name, bool writable);
        //originally static
        IRegistryKeyWrapper OpenBaseKey(RegistryHive hKey, RegistryView view);
    }

    public class RegistryKeyWrapper : IRegistryKeyWrapper
    {
        private RegistryKey _baseKey;
        private RegistryKeyWrapper(RegistryKey key)
        {
            _baseKey = key;
        }
        //etc.

Why not? Follow along.

The RegistryService constructor:

    public class RegistryService
    {
        private IRegistryWrapper _registry;
        private IRegistryKeyWrapper _registryKey;

        public RegistryService(IRegistryWrapper registry, IRegistryKeyWrapper key)
        {
            _registry = registry;
            _registryKey = key;
        }

Using the RegistryService:

//this won't compile; RegistryKeyWrapper requires base key!
RegistryService service = new RegistryService(new RegistryWrapper(), new RegistryKeyWrapper());

I need to be able to create a new RegistryKeyWrapper(), but my live registry wrapper right now requires an actual registry base key. The (unfortunate) solution is to allow the user to create an instance of the class that can’t work.

    public class RegistryKeyWrapper : IRegistryKeyWrapper
    {
        private RegistryKey _baseKey;

        /// 
        /// Note the class can't be used until instantiated using OpenBaseKey or setting to another RegistryKeyWrapper.
        /// This constructor is here to allow an instance to be passed via dependency injection.
        /// 
        public RegistryKeyWrapper() { }

        private RegistryKeyWrapper(RegistryKey key)
        {
            _baseKey = key;
        }

In the RegistryService example, above, if the user were to use _registryKey immediately, without setting it using OpeBaseKey, it would fail because the private variable _baseKey has not been set.

I wouldn’t call this ideal, but it does allow us to test the class.

EDIT 8/31/16
-------------------------------------------------------------

Not only is it not ideal, but it’s not even good. What I’m trying to do is wrap RegistryKey in a pass-through manner. But that’s a mistake, because it implements IDisposable and in my wrapper I should not implement IDisposable.

In a later post, I’ll explain how I rewrote my RegistryKey wrapper in a better way. It doesn’t affect the validity of the info in this post.

-------------------------------------------------------------

First, I’ll fix up my previous tests for ParseRegistryHive that relied on Registry. In my unit test project I create the mock classes. At this point, I don’t even make the classes work, since ParseRegistryHive doesn’t depend on any methods.

    class RegistryMock : IRegistryWrapper
    {
        public object GetValue(string keyName, string valueName, object defaultValue)
        {
            throw new NotImplementedException();
        }

        public void SetValue(string keyName, string valueName, object value, RegistryValueKind valueKind)
        {
            throw new NotImplementedException();
        }
    }

And update a test to use the new service, passing in the mocks.

        IRegistryWrapper _registry = new RegistryMock();
        IRegistryKeyWrapper _registryKey = new RegistryKeyMock();
                
        [Theory]
        [InlineData("HKEY_CLASSES_ROOT")]
        [InlineData("HKEY_CURRENT_USER")]
        [InlineData("HKEY_LOCAL_MACHINE")]
        [InlineData("HKEY_USERS")]
        [InlineData("HKEY_CURRENT_CONFIG")]
        [InlineData("hkey_current_config")]
        public void NotThrowErrorWithValidHive(string name)
        {
            var service = new RegistryService(_registry, _registryKey);
            var hive = service.ParseRegistryHive(name);
        }

Now, create a test for the RegistryMock.

    public class RegistryService_GetRegistryKeyValue_Should
    {
        IRegistryWrapper _registry = new RegistryMock();
        IRegistryKeyWrapper _registryKey = new RegistryKeyMock();

        [Fact]
        public void ReturnNullConstantTextIfValueIsMissing()
        {
            var service = new RegistryService(_registry, _registryKey);
            var result = service.GetRegistryKeyValue(@"HKEY_LOCAL_MACHINE\SOFTWARE\EduAgent", "Version");
            Assert.Equal(result, service.NoValue);
        }
    }

Which requires RegistryMock.GetValue to be implemented:

    class RegistryMock : IRegistryWrapper
    {
        public object GetValue(string keyName, string valueName, object defaultValue)
        {
            //this may become cumbersome, but for now specify some return values
            switch (keyName)
            {
                case @"HKEY_LOCAL_MACHINE\SOFTWARE\EduAgent":
                    if (valueName == "Version") return defaultValue;
                    break;
            }
            throw new Exception("Invalid keyName");
        }

Running the test, it passes.

Next up, using the RegistryKeyMock. Here’s the test:

    public class RegistryService_OpenSubKey64_Should
    {
        IRegistryWrapper _registry = new RegistryMock();
        IRegistryKeyWrapper _registryKey = new RegistryKeyMock();

        [Fact]
        public void ThrowExceptionWhenUsingInvalidKey()
        {
            var service = new RegistryService(_registry, _registryKey);
            var ex = Assert.Throws(() => service.OpenSubKey64(@"HKEY_LOCAL_MACHINE\SOFTWARE\EduAgentMissing"));
            Assert.Equal("Error opening subkey 'HKEY_LOCAL_MACHINE\\SOFTWARE\\EduAgentMissing'. Operation returned null.", ex.Message);
        }
    }

Here are the relevant methods I had to implement. This one was kind of tricky because I have to set properties that are read only. I’m making use of the latest C# language features to set the value of read only properties in the constructor. I also needed a helper method to return the string representation of a hive name.

    public class RegistryKeyMock : IRegistryKeyWrapper
    {
        public RegistryKeyMock() { }
        public RegistryKeyMock(string name, int subKeyCount, int valueCount, RegistryView view)
        {
            Name = name;
            SubKeyCount = subKeyCount;
            ValueCount = valueCount;
            View = view;
        }

        public string Name { get; }
        public int SubKeyCount { get; }
        public int ValueCount { get; }
        public RegistryView View { get; }

        public IRegistryKeyWrapper OpenBaseKey(RegistryHive hKey, RegistryView view)
        {
            return new RegistryKeyMock(HiveToString(hKey), 10, 10, view);
        }

        public IRegistryKeyWrapper OpenSubKey(string name, bool writable)
        {
            if (name == @"SOFTWARE\EduAgentMissing")
                return null;
            return new RegistryKeyMock(Name + "\\" + name, SubKeyCount, ValueCount, View);
        }

        private string HiveToString(RegistryHive hive)
        {
            string key = "HKEY_";
            if (hive == RegistryHive.ClassesRoot) return key + "CLASSES_ROOT";
            if (hive == RegistryHive.CurrentConfig) return key + "CURRENT_CONFIG";
            if (hive == RegistryHive.CurrentUser) return key + "CURRENT_USER";
            if (hive == RegistryHive.LocalMachine) return key + "LOCAL_MACHINE";
            if (hive == RegistryHive.Users) return key + "USERS";
            return "";
        }
    }

Test it, it passes, love those green lights!

Tag

This was like a special made-for-TV movie of Fantastic Journey. With all the stuff I’ve done, what haven’t I? Well, there are two big ones:

But I’m leaving those for a future post. Otherwise, like our intrepid explorers, I’ll be canceled and never get back to my own time and place.

No comments:

Post a Comment