Unit testing challenge

challenge.md

I'm looking for the cleanest way to make the test below pass.

The rules are:

1. It must be a unit test (so don't touch the file system).
2. The public signature for CodeUnderTest must not be changed. can be extended not reduced
3. You're allowed to use DI/your favorite mocking framework.
4. You can only add code where you see /**/.

        [Test]
        public void Test()
        {
            /**/
            var text = CodeUnderTest.GetUpperText("foo.txt" /**/);

            Assert.That(text, Is.EqualTo("BAR"));
            /**/
        }

        public class CodeUnderTest
        {
            /**/
            public static string GetUpperText(string path /**/)
            {
                var text = File.ReadAllText(path);
                return text.ToUpperInvariant();
            }
            /**/
        }

Please comment below or tweet me @jcansdale.

Here you go; no need to change the test:

public class CodeUnderTest
{
    private class AllTextReader
    {
        public string ReadAllText(string _)
        {
            return "BAR";
        }
    }

    static AllTextReader File = new AllTextReader();

    public static string GetUpperText(string path /**/)
    {
        var text = File.ReadAllText(path);
        return text.ToUpperInvariant();
    }
}

Dammit, I should have known someone would do that! 😉

How about we change the test to this?

        [TestCase("foo.txt", "foo", "FOO")]
        [TestCase("bar.txt", "Bar", "BAR")]
        public void GetUpperText(string filePath, string fileText, string expected)
        {
                /**/
                var text = CodeUnderTest.GetUpperText(filePath /**/);

                Assert.That(text, Is.EqualTo(expected));
                /**/
            }
        }

Also, assume the test is being called from an external assembly and no InternalsVisibleToAttribute funny business!

BTW, I was hoping you would bite, being the resident DI guru. It's good to have your thoughts!

Easy 😉

public class CodeUnderTest
{
    private class AllTextReader
    {
        public string ReadAllText(string fileName)
        {
            return System.IO.Path.GetFileNameWithoutExtension(fileName).ToUpper();
        }
    }

    static AllTextReader File = new AllTextReader();

    public static string GetUpperText(string path /**/)
    {
        var text = File.ReadAllText(path);
        return text.ToUpperInvariant();
    }
}

Your move: 😉

        [Test]
        public void GetUpperText()
        {
            var filePath = Guid.NewGuid() + ".txt";
            var fileText = Guid.NewGuid().ToString();
            var expect = fileText.ToUpperInvariant();
            /**/
            var text = CodeUnderTest.GetUpperText(filePath /**/);

            Assert.That(text, Is.EqualTo(expect));
        }

Now you're forcing my hand. This is no longer pretty, and I'd never write code like this, but I can't think of anything else since I can't change the body of GetUpperText...

[Test]
public void GetUpperText()
{
    var filePath = Guid.NewGuid() + ".txt";
    var fileText = Guid.NewGuid().ToString();
    var expect = fileText.ToUpperInvariant();

    AllTextReader.Instance.Imp = _ => expect;

    var text = CodeUnderTest.GetUpperText(filePath /**/);

    Assert.That(text, Is.EqualTo(expect));
}

public class AllTextReader
{
    private AllTextReader() { }

    public static AllTextReader Instance = new AllTextReader();

    public Func<string, string> Imp { get; set; }

    public string ReadAllText(string fileName)
    {
        return this.Imp(fileName);
    }
}

public class CodeUnderTest
{
    static AllTextReader File = AllTextReader.Instance;

    public static string GetUpperText(string path /**/)
    {
        var text = File.ReadAllText(path);
        return text.ToUpperInvariant();
    }
}

I reject your second constraint. The type signature tightly-couples the side-effect of reading the filesystem with the pure transformation of text.

If you decouple them, and I believe you should, then the side-effect is explicitly ignored by your first constraint, and the remainder is probably too trivial to test. ;-)

I like the way you haven't touched the CodeUnderTest interface at all. That's probably about as clean as you can make it. Easy to forget to reset Imp though.

Here it is written using the library I've been working on:

        using StaticMocks; // Find it on NuGet.
        using NSubstitute;
        using NUnit.Framework;

        [Test]
        public void GetUpperText()
        {
            using (var staticMock = new StaticMock(typeof(CodeUnderTest)))
            {
                staticMock.For(() => File.ReadAllText("foo.txt")).Returns("bar");

                var text = CodeUnderTest.GetUpperText("foo.txt");

                Assert.That(text, Is.EqualTo("BAR"));
            }
        }

        public class CodeUnderTest
        {
            public static string GetUpperText(string path)
            {
                var text = File.ReadAllText(path);
                return text.ToUpperInvariant();
            }

            class File
            {
                internal static Func<string, string> ReadAllText = (string path) => System.IO.File.ReadAllText(path);
            }
        }

You can find more information here:
https://github.com/jcansdale/StaticMocks

I'd be interested to hear what you think. 😄

Add a IFileDataProvider but at this point your test has essentially become useless as it just tests string.ToUpperInvariant() you will later need a test on FileDapaProvider that it actually reads the data. That said hitting a small file in a unit test is not the end of the world. People like to get caught up in whether something is an integration test, a unit test, an acceptance test, etc; What I am interested in is "is it fast or slow"

I should add that "static mocking the file method" essentially does the same.

TDD is not the be all end all of everything. I would suggest spending time on something more productive like learning queue theory.

I reject your second constraint.

@krisajenkins Fair call. I've updated it.

@ploeh What would you have done if you could change the body of GetUpperText?

TDD is not the be all end all of everything.

@gregoryyoung Agreed, I've written some terrible tests in my time. I think "is it obvious what's going on" and "does it run in a reasonable time" are the important ones.

Indeed, what your StaticMocks library requires from the SUT seems to be fairly close to what I ended up doing. There still seems to be some clever wizardry going on that I can't quite figure out 😄

What would I have done if I could change the body of GetUpperText? Well, used proper dependency injection!

[Test]
public void GetUpperText()
{
    var filePath = Guid.NewGuid() + ".txt";
    var fileText = Guid.NewGuid().ToString();
    var expect = fileText.ToUpperInvariant();

    var td = new Mock<ITextReader>();
    td.Setup(r => r.ReadAllText(filePath)).Returns(expect);

    var text = CodeUnderTest.GetUpperText(filePath, td.Object);

    Assert.That(text, Is.EqualTo(expect));
}

public interface ITextReader
{
    string ReadAllText(string path);
}

public class CodeUnderTest
{
    public static string GetUpperText(string path, ITextReader reader)
    {
        var text = reader.ReadAllText(path);
        return text.ToUpperInvariant();
    }
}

My next step, if I could change things even more around, would be to make CodeUnderTest a proper class, so that I could use Constructor Injection instead of Method Injection.

Going even further, if I could change things arbitrarily, would be to migrate to F# and get rid of the sorry mess altogether, but that's a different story 😉

While I agree with Greg, I consider this challenge as a stand-in for a more complicated problem, so I'm not so worried about 'testing that ToUpperInvariant works' 😄

@ploeh,

You did have the option of doing this: 😄

            public static string GetUpperText(string path, ITextReader File)
            {
                var text = File.ReadAllText(path);
                return text.ToUpperInvariant();
            }

I understand that starting a parameter with with a capital might break C# coding convention, but I might be inclined to bend this rule because it makes it pretty clear what it's a stand-in for.

If you have time, I'd be interested to see what this (and the test) would look like in F#. Would it make it that much cleaner? I've dabbled with F#, but I'd be interested to see how an F# advocate would code it.

The idea behind StaticMocks is that it allows you to do dependency injection of static methods whilst barely touching the target code (baring a single line private nested class). I think this is useful for some commonly used static classes (e.g. File), when I don't particularly want to create/implement a public interface just so that I can test my code.

A good functional implementation of this challenge is trivial, so here we suffer from the simplicity of the original problem statement. While you can call File.ReadAllText from an F# function, it's by definition impure. The property of impurity is transitive, so because File.ReadAllText is impure, any function calling it would also be impure.

In functional programming, we really, really like our functions to be pure, so we'd refactor any impure indirect input to direct input. Since the output type of File.ReadAllText is string, this can be refactored into a function that takes a string as input:

let getUpperText (text : string) = text.ToUpperInvariant ()

This is where the simplicity of the example begins to show, because the getUpperText function doesn't seem to add much value... Again, we should consider it a stand-in for more complex logic.

Since this function is pure, it's easy to unit test:

[<Theory>]
[<InlineData("foo", "FOO")>]
[<InlineData("bar", "BAR")>]
let ``getUpperText returns correct value`` input expected =
    let actual = getUpperText input
    Assert.Equal(expected, actual)

In a production system, then, you can compose it with any source of string values, including the contents of files:

// string -> string
File.ReadAllText >> getUpperText

That example composes a function that takes a string (a file path) as input, and returns a string.

For a more complex demonstration of such separation of pure and impure functions, see e.g. this article: http://blog.ploeh.dk/2016/03/18/functional-architecture-is-ports-and-adapters

This is where the simplicity of the example begins to show, because the getUpperText function doesn't seem to add much value... Again, we should consider it a stand-in for more complex logic.

I wanted a simple stand-in that wouldn't obscure what was going on. Looks like I made it a little too simple!

How about we up the complexity slightly to this:

    public static string GetUpperText(string path)
    {
        if(!File.Exists(path)) return "DEFAULT";
        var text = File.ReadAllText(path);
        return text.ToUpperInvariant();
    }

Testing it using StaticMocks would look like this (expand)

using StaticMocks; // Find it on NuGet.
using NSubstitute;
using NUnit.Framework;
using System;
using System.IO;

public class Tests
{
    [Test]
    public void GetUpperText_FromFile()
    {
        using (var staticMock = new StaticMock(typeof(CodeUnderTest)))
        {
            staticMock.For(() => File.Exists(null)).ReturnsForAnyArgs(false);
            staticMock.For(() => File.Exists("foo.txt")).Returns(true);
            staticMock.For(() => File.ReadAllText("foo.txt")).Returns("bar");

            var text = CodeUnderTest.GetUpperText("foo.txt");

            Assert.That(text, Is.EqualTo("BAR"));
        }
    }

    [Test]
    public void GetUpperText_Default()
    {
        using (var staticMock = new StaticMock(typeof(CodeUnderTest)))
        {
            staticMock.For(() => File.Exists(null)).ReturnsForAnyArgs(false);
            staticMock.For(() => File.ReadAllText(null)).Returns(args =>
            {
                throw new FileNotFoundException((string)args[0]);
            });

            var text = CodeUnderTest.GetUpperText("foo.txt");

            Assert.That(text, Is.EqualTo("DEFAULT"));
        }
    }
}

public class CodeUnderTest
{
    public static string GetUpperText(string path)
    {
        if(!File.Exists(path)) return "DEFAULT";
        var text = File.ReadAllText(path);
        return text.ToUpperInvariant();
    }

    class File
    {
        internal static Func<string, string> ReadAllText = (string path) => System.IO.File.ReadAllText(path);
        internal static Func<string, bool> Exists = (string path) => System.IO.File.Exists(path);
    }
}

How would testing that look in F#?

Am I correct in thinking all top level F# functions are public? Is it considered good practice to have your pure functions public and hence accessible for testing? If I was writing C# in a more functional style, wouldn't many of these functions tend to be private?

Thanks for the links. This is all very interesting. 😄

Looks like a method containing both file system operations and business logic. I'd split them.

One method (impure) for getting the file content, or null if no file.
One method (pure) containing the rules for how to format the result.

These methods would be standalone and you would only have to change one of them if the data layer or business rules changed. Integration tests for the first. Unit tests for the second (which often contains the edge cases and complex logic).

I remember one article that described an idea as "dependency elimination", which I guess this relates to.
http://qualityisspeed.blogspot.no/2014/09/beyond-solid-dependency-elimination.html

(And since this is the internet, obligatory troll content: "Mocking considered harmful" ;)

How would that look in F#? That question triggered a blog post, but the summary is this:

// string -> string
let getUpperText path =
    path
    |> Some
    |> Option.filter File.Exists
    |> Option.map (File.ReadAllText >> getUpper)
    |> Option.defaultIfNone "DEFAULT"

Apart from getUpper there's nothing else to test 😉