4
\$\begingroup\$

I am developing a project (which purpose is not relevant for this question) where I have a class responsible for retrieving a list of running Windows processes. The implementation will basically use the System.Diagnostics.Process.GetProcesses() call but I want to abstract that, so I don't have the call of that static method at the code.

As a model I have:

namespace ProcessExample.Model
{
    /// <summary>
    /// Representation of a Windows process.
    /// </summary>
    public class ProcessInfo
    {
        public string Name { get; set; }
        public long MemoryPrivateWorkingSet { get; set; }
    }
}

As you can see, I decided to keep the model as a POCO, so no dependencies whatsoever, instead of just using the System.Diagnostics.Process which is the returned object from the System.Diagnostics.Process.GetProcesses().

This way, I will build later on a ProcessInfo out of a System.Diagnostics.Process, copying from only the few properties I am interested.

Now it follows a possible implementation for the ProcessReader:

namespace ProcessExample.BusinessLogic
{
    /// <summary>
    /// Gets a list of running processes.
    /// </summary>
    public class ProcessReader
    {
        public ICollection<ProcessInfo> GetProcesses()
        {
            var processInfoList = new List<ProcessInfo>();

            Process[] processes = System.Diagnostics.Process.GetProcesses();
            foreach (var process in processes)
            {
                var processInfo = new ProcessInfo();
                processInfo.Name = process.ProcessName;
                processInfo.MemoryPrivateWorkingSet = process.WorkingSet64;

                processInfoList.Add(processInfo);
            }
            return processInfoList;
        }
    }
}

As seen above, this code calls the static method System.Diagnostics.Process.GetProcesses() and it is something I will improve soon.

With this implementation, it's a pain during testing, leading to performance penalties since it's actually reading a list of process running on the test machine, besides giving low flexibility for the tests. It quite limits what we can do with testing, as the tests are pretty vague, specially the last one:

namespace ProcessExample.BusinessLogic.Tests
{
    [TestFixture]
    public class ProcessReaderTests
    {
        [Test]
        public void     GetProcesses_Should_get_a_list_of_ProcessInfo_When_called()
        {
            // arrange
            var processReader = new ProcessReader();

            // act
            var processes = processReader.GetProcesses();

            // assert           
            Assert.That(processes.Count, Is.Not.Zero);
        }

        [Test]
        public void GetProcesses_Should_get_a_list_of_filled_in_ProcessInfo_When_called()
        {
            // arrange
            var processReader = new ProcessReader();

            // act
            var processes = processReader.GetProcesses();

            // assert           
            Assert.That(processes.First().Name, Is.Not.Empty);
            Assert.That(processes.First().MemoryPrivateWorkingSet, Is.Not.Zero);
        }
    }
}

An improvement on the design is introduced on the ProcessReader2:

namespace ProcessExample.BusinessLogic
{
    /// <summary>
    /// Gets a list of running processes.
    /// </summary>
    public class ProcessReader2
    {
        private readonly ISystemDiagnosticsProcessWrapper _systemDiagnosticsProcessWrapper;

        public ProcessReader2(ISystemDiagnosticsProcessWrapper systemDiagnosticsProcessWrapper)
        {
            _systemDiagnosticsProcessWrapper = systemDiagnosticsProcessWrapper;
        }

        public ICollection<ProcessInfo> GetProcesses()
        {
            var processInfoList = new List<ProcessInfo>();

            Process[] processes = _systemDiagnosticsProcessWrapper.GetProcesses();
            foreach (var process in processes)
            {
                var processInfo = new ProcessInfo();
                processInfo.Name = process.ProcessName;
                processInfo.MemoryPrivateWorkingSet = process.WorkingSet64;

                processInfoList.Add(processInfo);
            }
            return processInfoList;
        }
    }
}


public interface ISystemDiagnosticsProcessWrapper
{
    Process[] GetProcesses();
}

namespace ProcessExample.BusinessLogic
{
    public class SystemDiagnosticsProcessWrapper : ISystemDiagnosticsProcessWrapper
    {
        public Process[] GetProcesses()
        {
            return System.Diagnostics.Process.GetProcesses();
        }
    }
}

Now when I try to unit test the ProcessReader2, I face the problem that although I can instantiate a System.Diagnostics.Process, the properties ProcessName and WorkingSet64 are read-only, so the last test is not possible to setup:

namespace ProcessExample.BusinessLogic.Tests
{
    [TestFixture]
    public class ProcessReader2Tests
    {
        [Test]
        public void GetProcesses_Should_get_a_list_of_ProcessInfo_When_called()
        {
            // arrange
            var systemDiagnosticsProcessWrapper = new Mock<ISystemDiagnosticsProcessWrapper>();
            systemDiagnosticsProcessWrapper.Setup(s => s.GetProcesses()).Returns(new[] {new Process()});// {ProcessName = "process1", WorkingSet64 = 100}});

            var processReader = new ProcessReader2(systemDiagnosticsProcessWrapper.Object);

            // act
            var processes = processReader.GetProcesses();

            // assert           
            Assert.That(processes.Count, Is.Not.Zero);
        }

        [Test]
        public void GetProcesses_Should_get_a_list_of_filled_in_ProcessInfo_When_called()
        {
            // arrange
            var systemDiagnosticsProcessWrapper = new Mock<ISystemDiagnosticsProcessWrapper>();
            //systemDiagnosticsProcessWrapper.Setup(s => s.GetProcesses()).Returns(new[] { new Process {ProcessName = "process1", WorkingSet64 = 100}});  // Doesn't compile as ProcessName and WorkingSet64 are read only.

            var processReader = new ProcessReader2(systemDiagnosticsProcessWrapper.Object);

            // act
            var processes = processReader.GetProcesses();

            // assert           
            Assert.That(processes.First().Name, Is.EqualTo("process1"));
            Assert.That(processes.First().MemoryPrivateWorkingSet, Is.EqualTo(100));
        }
    }
}

How can I improve the design so that I can properly unit-testing it and/or TDD it?

\$\endgroup\$
  • \$\begingroup\$ I'm sorry but can you elaborate more on - why is this better than a simple mapper? \$\endgroup\$ – Denis Jul 7 '17 at 20:37
  • \$\begingroup\$ @Denis: Risking asking a basic question but what do you mean by a mapper? \$\endgroup\$ – Pedro Duarte Jul 8 '17 at 16:01
  • \$\begingroup\$ A mapper is an object or a family of classes that deal with conversion between different types. Often used place would be in asp.net - Product -> ProductDTO and vice versa. You copy some properties from ObjectA to ObjectB, you might take only a few or you might modify some. It's similar to what you're doing in your first code snippet. It seems to me that you're over-complicating the code later on. I might write an answer on that but I need to know if I'm missing some advantage that your version is offering compared to the one I mentioned earlier. There are no basic questions :) \$\endgroup\$ – Denis Jul 8 '17 at 16:20
  • \$\begingroup\$ Thanks. My intention is actually not the service I wrote (could be one doing something totally different) but how to solve the challenge I face into: How can I fully unit-test this particular ProcessReader2.GetProcesses() method, covering too the mapper kind of functionality, since I cannot set the relevant System.Diagnostics.Process read-only methods? Maybe with some funky testing framework such as Microsoft Fakes? Or somehow changing the model or using a difference architecture for my implemented code? \$\endgroup\$ – Pedro Duarte Jul 9 '17 at 20:57
  • 1
    \$\begingroup\$ Please see What should I do when someone answers my question? --- you may not add the improved version to the question (a follow-up or a self-answer would be ok). Please revert it. Otherwise someone else will do it for you ;-) \$\endgroup\$ – t3chb0t Jul 10 '17 at 16:29
4
\$\begingroup\$
[TestFixture]
public class ProcessReaderTests
{
    [Test]
    public void     GetProcesses_Should_get_a_list_of_ProcessInfo_When_called()
    {
        // arrange
        var processReader = new ProcessReader();

        // act
        var processes = processReader.GetProcesses();

        // assert           
        Assert.That(processes.Count, Is.Not.Zero);
    }

    [Test]
    public void GetProcesses_Should_get_a_list_of_filled_in_ProcessInfo_When_called()
    {
        // arrange
        var processReader = new ProcessReader();

        // act
        var processes = processReader.GetProcesses();

        // assert           
        Assert.That(processes.First().Name, Is.Not.Empty);
        Assert.That(processes.First().MemoryPrivateWorkingSet, Is.Not.Zero);
    }
}

These two tests are virtually the only useful tests you have so far where you validate that the ProcessReader actually works and retrieves processes. What you could additionaly do is to check whether the explorer.exe or something common is on the list.

Now I'll ask why are you trying to abstract it? I'll answer it for you: because you want to be able to exchange this service in order to return a fake process list for another modules that uses it. This means that all you need is an interface like you already have it (I ignore the fact that the name isn't a good one IProcessReader would be enough):

public interface ISystemDiagnosticsProcessWrapper
{
    Process[] GetProcesses();
}

[Test]
public void GetProcesses_Should_get_a_list_of_ProcessInfo_When_called()
{
    // arrange
    var systemDiagnosticsProcessWrapper = new Mock<ISystemDiagnosticsProcessWrapper>();
    systemDiagnosticsProcessWrapper.Setup(s => s.GetProcesses()).Returns(new[] {new Process()});// {ProcessName = "process1", WorkingSet64 = 100}});

    var processReader = new ProcessReader2(systemDiagnosticsProcessWrapper.Object);

    // act
    var processes = processReader.GetProcesses();

    // assert           
    Assert.That(processes.Count, Is.Not.Zero);
}

With this test however you test that the mock that you created returns a fake process list that you created.... this is pointless as this doesn't test anything useful. You are testing the mock itslef.

It would be fine to mock the IProcessReader if you used it as a dependency for something else that requries a list of processe but in this test, there is no such thing. This test has no value.


I find the design with the interface is ok but the tests are not. You need to actually mock it and inject it and test something else that depends on it.


There's one more thing I forgot but you mentioned it in a comment. You need to fake the Process too but it's properties are not settable but you can mock an interface so you'll need another wrapper.

interface IProcess
{
    string ProcessName { get; }
    long WorkingSet64 { get; }
}

internal class Process2 : IProcess 
{
    private readonly Process process;

    public Process2(Process process) 
    {
        this.process = process;
    }

    public string ProcessName => process.ProcessName;

    public long WorkingSet64 => process.WorkingSet64;
 }

now you IProcessReader could look like this:

public interface IProcessReader
{
    IProcess[] GetProcesses();
}

with these interfaces you should be able to create the desired tests.

\$\endgroup\$
  • \$\begingroup\$ "What you could additionaly do is to check whether the explorer.exe or something common is on the list": No, I don't want to check for real processes. My goal is to use a mock during testing, that is why I introduced the interface. \$\endgroup\$ – Pedro Duarte Jul 8 '17 at 16:12
  • \$\begingroup\$ @PedroDuarte I think you've missed the point. Currently you are testing your mock which is useless. \$\endgroup\$ – t3chb0t Jul 8 '17 at 16:19
  • \$\begingroup\$ I do agree that the test GetProcesses_Should_get_a_list_of_ProcessInfo_When_called()is pointless when checking the returned list is not empty since the list comes from a mock. However your tests are missing the verification of the funcionality of ProcessReader2 where it sets a ProcessInfo.Name and ProcessInfo.MemoryPrivateWorkingSet from a Process.ProcessName and Process.WorkingSet64 respectively. \$\endgroup\$ – Pedro Duarte Jul 8 '17 at 16:30
  • \$\begingroup\$ What I am trying to achieve with my question is the cleanest way to mock the retrieval of processes so that the tests are not dealing with real process retrieval. The problem I face is that, although I can instantiate a System.Diagnostics.Process I cannot set its ProcessName and WorkingSet64 properties. \$\endgroup\$ – Pedro Duarte Jul 8 '17 at 16:31
  • \$\begingroup\$ t3chb0t, if you refer to my originally posted test GetProcesses_Should_get_a_list_of_filled_in_ProcessInfo_When_called() (long name, I know :)) you see that I am trying to test ProcessReader2method if I was able to set the read only properties of Process (commented line). \$\endgroup\$ – Pedro Duarte Jul 9 '17 at 16:31
2
\$\begingroup\$

The second version is over-engineered in my opinion. The code bloat it introduces is obvious, the benefits are not. I think this api:

interface IProcessReader
{
    ICollection<ProcessInfo> GetProcesses();
}

is more than enough. Use actual implementation in your software, mock it in your tests. Done. Introducing another internal abstraction leads to creating even more abstractions (see t3chb0t's point about IProcess) and writing even more pointless tests (if you are obsessive about test coverage). I wouldn't recommend going down that road.

\$\endgroup\$
  • \$\begingroup\$ What you advised is fine if you only want to write tests for other classes that use IProcessReader. But what I want is to write a test for the ProcessReader implementation. I am not concerned for now with other classes that will use it nor with test coverage: My goal is to unit test ProcessReaderand its fully implicit functionality and/or go the TDD way. \$\endgroup\$ – Pedro Duarte Jul 10 '17 at 16:35
  • \$\begingroup\$ @PedroDuarte what is there to test? What is essentially a one liner return Process.GetProcesses().Select(p => new ProcessInfo(p)).ToList()? But without Process.GetProcesses() part? Well... I guess you can test a ProcessInfo constructor using current process. But it is still a pretty "meh" test. \$\endgroup\$ – Nikita B Jul 11 '17 at 8:08

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.