1

I'd like to unit test the following class.

How can I mock the HttpClient when it's used as a static readonly field in a static class?

This question doesn't help as the OP is using an instance field for the HttpClient.

Here's my class:

using Newtonsoft.Json;
using System;
using System.Collections.Generic;
using System.Net.Http;
using System.Text;
using System.Threading.Tasks;

namespace Integration.IdentityProvider
{
    public static class IdentityProviderApiCaller
    {
        private static readonly HttpClient HttpClient;

        static IdentityProviderApiCaller()
        {
            HttpClient = HttpClientFactory.Create();
            HttpClient.BaseAddress = new Uri("https://someApi.com");
            HttpClient.DefaultRequestHeaders.Add("Accept", "application/json");
        }

        public static async Task<IList<AddGroupResult>> AddGroups(AddGroup[] addGroupsModel)
        {
            var content = GetContent(addGroupsModel);
            var urlPath = "/groups";

            var result = await HttpClient.PutAsync(urlPath, content).ConfigureAwait(false);
            return await GetObject<IList<AddGroupResult>>(result).ConfigureAwait(false);
        }

        private static async Task<T> GetObject<T>(HttpResponseMessage result) where T : class
        {
            if (result.IsSuccessStatusCode)
            {
                return await DeserializeObject<T>(result.Content).ConfigureAwait(false);
            }
            var errorResult = await DeserializeObject<ErrorResult>(result.Content).ConfigureAwait(false);
            throw new Exception(errorResult.ExceptionMessage);
        }

        private static async Task<T> DeserializeObject<T>(HttpContent content) where T : class
        {
            var jsonContent = await content.ReadAsStringAsync().ConfigureAwait(false);
            T obj;
            try
            {
                obj = JsonConvert.DeserializeObject<T>(jsonContent);
            }
            catch (JsonSerializationException)
            {
                return await Task.FromResult<T>(null).ConfigureAwait(false);
            }
            return obj;
        }

        private static StringContent GetContent<T>(T obj)
        {
            var payload = JsonConvert.SerializeObject(obj);
            var content = new StringContent(payload, Encoding.UTF8, "application/json");
            return content;
        }
    }

    public class AddGroup
    {
        public string Name { get; set; }

        public string Description { get; set; }
    }

    public class AddGroupResult
    {
        public bool IsSuccessful { get; set; }
    }

    public class ErrorResult
    {
        public string ExceptionMessage { get; set; }
    }
}
David Klempfner
  • 8,700
  • 20
  • 73
  • 153
  • You may want to post this on code review - there are several issues IMHO, which when addressed, will allow you to test things. There is no separation of concern, verbs are used as a class (maybe better as a method), and I'd remove statics from here as behaviour feels to belong to an instance rather than a type. – oleksii Jun 05 '23 at 15:23
  • @oleksii The OP took your suggested and posted this question with no modifications on Code Review. We do not answer how to questions there. If you would like to perform a code review on the code [here is the question](https://codereview.stackexchange.com/questions/285359/how-would-you-make-this-unit-testable). Right now the question is pretty much off-topic on code review at least until the title changes. – pacmaninbw Jun 05 '23 at 23:55
  • 1
    Hey David, sorry this didn't work out, I would have hoped people will be able to suggest changes. Let me try making a few suggestions here, and see if any of those will be helpful. – oleksii Jun 06 '23 at 15:39

2 Answers2

2

In order to test things, I will suggest a few changes.

  1. Remove all static keyword from your code. I understand you would like to have one thing that does the job, but this can be achieved by having only one instance of the class. This also means it is easier to test your class - create a new instance for a test. You could restrict how many instances is created during the configuration/setup stage in aspnet core (if you are using that).

  2. Change the constructor from static to instance and pass the client as a dependency to it. The new ctor will look something like this:

private readonly HttpClient _client;

public IdentityProviderApiCaller(HttpClient client)
{
    if (client == null) throw new ArgumentNullException(nameof(client));
    _client = client
}

The key point here, is that you provide the dependency of IdentityProviderApiCaller in a ctor, so you can unit test it. In a unit test you provide a mock for the HTTP client, so you can set expectation for get or post and see if the method is being called correctly. In an integration test you can pass a real instance of HTTP client, so you can actually hit your back end services.

  1. Clarify arguments to the function, consider passing a simple list/array. If you rename the AddGroup class to a Group, then the code gets easier to read. Imagine you can also have APIs to Delete(Group group) or list groups. Having a name AddGroup for the group will be confusing. Also, you can simplify the the async. So all together the code should look something like:
public async Task<HttpResponseMessage> AddGroups(List<Group> groups)
{
    if (groups == null) throw new ArgumentNullException(nameof(groups));
    var content = GetContent(addGroupsModel);
    var urlPath = "/groups";
    return await _client.PutAsync(urlPath, content);
}
  1. Throw a more focused exception, the class Exception is very broad. Consider common exceptions ArgumentException, InvalidOperationException, etc. You could create your own exception too, but maybe best check what built-in exceptions are available
throw new Exception(errorResult.ExceptionMessage); // so this line can become a more focused exception

There may be a class, specifically for aspnet core, where you can return a failure code, it may look something like:

return BadRequest(errorResult.ExceptionMessage);

The idea is that you can specify which error code is returned to the client of your API, such as 401, 400. If an exception is thrown, I think it will be status code 500 Internal Server error, which may not be ideal for the client of the API


Now, back to the unit testing, in meta-code, this is how it will look:

[TestFixture]
public class IdentityProviderApiCallerTest
{
    private readonly IdentityProviderApiCaller _uut; // unit under test

    [Test]
    public void AddGroups()
    {
        var mock = Mock<HttpClient>.Create(); // syntax depends on the mock lib
        mock.Expect(x => x.Put(...)); // configure expectations for calling HTTP PUT
        _uut = new IdentityProviderApiCaller(mock) // this is how you pass the dependency
        var group = new Group();
        var result = _uut.AddGroups(group);
        assert.True(result.IsSuccessful)
    }
}
oleksii
  • 35,458
  • 16
  • 93
  • 163
2

Oleksii's advice is on point: inject the HttpClient in order to make it unit testable. But there are a few more things to say on the subject of HttpClient.

Set up your dependency injection to use IHttpClientFactory to give you HttpClients, to avoid socket contention and stale DNS issues.

HttpClient itself is a thin and straightforward wrapper around HttpMessageHandler. Instead of mocking HttpClient, you'll want to mock HttpMessageHandler, and pass it into an actual HttpClient instance. See https://stackoverflow.com/a/36427274/120955

StriplingWarrior
  • 151,543
  • 27
  • 246
  • 315