3
\$\begingroup\$

I'm looking for feedback on a way to avoid massive code duplication on unit tests of a generic interface. It is expected that the interface will have several dozen implementations with common constraints.

Consider the following simple builder interface:

public interface IBuilder<in TInput, out TOutput>
{
    TOutput Build(TInput input);
}

Now consider the fact that each implementation of this interface needs proper unit tests written for them, and that I always want to make sure the following rule is never violated:

If the input is null, an ArgumentNullException should be thrown

As a liskov substitution best practice, I don't want to allow implementors to "decide" if they are going to throw or not. They should always throw if it is null. Without code contracts, this is not possible to enforce directly on the interface though of course.

At the same time, I also don't want to restrict the interface to reference types. In other words, this is allowed:

public class ValueTypeBuilder : IBuilder<DateTime, int>
{
    int IBuilder<DateTime, int>.Build(DateTime input)
    {
        return input.Hour;
    }
}

Notice how in this particular case, it is impossible for input to be null, as it is a value type. Throwing an ArgumentNullException would thus not make sense in this particular implementation, and it should not be tested for it.

We can also have other implementations like:

public class ReferenceTypeBuilder : IBuilder<object, int>
{
    int IBuilder<object, int>.Build(object input)
    {
        if (input == null)
            throw new ArgumentNullException(nameof(input)));

        return input.GetHashCode();
    }
}

And a more complex example with interface dependencies:

public class ComplexBuilder : IBuilder<object, int>
{
    private readonly IHashCodeProvider hashCodeProvider;

    public ComplexBuilder(IHashCodeProvider hashCodeProvider)
    {
        this.hashCodeProvider = hashCodeProvider;
    }

    int IBuilder<object, int>.Build(object input)
    {
        if (input == null)
            throw new ArgumentNullException(nameof(input));

        return this.hashCodeProvider.GetHashCode(input);
    }
}

public interface IHashCodeProvider
{
    int GetHashCode<T>(T instance);
}

With the above in mind, this is my proposal to have centralized test logic for the null checking exception throwing rule.


First, I created a generic abstract test class:

public abstract class CustomBuilderTestsBase<TBuilder, TInput, TOutput>
    where TBuilder : IBuilder<TInput, TOutput>
    where TInput : class
{
    [TestMethod]
    public void BuildingFromNullSourceShouldThrowArgumentNullException()
    {
        var builder = CreateBuilder();
        Assert.ThrowsException<ArgumentNullException>(() => builder.Build(default(TInput)));
    }

    protected abstract TBuilder CreateBuilder();
}

This base class enforces the exception rule by having a single general purpose unit test that validates it. Consumers can simply inherit from it for any given builder to have the rule enforced, and can optionally add other unit tests covering the actual logic inside the builder.

This would be the a valid unit test class for the "complex" scenario:

[TestClass]
public sealed class ComplexBuilderTests : CustomBuilderTestsBase<ComplexBuilder, object, int>
{
    private IHashCodeProvider hashCodeProvider;

    [TestInitialize]
    public void Initialize()
    {
        hashCodeProvider = Mock.Of<IHashCodeProvider>();
    }

    [TestMethod]
    public void ShouldReturnCorrectHashCode()
    {
        var input = new object();
        var expectedHashCode = 10;
        Mock.Get(hashCodeProvider)
            .Setup(m => m.GetHashCode(input))
            .Returns(expectedHashCode);

        IBuilder<object, int> builder = CreateBuilder();

        var actualHashCode = builder.Build(input);

        Assert.AreEqual(expectedHashCode, actualHashCode);
    }

    protected override ComplexBuilder CreateBuilder()
    {
        return new ComplexBuilder(hashCodeProvider);
    }
}

Base test + specific test

Then I realized that in a lot of cases, the builder has no dependencies, so it would make sense to provide a class that knew how to create the instances:

public abstract class BuilderTestsBase<TBuilder, TInput, TOutput> : CustomBuilderTestsBase<TBuilder, TInput, TOutput>
    where TBuilder : IBuilder<TInput, TOutput>, new()
    where TInput : class
{
    protected sealed override TBuilder CreateBuilder()
    {
        return new TBuilder();
    }
}

Notice how this one relies on the other abstract class to share the single test, but due to the additional generic constraint, doesn't need to force consumers to override the creation method.

This leads to test classes like this:

[TestClass]
public sealed class ReferenceTypeBuilderTests : BuilderTestsBase<ReferenceTypeBuilder, object, int>
{
}

Now for the questions:

  1. Redundancy in generic arguments

    To restrict the abstract classes only to "reference type inputs", I had to declare the class with 3 generic arguments: one for the builder, and the other 2 to represent the input and output. To the caller, this seems unnecessary, since you are already providing the mapper type. Do you see any way to avoid that and make callers a bit simpler?

    I think I could add a non-generic version of IBuilder as a kind of marker interface to enable a generic constraint like where T : IBuilder, but then there would be no way of verifying that TInput is a reference type anymore.

    .

  2. On multiple abstract classes

    To make the life of consumers a little bit easier, I added the second abstract class. That was the only way I could come up with to have the same constraints with the additional new() one. Unfortunately, they cannot have the same "name" (there is no such thing as "class overload" in C#), which can make it harder to discover. Do you see another option that would result in the same behavior? I'm starting to think it might not be worth it providing this second version, since in cases of a simple constructor, it is straightforward enough to override the method. Still, I was wondering if there would be a way to make this work.

    .

  3. abstract method to create the builder

    My approach was to expose an abstract method so that each inherited class is able to customize the actual creation of the object. This can lead to potentially confusing code as above, where we have a TestInitialize method generating the dependencies, and then these dependencies are used inside the factory method. Do you see any immediate issues on that front, or is it intuitive enough for you? Any other suggestions for a pattern on creating complex builders? I'm not a fan of using the [TestInitialize] myself but in that particular case it made the most sense to me. I guess I could get rid of it and create the mock inside the CreateBuilder method as well in that case?

    .

  4. sealed CreateBuilder override

    I made the overriden CreateBuilder method sealed in the class which creates the instances automatically. I felt like it would not make sense to ever want to override this method on that particular class, and I also tend to favor sealed-by-default on everything I write. Do you agree with that decision, or do you see a case where it would still make sense to override the CreateBuilder method there?

I'd also appreciate if you could share your general thoughts on this idea, as there is a high chance that it could be extended for any kind of generic interface with lots of implementations. Maybe you think this is too clever and you'd replicate the exact same unit test across the few dozen unit test classes?

\$\endgroup\$
  • \$\begingroup\$ At a quick glance, seems sane, but do you have a corresponding abstract base class for value types or will value type implementers simply use the interface? \$\endgroup\$ – RubberDuck Oct 10 '18 at 21:48
  • 1
    \$\begingroup\$ @RubberDuck since the only "contract" constraint is the argument not being null, there would be nothing to check on a base class specific for valuetype inputs, so I didn't create one. The idea with the abstract class is to avoid having to add the exact same unit test to each and every implementation that relied on reference input types (99% of them pretty much). Unit tests for value types would not need to inherit from any base class in that case. \$\endgroup\$ – julealgon Oct 10 '18 at 22:36
1
\$\begingroup\$

I think this solution is way too complex because it requires abstract classes and inheriting from them. This is a lot to do and the simple test case does not justify this effort. Also creating instances of the builder, which is the arrange part could be complex too, so I doubt it's possible to hide this step (unless you really have some standarized procedure for it).

Instead I suggest creating a simple extension for the Assert.That property. You pass it an instance of the builder and it does the rest.

internal static class AssertExtensions
{
    public static void DoesNotAllowNullArgument<TInput, TOutput>(this Assert assert, IBuilder<TInput, TOutput> builder) where TInput : class
    {
        Assert.ThrowsException<ArgumentNullException>(() => builder.Build(default));
    }
}

The arrage part is left to the user but even for this, I would rather create a factory class than using inheritance. What if you need/want to standarize other test cases like the Build method must not return null. You couldn't create another base class because C# does not support multiple inheritance. You'd either need multiple derived test classes or you'd have to extend the first class. Both solutions are IMO an overkill.

This is how you could use that new extension:

[TestClass]
public class MyTestClass
{
    [TestMethod]
    public void MyTestMethod()
    {
        var builder = Mock.Create<IBuilder<object, object>>();
        Mock.Arrange(builder, b => b.Build(Arg.AnyObject)).DoInstead(() => throw new ArgumentNullException());
        Assert.That.DoesNotAllowNullArgument(builder);
    }
}

I used Telerik.JustMock here to implement a fake instace of the IBuilder.


Alternatively you can create a more geneirc test just for the Build method:

public static void DoesNotAllowNullArgument<TInput, TOutput>(this Assert assert, Func<TInput, TOutput> build) where TInput : class
{
    Assert.ThrowsException<ArgumentNullException>(() => build(default));
}

and pass only the Func to the test:

Assert.That.DoesNotAllowNullArgument<object, object>(builder.Build);
\$\endgroup\$
  • \$\begingroup\$ ok, so what's wrong with this answer? \$\endgroup\$ – t3chb0t Oct 11 '18 at 18:32
  • \$\begingroup\$ No idea. I upvoted it as I think it is a valid approach. Having said that, I don't think you are really "solving" the original problem here, which was that one needed to replicate the same test on each and every implementation of IBuilder. Honestly, I don't think the extension method is helping that much in this case as you still have to create the entire method, and the original expression inside Assert.ThowsException was fairly straightforward to begin with. \$\endgroup\$ – julealgon Oct 13 '18 at 13:04
  • \$\begingroup\$ Also, if I needed to have another common behavior tested, like "not returning null" as you mentioned, it would just be a matter of adding a new unit test inside the base class (no need to have another base class or multiple inheritance). Your feedback that my strategy is too complex is good, I appreciate that. \$\endgroup\$ – julealgon Oct 13 '18 at 13:04
  • \$\begingroup\$ Lastly, when a complex arrange is needed, one can override the CreateBuilder method as I did I'm my example of the more complex scenario with dependencies. In that case, I'm even forcing the override as the method is abstract, so I'm not really trying to standardize the arrange as you mentioned, unless I'm missing something. \$\endgroup\$ – julealgon Oct 13 '18 at 13:09
0
\$\begingroup\$

In an attempt to try to understand what you want to achieve, I came up with the below. Don't consider it as a review to make your code better - but just as an attempt to think along the paths of yours and t3chb0ts:

  public abstract class TesterBase
  {
    protected bool IsNullableType(Type type)
    {
      return Nullable.GetUnderlyingType(type) != null;
    }

    public virtual void NullArgumentShouldThrow<TIn, TOut>(Func<TIn, TOut> function)
    {
      if (typeof(TIn).IsClass || IsNullableType(typeof(TIn)))
      {
        Assert.ThrowsException<ArgumentNullException>(() => function(default(TIn)), "Should throw ArgumentNullException");
      }
    }
  }

  public abstract class IBuilderTesterBase<TBuilder, TIn, TOut> : TesterBase where TBuilder : IBuilder<TIn, TOut>
  {
    protected abstract TBuilder Create();

    [TestMethod]
    public void TestBuildArgumentNullException()
    {
      var builder = Create();
      NullArgumentShouldThrow<TIn, TOut>(builder.Build);
    }

    // TODO: Other common IBuilder Tests
  }

  public abstract class SimpleIBuilderTesterBase<TBuilder, TIn, TOut> : IBuilderTesterBase<TBuilder, TIn, TOut> where TBuilder : IBuilder<TIn, TOut>, new()
  {
    protected override TBuilder Create()
    {
      return new TBuilder();
    }
  }

  public abstract class ComplexIBuilderTesterBase<TBuilder, TIn, TOut> : IBuilderTesterBase<TBuilder, TIn, TOut> where TBuilder : IBuilder<TIn, TOut>
  {
    Func<TBuilder> m_creator;

    public ComplexIBuilderTesterBase(Func<TBuilder> creator)
    {
      m_creator = creator;
    }

    protected override TBuilder Create()
    {
      return m_creator();
    }
  }

  [TestClass]
  public class ObjectBuilderTester : SimpleIBuilderTesterBase<ReferenceTypeBuilder, object, int>
  {
    // TODO: Other ObjectBuilder tests
  }

  [TestClass]
  public class DateTimeBuilderTester : SimpleIBuilderTesterBase<ValueTypeBuilder, DateTime, int>
  {
    // TODO: Other DateTimeBuilder tests
  }

  [TestClass]
  public class ComplexBuilderTester : ComplexIBuilderTesterBase<ComplexBuilder, object, int>
  {
    public ComplexBuilderTester() : base(DoCreate)
    {

    }

    private static ComplexBuilder DoCreate()
    {
      return new ComplexBuilder(null);
    }

    // TODO: Other ComplexBuilder tests
  }

I admit that it maybe doesn't add any clarity to the picture, but I think it gets the job done.


Some explanation and thoughts about the above in order to answer the questions julealgon asks in the comment:

1) The idea of the TesterBase.NullArgumentShouldThrow(...) is, that it can be used by other tests. In fact the idea of TesterBase is that it should server as the owner of reusable "low level"/general purpose tests, that can be called by sub classes. It could have been implemented as a static helper class as well.

2) I think it's OK, that a test of null argument on a value type is bypassed without notice, because the test is irrelevant for those types. But you may have an other opinion about that.

3) In general IMO you should not be concerned about the depth of a class hierarchy as long as each level (subclass) serves a well defined purpose and is logical.

But again my answer is not postulating to be better than the approach of OP or even good. I were just trying to dive in to the field of the problem and understand it.

\$\endgroup\$
  • \$\begingroup\$ So, while I understand what you did and can see it indeed works fine, I have a few questions: 1. Why passing a Func to the base class is better than a direct override to you? They seem to be achieving the same end result, but your approach seems more complex IMHO. 2. By having just a single base class with a runtime type check, you'll end up outputting a "passed" test for 'null' checks on a value type. Don't you think that's misleading? 3. Your approach has a deeper hierarchy chain than mine. I try to limit inheritance as much as possible. Any comments in that regard? \$\endgroup\$ – julealgon Oct 13 '18 at 12:52

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.