CODEWITHSUNDEEP
c#unit-testingmoqxunit
Zizo
Zizo

Reputation: 43

How would I unit test this method?

I am new to unit testing (xUnit) and I am not quite sure how to approach this method when writing tests for it.

Basically I have a class called Label.cs and in the constructor I use DI to inject 2 interfaces.

I also have a Get() method where it:

  1. Constructs a XML request
  2. Removes any invalid characters
  3. Calls the API
  4. Reads the label info from the response
  5. Returns the response

Is it possible to write tests for a method like this? I understand that I can mock the dependencies, pass them into the constructor and setup the methods using Moq but what if I had 4 or 5 more validations methods from ILabelValidation inside the Get() method? Would I need to setup all those methods using Moq?

public class Label 
{
    private readonly ICarrierService _carrierService;
    private readonly ILabelValidation _validator;

    public Label(int accountId, ICarrierService carrierService, ILabelValidation validator)
    {
        _carrierService = carrierService;
        _validator = validator;
    }

    public override async Task<CreateShipmentResponse> Get(int accountId, Shipment shipment)
    {
        string xmlRequest = ConstructRequest(shipment);
        
        // strip out any invalid characters inside the request
        xmlRequest = _validator.InvalidCharacterValidation(xmlRequest);

        // what if I have another method that I use from the ILabelValidation?
        // _validator.ValidatePackageCount(3);

        var stringContent = new StringContent(xmlRequest, Encoding.UTF8, "text/xml");

        // make the API request
        var shipmentResponse = await _carrierService.CreateShipment(dict, stringContent);

        var stream = await shipmentResponse.Content.ReadAsStreamAsync();
        Models.Label labelInfo = GetLabelInfo(stream);

        var response = new CreateShipmentResponse();
        response.labels = new List<string>();

        if (!string.IsNullOrEmpty(labelInfo.Base64String))
            response.labels.Add(labelInfo.Base64String);

        return response;
    }
}

Upvotes: 3

Views: 205

Answers (2)

Zdeněk Jel&#237;nek
Zdeněk Jel&#237;nek

Reputation: 2913

As you have noted yourself, your method does multiple things. This is usually not a good thing. As you find it hard to test, it is a signal that your design needs more work.

In order to test things, it's best if method/component does as few things as possible. Ideally one thing.

Obviously, on the Label level of abstraction, the one thing is Get. However, as you drill down, you find out that you have to 1. Create a request, 2. Send the request, 3. Process the response. Now those are additional three steps that are one level of abstraction below but are separate and should be represented by methods on lower level of abstraction. Now these methods will probably be much easier to test, but may need further decomposition.

With this approach, you may choose to only have a couple of sanity tests for Label::Get and detailed test suites covering all edge cases for <Your new request creation component>::CreateRequest and <Your new response processing component>::ParseResponse. The Label::Get tests would probably need a spy double for the request verification with ICarrierService and the CreateRequest might also use a stub for ILabelValidation. Although this really depends on your final design which may differ from my assumptions. If you're not sure about the doubles and how they match with Moq, you might want to check Martin Fowler's Mocks aren't stubs.

Please also note that doing things like Label::Get may look very OOP and stuff, but if you find yourself having to mock out ICarrierService when implementing unit test for some business logic with Labels that is completey independent from how Labels or CreateShipmentResponse are acquired, your design has some more issues. This gets especially unpleasant if you add one more dependency to Label and suddenly have to rework hundreds of unrelated tests. The idea of putting everything remotely related to one concept into one class leads over time to 5+kLoC legacy monsters that are extremely hard to change and test. But again, I don't know your system in detail so this is just for you to consider. To me, it just seems strange to have a constructor that takes int, ICarrierService and ILabelValidation as those three things probably have completely different lifetimes and abstraction levels.

Upvotes: 2

StefanFFM
StefanFFM

Reputation: 1898

Unless you use MockBehaviour.Loose you have to create a setup for each call. Which makes sense in your scenario as you will need to provide data through the mock ups so you can make meaningful assertions on what the method should return.

With loose behaviour you don't need to provide setups but all method calls on the mocks will return the default value of the methods return type (null) which won't work with your code.

Upvotes: 0

Related Questions