1
\$\begingroup\$

Say I have a piece of code

public class OrderValidator : IValidator<OrderCommand>
{
    private readonly IValidationService validationService;

    public OrderValidator(IValidationService validationService)
    {
        this.validationService = validationService;
    }

    public ValidationResult Validate(OrderCommand command)
    {
         var stepBuilder = this.validationService.CreateValidationStepBuilder(command);

         stepBuilder
             .AddCommandStep<OrderedItemIsValid>()
             .AddBreakIfNotValidStep()
             .AddCommandStep<CustomerIsValid>()
             .AddCommandStep<CustomerAddressIsValid>()
             .AddBreakIfNotValidStep()
             .AddCommandStep<OrderPaymentIsValid>()
             .AddCommandStep<OrderPaymentSuccessful>();                

         return stepBuilder.Validate();
    }
}

The purpose of the validator is to setup the various validation steps (the Add... bit). The actual task of validation is delegated to a service, which creates a step builder, which exposes a validate method. So the validation step builder has abstracted away the job of actually storing and running the steps in order away from the validator.

In my unit test for this class, I have to make sure that the validation steps have been applied in the correct order. Since that is the only responsibility of the validator class.

[Fact]
public void AssertThatStepsAreAddedInOrder()
{
    var validationStepBuilderMock = this.CreateStepBuilderMock();

    // assume step builder mock has been hooked up correctly to the validator under test at this point
    this.Validator.Validate(new OrderCommand());

    // Using NSubstitute, verify that the order of steps is correct
    Received.InOrder(() =>
    {
        validationStepBuilderMock
             .AddCommandStep<OrderedItemIsValid>()
             .AddBreakIfNotValidStep()
             .AddCommandStep<CustomerIsValid>()
             .AddCommandStep<CustomerAddressIsValid>()
             .AddBreakIfNotValidStep()
             .AddCommandStep<OrderPaymentIsValid>()
             .AddCommandStep<OrderPaymentSuccessful>(); 
    });
}

Is this a good unit test? My colleagues have argued no since all I have done is copy and paste the steps into the test (unit test exhibits tautology). If the steps are changed, or a new one is added, then all that needs to be done is to update the test method order.

The code does look extremely similar to the validator under test due to the fluent nature of the step builder (Add... method chaining). But how else would you test for order of execution? Surely the point of a test that tests the order of code execution is to break when that order is violated?

\$\endgroup\$
1
\$\begingroup\$

I think your colleagues are correct in that your test looks like it is testing code written as part of your test. Meaning code that looks similar to the production code, but isn't. But there are things of value here to test.

You mentioned that your builder has tests around it. I would think that these "do things execute in order" questions are best answered by testing your builder in different configurations. the question is simply, "can the builder build things that execute in order".

Secondly, the validation service is a concrete implementation with and interface (abstraction). You'll likely want to test the concrete implementation because it will probably (unless configured dynamically) have a very fixed set of validation rules that are set for your business needs. Those tests become, "is my validation service catching all of the currently considered scenarios".

\$\endgroup\$
  • \$\begingroup\$ Indeed. The validation service which creates the builder already has a bunch of tests around it - Making sure validation steps are added and executed in order. This just leaves the actual validator with setting up the steps. Which I want to test! \$\endgroup\$ – Umair Oct 20 '17 at 15:05
  • \$\begingroup\$ @Umair for what it's worth, unit testing is at it's most effective when you are testing outputs against inputs. testing the plumbing of a function can lead to very brittle tests. The implementation of a function should be allowed to change as long as it continues to produce the correct result. Currently, the function could be producing the correct result, but tests will fail. Just food for thought. \$\endgroup\$ – TBone Oct 20 '17 at 16:54
  • \$\begingroup\$ @Umair I think you've suggested that you have a way to test ordering for some part of it. Can that not be used for the other parts as well? \$\endgroup\$ – TBone Oct 20 '17 at 16:55
  • \$\begingroup\$ I understand. Thanks for the input! What I tried to do was to abstract away the storing and execution of the validation steps. So the validators are only concerned with setting up of the steps. The testing of the ordering of is being done in the tests for the validation service and step builder. I need to find another way of abstracting as much as I can but making the validators still be meaningful testing-wise. \$\endgroup\$ – Umair Oct 20 '17 at 17:12
1
\$\begingroup\$

Is this a good unit test? My colleagues have argued no.

The test that almost precisely repeats the code under test indeed does not provide much value and is tautological.

But how else would you test for order of execution? Surely the point of a test that tests the order of code execution is to break when that order is violated?

I am inclined to disagree. The test that verifies step execution order seem to be targeting the internals ("implementation details") rather than the contract.


Here's the way I think about it: As a consumer of the OrderValidator I will never care about the specific sequence of validation steps. I only care about:

  • the reliability of the ValidationResult (no false positives, nor false negatives);
  • the completeness of the ValidationResult (tell me either as much as you can, or as much as it's relevant for me to learn about the object being validated).

I believe it's better to thoroughly inspect the ValidationResults for various scenarios/validated object kinds, because this is what the consumer of the class will care after all. This is all correct within the context of one important assumption: OrderValidator is always set up to run in a single specific configuration (meaning, there's only one sequence of steps).


If this assumption is wrong, and you'll have various OrderValidators that run steps in different order, than your tests should indeed make sure that the prescribed order is followed.

Your current code is using the mock that intercepts the method invocation. I think, this is still checking the implementation detail.

Instead, I would expose the collection of steps as a read-only/immutable property. Then I could do something like this:

var expectedSteps = new [] {
  "OrderedItemIsValid",
  "CustomerIsValid",
  "CustomerAddressIsValid",
  "OrderPaymentIsValid",
  "OrderPaymentSuccessful"
};
var stepBuilder = orderValidator.CreateStepBuilder(orderCommand);
Assert.True(stepBuilder.StepsNames.SequenceEqual(expectedSteps));

That would require changes similar to those:

public StepBuilder CreateStepBuilder(OrderCommand command)
{
     var stepBuilder = this.validationService.CreateValidationStepBuilder(command);

     stepBuilder
         .AddCommandStep<OrderedItemIsValid>()
         .AddBreakIfNotValidStep()
         .AddCommandStep<CustomerIsValid>()
         .AddCommandStep<CustomerAddressIsValid>()
         .AddBreakIfNotValidStep()
         .AddCommandStep<OrderPaymentIsValid>()
         .AddCommandStep<OrderPaymentSuccessful>();                

     return stepBuilder;
}

var validationResult = orderValidator.CreateStepBuilder(orderCommand).Validate();

Of course, this code is only a sketch to demonstrate the idea. It should be polished. For example, I find it a bit strange that even the original code does return stepBuilder.Validate(); instead of return stepBuilder.Build().Validate();.


This are just my 2 cents. I do not expect other developers necessarily agree with my opinion, that's fine. My objective was to provide yet another perspective and try to message that the best answer is probably still "it depends".

Oh, and by the way, it's really important to get as close to the consensus with your peers as it's possible (both for the team health and for project direction strength). Their critique of the unit tests seems valid to me. But arguments aside, what exactly do the colleagues propose? It's not a secret that if the criticism is not constructive, it usually poisonous...

\$\endgroup\$
  • \$\begingroup\$ A very good answer and suggestions! Thank you! \$\endgroup\$ – Umair Oct 22 '17 at 19:36
0
\$\begingroup\$

An additional approach would be to test from the perspective of the calling code, by providing values and their expected validation results.

Technically this is an integration test not a unit test as it will be testing OrderValidator and ValidationService together. This method will tell you if the implementation is working in line with the requirements, where arguably the existing test does not.

More specific test methods and test case sources could be added, for example ValidOrderedItem, InvalidOrderedItem, ValidCustomer, ValidCustomerInvalidOrderedItem, etc, depending on the granularity you deem appropriate.

public class OrderValidatorTests
{
    private OrderValidator _orderValidator { get; set; }

    [SetUp]
    public void Setup()
    {
        _orderValidator = new OrderValidator(new ValidationService());
    }

    [Test]
    [TestCaseSource(typeof(OrderValidatorTestCaseProvider), "ValidOrderCommands")]
    public void Validate_ValidOrder_ReturnsPositiveValidationResult(OrderCommand orderCommand)
    {
        // Act
        var result = _orderValidator.Validate(orderCommand);

        // Assert
        Assert.IsTrue(result.Success);
    }


    [Test]
    [TestCaseSource(typeof(OrderValidatorTestCaseProvider), "InvalidOrderCommands")]
    public void Validate_InvalidOrder_ReturnsNegativeValidationResult(OrderCommand orderCommand)
    {
        // Act
        var result = _orderValidator.Validate(orderCommand);

        // Assert
        Assert.IsFalse(result.Success);
    }
}

public class OrderValidatorTestCaseProvider
{
    public static IEnumerable<OrderCommand> ValidOrderCommands()
    {
        yield return new OrderCommand() { Data = "Some valid data" };
        yield return new OrderCommand() { Data = "Different valid data" };
        yield return new OrderCommand() { Data = "Another valid data" };
    }

    public static IEnumerable<OrderCommand> InvalidOrderCommands()
    {
        yield return new OrderCommand() { Data = "Some invalid data" };
        yield return new OrderCommand() { Data = "Different invalid data" };
        yield return new OrderCommand() { Data = "Another invalid data" };
    }
}
\$\endgroup\$

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.