2

I have a similar scenario to this:

public class VatCalculator
{
    public decimal GetValueWithVat(decimal value, decimal? vatRatePercentage)
    {
        if(vatRate == null)
        {
            return value;
        }

        return value * (1 + (vatRate / 100));
    }
}


public ReceiptGenerator
{
    private readonly VatCalculator _vatCalculator;

    public ReceiptGenerator(VatCalculator vatCalculator)
    {
        _vatCalculator = vatCalculator;
    }

    public Receipt GenerateReceipt(Item purchasedItem, decimal? vatRate)
    {
        var receipt = new Receipt();

        receipt.PriceBeforeVat = purchasedItem.Price
        receipt.PriceAfterVat = _vatCalculator.GetValueWithVat(purchasedItem.Price, vatRate)
    }
}

In this case, I have a few unit tests I'd like to write, e.g.

  • If no VAT rate is provided, the 'Price After Vat' value should be equal to the purchased item's price.
  • If a VAT rate is provided, the 'Price After Vat' value should be adjusted accordingly

My question is whether I should write these unit tests at the level of the ReceiptGenerator class or the VatCalculator class.

If I test at the ReceiptGenerator class level it means I have more durable tests; The code of GenerateReceipt can be refactored e.g. to use a private method or an API and the tests should still pass with no changes. But this seems like too large of a scope for a unit test, my understanding was that it should be a very granular focused test

If I test at the VatCalculator level I can use that class in other consumers (for example, if I move my business online, I can use it to provide on-the-go prices for customers browsing a product) without having to test the consumers themselves.

3

You should test at both the VatCalculator and at the ReceiptGenerator level.

In the unit-tests for the VatCalculator class, you verify that the right adjustment to the given price is made based on the given VAT rate.

In the unit-tests for ReceiptGenerator, you verify that the receipt shows the right amounts, no matter how silly the result given by your VAT calculation class. If the VAT calculator you use in the test says that the price becomes negative after adding VAT, then the receipt better show that. This proves that ReceiptGenerator actually uses the provided VAT calculator and does not try to do its own calculation.
These tests should not use the VatCalculator class, but rather a test version of that class where you can return silly results as well as plausible results.

Finally, you would have an integration (or even higher-level) test to prove that the ReportGenerator and VatCalculator classes work correctly together.

  • Are you suggesting I abstract the vatCalculator parameter of ReceiptGenerator and use dependency inversion to pass in a fake one during testing, which I can use to output custom values, then assert that those values are used for PriceAfterVat? – FLSH Sep 20 '16 at 11:16
  • 1
    @FLSH: Yes. And as you are already injecting the VatCalculator dependency into ReceiptGenerator, the change to your code will not be that large. – Bart van Ingen Schenau Sep 20 '16 at 11:19
  • 1
    But that would make the tests for ReceiptGenerator somewhat more brittle. If it gets refactored and no longer uses VatCalculator all the tests would have to be changed. – FLSH Sep 20 '16 at 11:22
  • "re you suggesting I abstract the vatCalculator parameter of ReceiptGenerator and use dependency inversion to pass in a fake one during testing". No, please never do this! This is terrible advice as you'll end up testing your mock, not the real code. – David Arno Sep 20 '16 at 11:49
  • @FLSH: If you change the interface of ReceiptGenerator or what that class depends on to do its work, then it is to be expected that the relevant test cases need to be refactored as well. What do you think would happen to the tests of ReceiptGenerator if VatCalculator gets changed to take the shipping address into account? It is a sign of brittle test cases if a change to class A causes the unit-tests for class B to start failing. – Bart van Ingen Schenau Sep 20 '16 at 12:17
0

There tends to be a lot of confusion around "unit tests". The easiest way to think of them is an a unit of work, ie a self-contained piece of work. By "self-contained", I mean that unit tests should be able to run in parallel, with no side effects, and thus not affect each other.

In your case, VatCalculator.GetValueWithVat is a pure function, so there is no benefit in mocking it, or testing it in isolation. It can be safely tested as part of other unit tests. Being a pure function in a class that doesn't implement an interface, you could make it static and you could improve ReceiptGenerator by injecting it directly via a Func<decimal, decimal?, decimal> in the ReceiptGenerator's constructor.

So test both GetValueWithVat and ReceiptGenerator in your unit tests.

The only other point to note about your code is the var receipt = new Receipt();. This is a tight coupling weak point in your code that could cause problems in future. You'd be better off injecting an instance of IReceiptFactory into ReceiptGenerator and creating instances of Receipt via that factory.

  • Suppose I create another class which also consumes VatCalculator in a similar way, and I also want to write unit tests for this class to confirm VAT is applied correctly. I would surely end up writing the exact same tests. Is this a bad thing? – FLSH Sep 20 '16 at 12:38
0

The point of a unit-test is to verify that each unit concerned functions according to its design (and implicitly therefore, to its specification).

As such, appropriate test cases are required for each of:

  • VatCalculator::GetValueWithVat()
  • ReceiptGenerator::ReceiptGenerator()
  • ReceiptGenerator::GenerateReceipt()

If you do not prove VatCalculator::GetValueWithVat() stand-alone, then you cannot assume it is correct when using it as part of the testing of ReceiptGenerator::GenerateReceipt()

It is accepted that some of these test cases will be trivial, but even trivial tests can fail!

-1

Both.

Make sure you test each part individually; they're unit tests, after all. This will allow you to check the logic on each part is correct. It will also allow you to swap implementations and make sure each part still works as expected.

Having the ReceiptGenerator check whether the VatCalculator does its jobs is not required, since you have tested the VatCalculator on its own. But you want to make sure the ReceiptGenerator correctly adds and removes products from the receipt, calculates the correct change, etc.

  • So what tests would you recommend for ReceiptGenerator related to VatCalculator? – FLSH Sep 20 '16 at 11:17
  • @FLSH - you would verify that a call to ReceiptGenerator sets _vatCalculator appropriately. – Andrew Sep 20 '16 at 14:25

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.