An simple way to mock an httpClient.GetAsync(..) method for unit tests? #1624

Closed
PureKrome opened this Issue May 4, 2015 · 156 comments

Comments

Projects
None yet
@PureKrome

System.Net.Http has now been uploaded to the repo 馃槃 馃帀 馃巿

Whenever I've used this in some service, it works great but makes it hard to unit test => my unit tests don't want to actually ever hit that real end point.

Ages ago, I asked @davidfowl what should we do? I hoping I paraphrase and don't misquote him - but he suggested that I need to fake up a message handler (ie. HttpClientHandler), wire that up, etc.

As such, I ended up making a helper library called HttpClient.Helpers to help me run my unit tests.

So this works ... but it feels very messy and .. complicated. I'm sure I'm not the first person that needs to make sure my unit tests don't do a real call to an external service.

Is there an easier way? Like .. can't we just have an IHttpClient interface and we can inject that into our service?

@sharwell

This comment has been minimized.

Show comment
Hide comment
@sharwell

sharwell May 4, 2015

Member

HttpClient is nothing more than an abstraction layer over another HTTP library. One of its primary purposes is allowing you to replace the behavior (implementation), including but not limited to the ability to create deterministic unit tests.

In other works, HttpClient itself serves as the both the "real" and the "mock" object, and the HttpMessageHandler is what you select to meet the needs of your code.

Member

sharwell commented May 4, 2015

HttpClient is nothing more than an abstraction layer over another HTTP library. One of its primary purposes is allowing you to replace the behavior (implementation), including but not limited to the ability to create deterministic unit tests.

In other works, HttpClient itself serves as the both the "real" and the "mock" object, and the HttpMessageHandler is what you select to meet the needs of your code.

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome May 4, 2015

But if feels like sooo much work is required to setup the message handler when (it feels like) this could be handled with a nice interface? Am I totally misunderstanding the solution?

But if feels like sooo much work is required to setup the message handler when (it feels like) this could be handled with a nice interface? Am I totally misunderstanding the solution?

@SidharthNabar

This comment has been minimized.

Show comment
Hide comment
@SidharthNabar

SidharthNabar May 5, 2015

@PureKrome - thanks for bringing this up for discussion. Can you please elaborate on what you mean by "so much work is required to setup the message handler"?

One way to unit test HttpClient without hitting the network is -

  1. Create a new Handler class (e.g. FooHandler) that derives from HttpMessageHandler
  2. Implement the SendAsync method as per your requirements - don't hit the network/log the request-response/etc.
  3. Pass an instance of this FooHandler into the constructor of the HttpClient:
    var handler = new FooHandler();
    var client = new HttpClient(handler);

Your HttpClient object will then use your Handler instead of the inbuilt HttpClientHandler.

Thanks,
Sid

@PureKrome - thanks for bringing this up for discussion. Can you please elaborate on what you mean by "so much work is required to setup the message handler"?

One way to unit test HttpClient without hitting the network is -

  1. Create a new Handler class (e.g. FooHandler) that derives from HttpMessageHandler
  2. Implement the SendAsync method as per your requirements - don't hit the network/log the request-response/etc.
  3. Pass an instance of this FooHandler into the constructor of the HttpClient:
    var handler = new FooHandler();
    var client = new HttpClient(handler);

Your HttpClient object will then use your Handler instead of the inbuilt HttpClientHandler.

Thanks,
Sid

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome May 5, 2015

Hi @SidharthNabar - Thank you for reading my issue, I really appreciate it also.

Create a new Handler class

You just answered my question :)

That's a large dance to wiggle too, just to ask my real code to not hit the network.

I even made a the HttpClient.Helpers repo and nuget package .. just to make testing easier! Scenario's like the Happy path or an exception thrown by the network endpoint ...

That's the problem -> can we not do all of this and ... just a mock a method instead?

I'll try and explain via code..

Goal: Download something from the interwebs.

public async Foo GetSomethingFromTheInternetAsync()
{
    ....
    using (var httpClient = new HttpClient())
    {
        html = await httpClient.GetStringAsync("http://www.google.com.au");
    }
    ....
}

Lets look at two examples:

Given an interface (if it existed):

public interface IHttpClient
{
    Task<string> GetStringAsync(string requestUri);    
}

My code can now look like this...

public class SomeService(IHttpClient httpClient = null)
{
    public async Foo GetSomethingFromTheInternetAsync()
    {
        ....
        using (var httpClient = _httpClient ?? new HttpClient()) // <-- CHANGE #1
        {
            html = await httpClient.GetStringAsync("http://www.google.com.au");
        }
        ....
    } 
}

and the test class

public async Task GivenAValidEndPoint_GetSomethingFromTheInternetAsync_ReturnsSomeData()
{
    // Create the Mock : FakeItEasy > Moq.
    var httpClient = A.Fake<IHttpClient>();

    // Define what the mock returns.
    A.CallTo(()=>httpClient.GetStringAsync("http://www.google.com.au")).Returns("some html here");

    // Inject the mock.
    var service = new SomeService(httpClient);
    ...
}

Yay! done.

Ok, now with the current way...

  1. create a new Handler class - yes, a class!
  2. Inject the handler into the service
public class SomeService(IHttpClient httpClient = null)
{
    public async Foo GetSomethingFromTheInternetAsync()
    {
        ....
        using (var httpClient = _handler == null 
                                    ? new HttpClient()
                                    : new HttpClient(handler))
        {
            html = await httpClient.GetStringAsync("http://www.google.com.au");
        }
        ....
    } 
}

But the pain is now that I have to make a class. You've now hurt me.

public class FakeHttpMessageHandler : HttpClientHandler
{
 ...
}

And this class starts out pretty simple. Until I when I have multiple GetAsync calls (so i need to provide multiple Handler instances?) -or- multiple httpClients in a single service. Also, do we Dispose of the handler or reuse it if we're doing multiple calls in the same block of logic (which is a ctor option)?

eg.

public async Foo GetSomethingFromTheInternetAsync()
{
    string[] results;
    using (var httpClient = new HttpClient())
    {
        var task1 = httpClient.GetStringAsync("http://www.google.com.au");
        var task2 = httpClient.GetStringAsync("http://www.microsoft.com.au");

        results = Task.WhenAll(task1, task2).Result;
    }
    ....
}

this can be made so much easier with this..

var httpClient = A.Fake<IHttpClient>();
A.CallTo(() = >httpClient.GetStringAsync("http://www.google.com.au"))
    .Returns("gooz was here");
A.CallTo(() = >httpClient.GetStringAsync("http://www.microsoft.com.au"))
    .Returns("ms was here");

clean clean clean :)

Then - there is the next bit : Discoverability

When I first started using MS.Net.Http.HttpClient the api was pretty obvious 馃憤 Ok - get string and do it asyncly.. simple ...

... but then I hit testing .... and now I'm suppose to learn about HttpClientHandlers? Um why? I feel that this should all be hidden under the covers and I shouldn't have to worry about all this implementation detail. It's too much! You're saying that I should start to look inside the box and learn some of the plumbing ... which hurts 馃槩

It's all making this more complex than it should be, IMO.


Help me Microsoft - you're my only hope.

Hi @SidharthNabar - Thank you for reading my issue, I really appreciate it also.

Create a new Handler class

You just answered my question :)

That's a large dance to wiggle too, just to ask my real code to not hit the network.

I even made a the HttpClient.Helpers repo and nuget package .. just to make testing easier! Scenario's like the Happy path or an exception thrown by the network endpoint ...

That's the problem -> can we not do all of this and ... just a mock a method instead?

I'll try and explain via code..

Goal: Download something from the interwebs.

public async Foo GetSomethingFromTheInternetAsync()
{
    ....
    using (var httpClient = new HttpClient())
    {
        html = await httpClient.GetStringAsync("http://www.google.com.au");
    }
    ....
}

Lets look at two examples:

Given an interface (if it existed):

public interface IHttpClient
{
    Task<string> GetStringAsync(string requestUri);    
}

My code can now look like this...

public class SomeService(IHttpClient httpClient = null)
{
    public async Foo GetSomethingFromTheInternetAsync()
    {
        ....
        using (var httpClient = _httpClient ?? new HttpClient()) // <-- CHANGE #1
        {
            html = await httpClient.GetStringAsync("http://www.google.com.au");
        }
        ....
    } 
}

and the test class

public async Task GivenAValidEndPoint_GetSomethingFromTheInternetAsync_ReturnsSomeData()
{
    // Create the Mock : FakeItEasy > Moq.
    var httpClient = A.Fake<IHttpClient>();

    // Define what the mock returns.
    A.CallTo(()=>httpClient.GetStringAsync("http://www.google.com.au")).Returns("some html here");

    // Inject the mock.
    var service = new SomeService(httpClient);
    ...
}

Yay! done.

Ok, now with the current way...

  1. create a new Handler class - yes, a class!
  2. Inject the handler into the service
public class SomeService(IHttpClient httpClient = null)
{
    public async Foo GetSomethingFromTheInternetAsync()
    {
        ....
        using (var httpClient = _handler == null 
                                    ? new HttpClient()
                                    : new HttpClient(handler))
        {
            html = await httpClient.GetStringAsync("http://www.google.com.au");
        }
        ....
    } 
}

But the pain is now that I have to make a class. You've now hurt me.

public class FakeHttpMessageHandler : HttpClientHandler
{
 ...
}

And this class starts out pretty simple. Until I when I have multiple GetAsync calls (so i need to provide multiple Handler instances?) -or- multiple httpClients in a single service. Also, do we Dispose of the handler or reuse it if we're doing multiple calls in the same block of logic (which is a ctor option)?

eg.

public async Foo GetSomethingFromTheInternetAsync()
{
    string[] results;
    using (var httpClient = new HttpClient())
    {
        var task1 = httpClient.GetStringAsync("http://www.google.com.au");
        var task2 = httpClient.GetStringAsync("http://www.microsoft.com.au");

        results = Task.WhenAll(task1, task2).Result;
    }
    ....
}

this can be made so much easier with this..

var httpClient = A.Fake<IHttpClient>();
A.CallTo(() = >httpClient.GetStringAsync("http://www.google.com.au"))
    .Returns("gooz was here");
A.CallTo(() = >httpClient.GetStringAsync("http://www.microsoft.com.au"))
    .Returns("ms was here");

clean clean clean :)

Then - there is the next bit : Discoverability

When I first started using MS.Net.Http.HttpClient the api was pretty obvious 馃憤 Ok - get string and do it asyncly.. simple ...

... but then I hit testing .... and now I'm suppose to learn about HttpClientHandlers? Um why? I feel that this should all be hidden under the covers and I shouldn't have to worry about all this implementation detail. It's too much! You're saying that I should start to look inside the box and learn some of the plumbing ... which hurts 馃槩

It's all making this more complex than it should be, IMO.


Help me Microsoft - you're my only hope.

@aarondandy

This comment has been minimized.

Show comment
Hide comment
@aarondandy

aarondandy May 6, 2015

I too would love to see an easy and simple way to test various things that use HttpClient. 馃憤

I too would love to see an easy and simple way to test various things that use HttpClient. 馃憤

@SidharthNabar

This comment has been minimized.

Show comment
Hide comment
@SidharthNabar

SidharthNabar May 8, 2015

Thanks for the detailed response and code snippets - that really helps.

First, I notice that you are creating new instances of HttpClient for sending each request - that is not the intended design pattern for HttpClient. Creating one HttpClient instance and reusing it for all your requests helps optimize connection pooling and memory management. Please consider reusing a single instance of HttpClient. Once you do this, you can insert the fake handler into just one instance of HttpClient and you're done.

Second - you are right. The API design of HttpClient does not lend itself to a pure interface-based mechanism for testing. We are considering adding a static method/factory pattern in the future that will allow you to alter the behavior of all HttpClient instances created from that factory or after that method. We haven't yet settled on a design for this yet, though. But the key issue will remain - you will need to define a fake Handler and insert it below the HttpClient object.

@ericstj - thoughts on this?

Thanks,
Sid.

Thanks for the detailed response and code snippets - that really helps.

First, I notice that you are creating new instances of HttpClient for sending each request - that is not the intended design pattern for HttpClient. Creating one HttpClient instance and reusing it for all your requests helps optimize connection pooling and memory management. Please consider reusing a single instance of HttpClient. Once you do this, you can insert the fake handler into just one instance of HttpClient and you're done.

Second - you are right. The API design of HttpClient does not lend itself to a pure interface-based mechanism for testing. We are considering adding a static method/factory pattern in the future that will allow you to alter the behavior of all HttpClient instances created from that factory or after that method. We haven't yet settled on a design for this yet, though. But the key issue will remain - you will need to define a fake Handler and insert it below the HttpClient object.

@ericstj - thoughts on this?

Thanks,
Sid.

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome May 8, 2015

@SidharthNabar why are you/team so hesitant to offering an interface for this class? I was hoping that the whole point of a developer having to learn about handlers and then having to create fake handler classes is enough of a pain point to justify (or at the very least, highlight) the need for an interface?

@SidharthNabar why are you/team so hesitant to offering an interface for this class? I was hoping that the whole point of a developer having to learn about handlers and then having to create fake handler classes is enough of a pain point to justify (or at the very least, highlight) the need for an interface?

@luisrudge

This comment has been minimized.

Show comment
Hide comment
@luisrudge

luisrudge May 8, 2015

Yeah, I don't see why an Interface would be a bad thing. It would make testing HttpClient so much easier.

Yeah, I don't see why an Interface would be a bad thing. It would make testing HttpClient so much easier.

@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj May 8, 2015

Member

One of the main reasons we avoid interfaces in the framework is that they don't version well. Folks can always create their own if they have a need and that won't get broken when the next version of the framework needs to add a new member. @KrzysztofCwalina or @terrajobst might be able to share more of the history/detail of this design guideline. This particular issue is a near religious debate: I'm too pragmatic to take part in that.

HttpClient has lots of options for unit testability. It's not sealed and most of its members are virtual. It has a base class that can be used to simplify the abstraction as well as a carefully designed extension point in HttpMessageHandler as @sharwell points out. IMO it's a quite well designed API, thanks to @HenrikFrystykNielsen.

Member

ericstj commented May 8, 2015

One of the main reasons we avoid interfaces in the framework is that they don't version well. Folks can always create their own if they have a need and that won't get broken when the next version of the framework needs to add a new member. @KrzysztofCwalina or @terrajobst might be able to share more of the history/detail of this design guideline. This particular issue is a near religious debate: I'm too pragmatic to take part in that.

HttpClient has lots of options for unit testability. It's not sealed and most of its members are virtual. It has a base class that can be used to simplify the abstraction as well as a carefully designed extension point in HttpMessageHandler as @sharwell points out. IMO it's a quite well designed API, thanks to @HenrikFrystykNielsen.

@TerribleDev

This comment has been minimized.

Show comment
Hide comment
@TerribleDev

TerribleDev May 8, 2015

馃憤 an interface

馃憤 an interface

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome May 8, 2015

馃憢 @ericstj Thanks heaps for jumping in 馃憤 馃嵃

One of the main reasons we avoid interfaces in the framework is that they don't version well.

Yep - great point.

This particular issue is a near religious debate

yeah ... point well taken on that.

HttpClient has lots of options for unit testability

Oh? I'm struggling on this 馃槉 hence the reason for this issue 馃槉

It's not sealed and most of its members are virtual.

Members are virtual? Oh drats, I totally missed that! If they are virtual, then mocking frameworks can mock those members out 馃憤 and we don't need an interface!

Lets have a looksies at HttpClient.cs ...

public Task<HttpResponseMessage> GetAsync(string requestUri) { .. }
public Task<HttpResponseMessage> GetAsync(Uri requestUri) { .. }

hmm. those aren't virtual ... lets search the file ... er .. no virtual keyword found. So do you mean other members to other related classes are virtual? If so .. then we're back at the issue I'm raising - we have to now look under the hood to see what GetAsync is doing so we then know what to create/wireup/etc...

I guess I'm just not understanding something really basic, here 炉(掳_掳)/炉 ?

EDIT: maybe those methods can be virtual? I can PR!

馃憢 @ericstj Thanks heaps for jumping in 馃憤 馃嵃

One of the main reasons we avoid interfaces in the framework is that they don't version well.

Yep - great point.

This particular issue is a near religious debate

yeah ... point well taken on that.

HttpClient has lots of options for unit testability

Oh? I'm struggling on this 馃槉 hence the reason for this issue 馃槉

It's not sealed and most of its members are virtual.

Members are virtual? Oh drats, I totally missed that! If they are virtual, then mocking frameworks can mock those members out 馃憤 and we don't need an interface!

Lets have a looksies at HttpClient.cs ...

public Task<HttpResponseMessage> GetAsync(string requestUri) { .. }
public Task<HttpResponseMessage> GetAsync(Uri requestUri) { .. }

hmm. those aren't virtual ... lets search the file ... er .. no virtual keyword found. So do you mean other members to other related classes are virtual? If so .. then we're back at the issue I'm raising - we have to now look under the hood to see what GetAsync is doing so we then know what to create/wireup/etc...

I guess I'm just not understanding something really basic, here 炉(掳_掳)/炉 ?

EDIT: maybe those methods can be virtual? I can PR!

@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj May 8, 2015

Member

SendAsync is virtual and almost every other API layers on top of that. 'Most' was incorrect, my memory serves me wrong there. My impression was that most were effectively virtual since they build on top of a virtual member. Usually we don't make things virtual if they are cascading overloads. There is a more specific overload of SendAsync that is not virtual, that one could be fixed.

Member

ericstj commented May 8, 2015

SendAsync is virtual and almost every other API layers on top of that. 'Most' was incorrect, my memory serves me wrong there. My impression was that most were effectively virtual since they build on top of a virtual member. Usually we don't make things virtual if they are cascading overloads. There is a more specific overload of SendAsync that is not virtual, that one could be fixed.

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome May 8, 2015

Ah! Gotcha 馃槉 So all those methods end up calling SendAsync .. which does all the heavy lifting. So this still means we have a discoverability issue here .. but lets put that aside (that's opinionated).

How would we mock SendAsync give this basic sample...
Install-Package Microsoft.Net.Http
Install-Package xUnit

public class SomeService
{
    public async Task<string> GetSomeData(string url)
    {
        string result;
        using (var httpClient = new HttpClient())
        {
            result = await httpClient.GetStringAsync(url);
        }

        return result;
    }
}

public class Facts
{
    [Fact]
    public async Task GivenAValidUrl_GetSomeData_ReturnsSomeData()
    {
        // Arrange.
        var service = new SomeService();

        // Act.
        var result = await service.GetSomeData("http://www.google.com");

        // Assert.
        Assert.True(result.Length > 0);
    }
}

Ah! Gotcha 馃槉 So all those methods end up calling SendAsync .. which does all the heavy lifting. So this still means we have a discoverability issue here .. but lets put that aside (that's opinionated).

How would we mock SendAsync give this basic sample...
Install-Package Microsoft.Net.Http
Install-Package xUnit

public class SomeService
{
    public async Task<string> GetSomeData(string url)
    {
        string result;
        using (var httpClient = new HttpClient())
        {
            result = await httpClient.GetStringAsync(url);
        }

        return result;
    }
}

public class Facts
{
    [Fact]
    public async Task GivenAValidUrl_GetSomeData_ReturnsSomeData()
    {
        // Arrange.
        var service = new SomeService();

        // Act.
        var result = await service.GetSomeData("http://www.google.com");

        // Assert.
        Assert.True(result.Length > 0);
    }
}
@MrJul

This comment has been minimized.

Show comment
Hide comment
@MrJul

MrJul May 8, 2015

One of the main reasons we avoid interfaces in the framework is that they don't version well. Folks can always create their own if they have a need and that won't get broken when the next version of the framework needs to add a new member.

I can totally understand this way of thinking with the full .NET Framework.

However, .NET Core is split into many small packages, each versioned independently. Use semantic versioning, bump the major version when there's a breaking change, and you're done. The only people affected will be the ones explicitly updating their packages to the new major version - knowing there are documented breaking changes.

I'm not advocating that you should break every interface every day: breaking changes should ideally be batched together into a new major version.

I find it sad to cripple APIs for future compatibility reasons. Wasn't one of the goal of .NET Core to iterate faster since you don't have to worry anymore about a subtle .NET update breaking thousands of already installed applications?

My two cents.

MrJul commented May 8, 2015

One of the main reasons we avoid interfaces in the framework is that they don't version well. Folks can always create their own if they have a need and that won't get broken when the next version of the framework needs to add a new member.

I can totally understand this way of thinking with the full .NET Framework.

However, .NET Core is split into many small packages, each versioned independently. Use semantic versioning, bump the major version when there's a breaking change, and you're done. The only people affected will be the ones explicitly updating their packages to the new major version - knowing there are documented breaking changes.

I'm not advocating that you should break every interface every day: breaking changes should ideally be batched together into a new major version.

I find it sad to cripple APIs for future compatibility reasons. Wasn't one of the goal of .NET Core to iterate faster since you don't have to worry anymore about a subtle .NET update breaking thousands of already installed applications?

My two cents.

@luisrudge

This comment has been minimized.

Show comment
Hide comment
@luisrudge

luisrudge May 8, 2015

@MrJul 馃憤
Versioning shouldn't be a problem. That's why the version numbers exist :)

@MrJul 馃憤
Versioning shouldn't be a problem. That's why the version numbers exist :)

@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj May 8, 2015

Member

Versioning is still very much an issue. Adding a member to an interface is a breaking change. For core libraries like this that are inbox in desktop we want to bring back features to desktop in future versions. If we fork it means folks can't write portable code that will run in both places. For more information on breaking changes have a look at: https://github.com/dotnet/corefx/wiki/Breaking-Changes.

I think this is a good discussion, but as I mentioned before I don't have a lot of scratch in the argument around interfaces vs abstract classes. Perhaps this is a topic to bring to the next design meeting?

I'm not super familiar with the test library you're using, but what I'd do is provide a hook to allow tests to set the instance of the client and then create a mock object that behaved however I want. The mock object could be mutable to permit some reuse.

If you have a specific compatible change you'd like to suggest to HttpClient that improves unit testability please propose it and @SidharthNabar and his team can consider it.

Member

ericstj commented May 8, 2015

Versioning is still very much an issue. Adding a member to an interface is a breaking change. For core libraries like this that are inbox in desktop we want to bring back features to desktop in future versions. If we fork it means folks can't write portable code that will run in both places. For more information on breaking changes have a look at: https://github.com/dotnet/corefx/wiki/Breaking-Changes.

I think this is a good discussion, but as I mentioned before I don't have a lot of scratch in the argument around interfaces vs abstract classes. Perhaps this is a topic to bring to the next design meeting?

I'm not super familiar with the test library you're using, but what I'd do is provide a hook to allow tests to set the instance of the client and then create a mock object that behaved however I want. The mock object could be mutable to permit some reuse.

If you have a specific compatible change you'd like to suggest to HttpClient that improves unit testability please propose it and @SidharthNabar and his team can consider it.

@KrzysztofCwalina

This comment has been minimized.

Show comment
Hide comment
@KrzysztofCwalina

KrzysztofCwalina May 8, 2015

Member

If the API is not mockable, we should fix it. But it has nothing to do with interfaces vs classes. Interface is no different than pure abstract class from mockability perspective, for all practical purposes.

Member

KrzysztofCwalina commented May 8, 2015

If the API is not mockable, we should fix it. But it has nothing to do with interfaces vs classes. Interface is no different than pure abstract class from mockability perspective, for all practical purposes.

@luisrudge

This comment has been minimized.

Show comment
Hide comment
@luisrudge

luisrudge May 8, 2015

@KrzysztofCwalina you, sir, hit the nail on the head! perfect summary!

@KrzysztofCwalina you, sir, hit the nail on the head! perfect summary!

@drub0y

This comment has been minimized.

Show comment
Hide comment
@drub0y

drub0y May 8, 2015

I guess I'm going to take the less popular side of this argument as I personally do not think an interface is necessary. As has already been pointed out, HttpClient is the abstraction already. The behavior really comes from the HttpMessageHandler. Yes, it's not mockable/fakeable using the approaches we've become accustom to with frameworks like Moq or FakeItEasy, but it doesn't need to be; that's not the only way there is do things. The API is still perfectly testable by design though.

So let's address the "I need to create my own HttpMessageHandler?". No, of course not. We didn't all write our own mocking libraries. @PureKrome has already shown his HttpClient.Helpers library. Personally I have not used that one, but I will check it out. I've been using @richardszalay's MockHttp library which I find fantastic. If you've ever worked with AngularJS's $httpBackend, MockHttp is taking exactly the same approach.

Dependency wise, your service class should allow an HttpClient to be injected and obviously can supply the reasonable default of new HttpClient(). If you need to be able to create instances, take a Func<HttpClient> or, if you're not a fan of Func<> for whatever reason, design your own IHttpClientFactory abstraction and a default implementation of that would, again, just return new HttpClient().

In closing, I find this API perfectly testable in its current form and see no need for an interface. Yes, it requires a different approach, but the aforementioned libraries already exist to help us with this differnt style of testing in perfectly logical, intuitive ways. If we want more functionality/simplicity let's contribute to those libraries to make them better.

drub0y commented May 8, 2015

I guess I'm going to take the less popular side of this argument as I personally do not think an interface is necessary. As has already been pointed out, HttpClient is the abstraction already. The behavior really comes from the HttpMessageHandler. Yes, it's not mockable/fakeable using the approaches we've become accustom to with frameworks like Moq or FakeItEasy, but it doesn't need to be; that's not the only way there is do things. The API is still perfectly testable by design though.

So let's address the "I need to create my own HttpMessageHandler?". No, of course not. We didn't all write our own mocking libraries. @PureKrome has already shown his HttpClient.Helpers library. Personally I have not used that one, but I will check it out. I've been using @richardszalay's MockHttp library which I find fantastic. If you've ever worked with AngularJS's $httpBackend, MockHttp is taking exactly the same approach.

Dependency wise, your service class should allow an HttpClient to be injected and obviously can supply the reasonable default of new HttpClient(). If you need to be able to create instances, take a Func<HttpClient> or, if you're not a fan of Func<> for whatever reason, design your own IHttpClientFactory abstraction and a default implementation of that would, again, just return new HttpClient().

In closing, I find this API perfectly testable in its current form and see no need for an interface. Yes, it requires a different approach, but the aforementioned libraries already exist to help us with this differnt style of testing in perfectly logical, intuitive ways. If we want more functionality/simplicity let's contribute to those libraries to make them better.

@luisrudge

This comment has been minimized.

Show comment
Hide comment
@luisrudge

luisrudge May 8, 2015

Personally, an Interface or virtual methods doesn't matter that much to me. But c'mon, perfectly testable is a little to much :)

Personally, an Interface or virtual methods doesn't matter that much to me. But c'mon, perfectly testable is a little to much :)

@drub0y

This comment has been minimized.

Show comment
Hide comment
@drub0y

drub0y May 8, 2015

@luisrudge Well can you give a scenario that can't be tested using the message handler style of testing that something like MockHttp enables? Maybe that would help make the case.

I haven't come across anything I couldn't codify yet, but maybe I'm missing some more esoteric scenarios that can't be covered. Even then, might just be a missing feature of that library that someone could contribute.

For now I maintain the opinion that it's "perfectly testable" as a dependency, just not in a way .NET devs are used to.

drub0y commented May 8, 2015

@luisrudge Well can you give a scenario that can't be tested using the message handler style of testing that something like MockHttp enables? Maybe that would help make the case.

I haven't come across anything I couldn't codify yet, but maybe I'm missing some more esoteric scenarios that can't be covered. Even then, might just be a missing feature of that library that someone could contribute.

For now I maintain the opinion that it's "perfectly testable" as a dependency, just not in a way .NET devs are used to.

@luisrudge

This comment has been minimized.

Show comment
Hide comment
@luisrudge

luisrudge May 8, 2015

It's testable, I agree. but it's still a pain. If you have to use an external lib that helps you do that, it's not perfectly testable. But I guess that's too subjective to have a discussion over it.

It's testable, I agree. but it's still a pain. If you have to use an external lib that helps you do that, it's not perfectly testable. But I guess that's too subjective to have a discussion over it.

@luisrudge

This comment has been minimized.

Show comment
Hide comment
@luisrudge

luisrudge May 8, 2015

One could argue that aspnet mvc 5 is perfectly testable, you just have to write 50LOC to mock every single thing a controller needs. IMHO, that's the same case with HttpClient. It's testable? Yes. It's easy to test? No.

One could argue that aspnet mvc 5 is perfectly testable, you just have to write 50LOC to mock every single thing a controller needs. IMHO, that's the same case with HttpClient. It's testable? Yes. It's easy to test? No.

@drub0y

This comment has been minimized.

Show comment
Hide comment
@drub0y

drub0y May 8, 2015

@luisrudge Yes, I agree, it's subjective and I completely understand the desire for interface/virtuals. I'm just trying to make sure anyone who comes along and reads this thread will at least get some education on the fact that this API can be leveraged in a code base in a very testable way without introducing all of your own abstractions around HttpClient to get there.

if you have to use an external lib that helps you do that, it's not perfectly testable

Well, we're all using one library or another for mocking/faking already* and, it's true, we can't just use that one for testing this style of API, but I don't think that means its any less testable than an interface based approach just because I have to bring in another library.

* At least I hope not!

drub0y commented May 8, 2015

@luisrudge Yes, I agree, it's subjective and I completely understand the desire for interface/virtuals. I'm just trying to make sure anyone who comes along and reads this thread will at least get some education on the fact that this API can be leveraged in a code base in a very testable way without introducing all of your own abstractions around HttpClient to get there.

if you have to use an external lib that helps you do that, it's not perfectly testable

Well, we're all using one library or another for mocking/faking already* and, it's true, we can't just use that one for testing this style of API, but I don't think that means its any less testable than an interface based approach just because I have to bring in another library.

* At least I hope not!

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome May 9, 2015

@drub0y from my OP I've stated that the library is testable - but it's just a real pain compared (to what i passionately believe) to what it could be. IMO @luisrudge spelt it out perfectly :

One could argue that aspnet mvc 5 is perfectly testable, you just have to write 50LOC to mock every single thing a controller needs

This repo is a major part of a large number of developers. So the default (and understandable) tactic is to be very cautious with this repo. Sure - I totally get that.

I'd like to believe that this repo is still in a position to be tweaked (with respect to the API) before it goes RTW. Future changes suddenly become really hard to do, include the perception to change.

So with the current api - can it be tested? Yep. Does it pass the Dark Matter Developer test? I personally don't think so.

The litmus test IMO is this: can a regular joe pickup one of the common/popular test frameworks + mocking frameworks and mock any of the main methods in this API? Right now - nope. So then the developer needs to stop what they're doing and start learning about the implimentation of this library.

As has already been pointed out, HttpClient is the abstraction already. The behavior really comes from the HttpMessageHandler.

This is my point - why are you making us having to spend cycles figuring this out when the library's purpose is to abstract all that magic .. only to say "Ok .. so we've done some magic but now that you want to mock our magic ... I'm sorry, you're now going to have to pull up your sleeves, lift up the roof of the library and start digging inside, etc".

It feels so ... convoluted.

We're in a position to make life easy for so many people and to keep coming back to the defensive position of : "It can be done - but .. read the FAQ/Code".

Here's another angle to approach this issue: Give 2 example to random Non-MS devs ... joe citizens developers that know how to use xUnit and Moq/FakeItEasy/InsertTheHipMockingFrameworkThisYear.

  • First API that uses an interface/virtual/whatever .. but the method can be mocked.
  • Second API that is just a public method and to mock the integration point .. they have to read the code.

It distills down to that, IMO.

See which developer can solve that problem and stay happy.

If the API is not mockable, we should fix it.

Right now it's not IMO - but there's ways to get around this successfully (again, that's opinionated - I'll concede that)

But it has nothing to do with interfaces vs classes. Interface is no different than pure abstract class from mockability perspective, for all practical purposes.

Exactly - that's an implementation detail. Goal is to be able to use a battle-tested mock framework and off you go.

@luisrudge if you have to use an external lib that helps you do that, it's not perfectly testable
@drub0y Well, we're all using one library or another for mocking/faking already

(I hope I understood that last quote/paragraph..) Not ... quiet. What @luisrudge was saying is: "We have one tool for testing. A second tool for mocking. So far, those a generic/overall tools not tied to anything. But .. you now want me to download a 3rd tool which is specific to mocking a specific API/service in our code because that specific API/service we're using, wasn't designed to be tested nicely?". That's a bit rich :(

Dependency wise, your service class should allow an HttpClient to be injected and obviously can supply the reasonable default of new HttpClient()

Completely agreed! So, can you refactor the sample code above to show us how easy this should/can be? There's many ways to skin a cat ... so what's a simple AND easy way? Show us the code.

I'll start. Appologies to @KrzysztofCwalina for using an interface, but this is just to kickstart my litmus test.

public interface IHttpClient
{
    Task<string> GetStringAsync(string url);
}

public class SomeService
{
    private IHttpClient _httpClient;

    // Injected.
    public SomeService(IHttpClient httpClient) { .. }

    public async Task<string> GetSomeData(string url)
    {
        string result;
        using (var httpClient = _httpClient ?? new HttpClient())
        {
            result = await httpClient.GetStringAsync(url);
        }

        return result;
    }    
}

@drub0y from my OP I've stated that the library is testable - but it's just a real pain compared (to what i passionately believe) to what it could be. IMO @luisrudge spelt it out perfectly :

One could argue that aspnet mvc 5 is perfectly testable, you just have to write 50LOC to mock every single thing a controller needs

This repo is a major part of a large number of developers. So the default (and understandable) tactic is to be very cautious with this repo. Sure - I totally get that.

I'd like to believe that this repo is still in a position to be tweaked (with respect to the API) before it goes RTW. Future changes suddenly become really hard to do, include the perception to change.

So with the current api - can it be tested? Yep. Does it pass the Dark Matter Developer test? I personally don't think so.

The litmus test IMO is this: can a regular joe pickup one of the common/popular test frameworks + mocking frameworks and mock any of the main methods in this API? Right now - nope. So then the developer needs to stop what they're doing and start learning about the implimentation of this library.

As has already been pointed out, HttpClient is the abstraction already. The behavior really comes from the HttpMessageHandler.

This is my point - why are you making us having to spend cycles figuring this out when the library's purpose is to abstract all that magic .. only to say "Ok .. so we've done some magic but now that you want to mock our magic ... I'm sorry, you're now going to have to pull up your sleeves, lift up the roof of the library and start digging inside, etc".

It feels so ... convoluted.

We're in a position to make life easy for so many people and to keep coming back to the defensive position of : "It can be done - but .. read the FAQ/Code".

Here's another angle to approach this issue: Give 2 example to random Non-MS devs ... joe citizens developers that know how to use xUnit and Moq/FakeItEasy/InsertTheHipMockingFrameworkThisYear.

  • First API that uses an interface/virtual/whatever .. but the method can be mocked.
  • Second API that is just a public method and to mock the integration point .. they have to read the code.

It distills down to that, IMO.

See which developer can solve that problem and stay happy.

If the API is not mockable, we should fix it.

Right now it's not IMO - but there's ways to get around this successfully (again, that's opinionated - I'll concede that)

But it has nothing to do with interfaces vs classes. Interface is no different than pure abstract class from mockability perspective, for all practical purposes.

Exactly - that's an implementation detail. Goal is to be able to use a battle-tested mock framework and off you go.

@luisrudge if you have to use an external lib that helps you do that, it's not perfectly testable
@drub0y Well, we're all using one library or another for mocking/faking already

(I hope I understood that last quote/paragraph..) Not ... quiet. What @luisrudge was saying is: "We have one tool for testing. A second tool for mocking. So far, those a generic/overall tools not tied to anything. But .. you now want me to download a 3rd tool which is specific to mocking a specific API/service in our code because that specific API/service we're using, wasn't designed to be tested nicely?". That's a bit rich :(

Dependency wise, your service class should allow an HttpClient to be injected and obviously can supply the reasonable default of new HttpClient()

Completely agreed! So, can you refactor the sample code above to show us how easy this should/can be? There's many ways to skin a cat ... so what's a simple AND easy way? Show us the code.

I'll start. Appologies to @KrzysztofCwalina for using an interface, but this is just to kickstart my litmus test.

public interface IHttpClient
{
    Task<string> GetStringAsync(string url);
}

public class SomeService
{
    private IHttpClient _httpClient;

    // Injected.
    public SomeService(IHttpClient httpClient) { .. }

    public async Task<string> GetSomeData(string url)
    {
        string result;
        using (var httpClient = _httpClient ?? new HttpClient())
        {
            result = await httpClient.GetStringAsync(url);
        }

        return result;
    }    
}
@sharwell

This comment has been minimized.

Show comment
Hide comment
@sharwell

sharwell May 9, 2015

Member

It sounds like all @PureKrome needs is a documentation update explaining which methods to mock/override in order to customize the behavior of the API during testing.

Member

sharwell commented May 9, 2015

It sounds like all @PureKrome needs is a documentation update explaining which methods to mock/override in order to customize the behavior of the API during testing.

@luisrudge

This comment has been minimized.

Show comment
Hide comment
@luisrudge

luisrudge May 9, 2015

@sharwell that's absolutely not the case. When I test stuff, I don't want to run the entire httpclient code:
Look the SendAsync method.
It's insane. What you're suggesting is that every developer should know the internals from the class, open github, see the code and that kind of stuff to be able to test it. I want that developer to mock the GetStringAsync and get shit done.

@sharwell that's absolutely not the case. When I test stuff, I don't want to run the entire httpclient code:
Look the SendAsync method.
It's insane. What you're suggesting is that every developer should know the internals from the class, open github, see the code and that kind of stuff to be able to test it. I want that developer to mock the GetStringAsync and get shit done.

@sharwell

This comment has been minimized.

Show comment
Hide comment
@sharwell

sharwell May 9, 2015

Member

@luisrudge Actually my suggestion would be to avoid mocking for this library altogether. Since you already have a clean abstraction layer with a decoupled, replaceable implementation, additional mocking is unnecessary. Mocking for this library has the following additional downsides:

  1. Increased burden on developers creating tests (maintaining the mocks), which in turn increases development costs
  2. Increased likelihood that your tests will fail to detect certain problematic behaviors, such as reusing the content stream associated with a message (sending a message results in a call to Dispose on the content stream, but most mocks will ignore this)
  3. Unnecessarily tighter coupling of the application to a particular abstraction layer, which increases development costs associated with evaluating alternatives to HttpClient

Mocking is a testing strategy targeting intertwined codebases that are difficult to test at a small scale. While the use of mocking correlates with increased development costs, the need for mocking correlates with increased amount of coupling in the code. This means mocking itself is a code smell. If you can provide the same input-output test coverage across and within your API without using mocking, you will benefit in basically every development aspect.

Member

sharwell commented May 9, 2015

@luisrudge Actually my suggestion would be to avoid mocking for this library altogether. Since you already have a clean abstraction layer with a decoupled, replaceable implementation, additional mocking is unnecessary. Mocking for this library has the following additional downsides:

  1. Increased burden on developers creating tests (maintaining the mocks), which in turn increases development costs
  2. Increased likelihood that your tests will fail to detect certain problematic behaviors, such as reusing the content stream associated with a message (sending a message results in a call to Dispose on the content stream, but most mocks will ignore this)
  3. Unnecessarily tighter coupling of the application to a particular abstraction layer, which increases development costs associated with evaluating alternatives to HttpClient

Mocking is a testing strategy targeting intertwined codebases that are difficult to test at a small scale. While the use of mocking correlates with increased development costs, the need for mocking correlates with increased amount of coupling in the code. This means mocking itself is a code smell. If you can provide the same input-output test coverage across and within your API without using mocking, you will benefit in basically every development aspect.

@ctolkien

This comment has been minimized.

Show comment
Hide comment
@ctolkien

ctolkien May 11, 2015

Since you already have a clean abstraction layer with a decoupled, replaceable implementation, additional mocking is unnecessary

With respect, if the option is having to write your own Faked Message Handler, which isn't obvious without digging through the code, then that is a very high barrier that is going to be hugely frustrating for a lot of developers.

I agree with @luisrudge 's comment earlier. Technically MVC5 was testable, but my god was it a pain in the ass and an immense source of hair pulling.

Since you already have a clean abstraction layer with a decoupled, replaceable implementation, additional mocking is unnecessary

With respect, if the option is having to write your own Faked Message Handler, which isn't obvious without digging through the code, then that is a very high barrier that is going to be hugely frustrating for a lot of developers.

I agree with @luisrudge 's comment earlier. Technically MVC5 was testable, but my god was it a pain in the ass and an immense source of hair pulling.

@luisrudge

This comment has been minimized.

Show comment
Hide comment
@luisrudge

luisrudge May 11, 2015

@sharwell Are you saying that mocking IHttpClient is a burden and implementing a fake message handler (like this one isn't? I can't agree with that, sorry.

@sharwell Are you saying that mocking IHttpClient is a burden and implementing a fake message handler (like this one isn't? I can't agree with that, sorry.

@sharwell

This comment has been minimized.

Show comment
Hide comment
@sharwell

sharwell May 11, 2015

Member

@luisrudge For simple cases of testing GetStringAsync, it's not nearly that hard to mock the handler. Using out-of-the-box Moq + HttpClient, all you need is this:

Uri requestUri = new Uri("http://google.com");
string expectedResponse = "Response text";

Mock<HttpClientHandler> mockHandler = new Mock<HttpClientHandler>();
mockHandler.Protected()
    .Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.Is<HttpRequestMessage>(message => message.RequestUri == requestUri), ItExpr.IsAny<CancellationToken>())
    .Returns(Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(expectedResponse) }));
HttpClient httpClient = new HttpClient(mockHandler.Object);
string result = await httpClient.GetStringAsync(requestUri).ConfigureAwait(false);
Assert.AreEqual(expectedResponse, result);

If that's too much, you can define a helper class:

internal static class MockHttpClientHandlerExtensions
{
    public static void SetupGetStringAsync(this Mock<HttpClientHandler> mockHandler, Uri requestUri, string response)
    {
        mockHandler.Protected()
            .Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.Is<HttpRequestMessage>(message => message.RequestUri == requestUri), ItExpr.IsAny<CancellationToken>())
            .Returns(Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(response) }));
    }
}

Using the helper class all you need is this:

Uri requestUri = new Uri("http://google.com");
string expectedResponse = "Response text";

Mock<HttpClientHandler> mockHandler = new Mock<HttpClientHandler>();
mockHandler.SetupGetStringAsync(requestUri, expectedResponse);
HttpClient httpClient = new HttpClient(mockHandler.Object);
string result = await httpClient.GetStringAsync(requestUri).ConfigureAwait(false);
Assert.AreEqual(expectedResponse, result);
Member

sharwell commented May 11, 2015

@luisrudge For simple cases of testing GetStringAsync, it's not nearly that hard to mock the handler. Using out-of-the-box Moq + HttpClient, all you need is this:

Uri requestUri = new Uri("http://google.com");
string expectedResponse = "Response text";

Mock<HttpClientHandler> mockHandler = new Mock<HttpClientHandler>();
mockHandler.Protected()
    .Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.Is<HttpRequestMessage>(message => message.RequestUri == requestUri), ItExpr.IsAny<CancellationToken>())
    .Returns(Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(expectedResponse) }));
HttpClient httpClient = new HttpClient(mockHandler.Object);
string result = await httpClient.GetStringAsync(requestUri).ConfigureAwait(false);
Assert.AreEqual(expectedResponse, result);

If that's too much, you can define a helper class:

internal static class MockHttpClientHandlerExtensions
{
    public static void SetupGetStringAsync(this Mock<HttpClientHandler> mockHandler, Uri requestUri, string response)
    {
        mockHandler.Protected()
            .Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.Is<HttpRequestMessage>(message => message.RequestUri == requestUri), ItExpr.IsAny<CancellationToken>())
            .Returns(Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(response) }));
    }
}

Using the helper class all you need is this:

Uri requestUri = new Uri("http://google.com");
string expectedResponse = "Response text";

Mock<HttpClientHandler> mockHandler = new Mock<HttpClientHandler>();
mockHandler.SetupGetStringAsync(requestUri, expectedResponse);
HttpClient httpClient = new HttpClient(mockHandler.Object);
string result = await httpClient.GetStringAsync(requestUri).ConfigureAwait(false);
Assert.AreEqual(expectedResponse, result);
@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome May 11, 2015

So to compare the two versions:
Disclaimer: this is untested, browser code. Also, I haven't used Moq in ages.

Current

public class SomeService(HttpClientHandler httpClientHandler= null)
{
    public async Foo GetSomethingFromTheInternetAsync()
    {
        ....
        using (var httpClient = _handler == null
                                  ? new HttpClient
                                  : new HttpClient(handler))
        {
            html = await httpClient.GetStringAsync("http://www.google.com.au");
        }
        ....
    } 
}

// Unit test...
// Arrange.
Uri requestUri = new Uri("http://google.com");
string expectedResponse = "Response text";
var responseMessage = new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(expectedResponse) };
Mock<HttpClientHandler> mockHandler = new Mock<HttpClientHandler>();
mockHandler.Protected()
    .Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.Is<HttpRequestMessage>(message => message.RequestUri == requestUri), ItExpr.IsAny<CancellationToken>())
    .ReturnsAsync(responseMessage);
HttpClient httpClient = new HttpClient(mockHandler.Object);
var someService = new SomeService(mockHandler.Object); // Injected...

// Act.
string result = await someService.GetSomethingFromTheInternetAsync();

// Assert.
Assert.AreEqual(expectedResponse, result);
httpClient.Verify(x => x.GetSomethingFromTheInternetAsync(), Times.Once);

versus offering a way to mock the API method directly.

Another way

public class SomeService(IHttpClient httpClient = null)
{
    public async Foo GetSomethingFromTheInternetAsync()
    {
        ....
        using (var httpClient = _httpClient ?? new HttpClient())
        {
            html = await httpClient.GetStringAsync("http://www.google.com.au");
        }
        ....
    } 
}

// Unit tests...
// Arrange.
var response = new Foo();
var httpClient = new Mock<IHttpClient>();
httpClient.Setup(x => x.GetStringAsync(It.IsAny<string>))
    .ReturnsAsync(response);
var service = new SomeService(httpClient.Object); // Injected.

// Act.
var result = await service.GetSomethingFromTheInternetAsync();

// Assert.
Assert.Equals("something", foo.Something);
httpClient.Verify(x => x.GetStringAsync(It.IsAny<string>()), Times.Once);

distilling that down, it mainly comes to this (excluding the slight differece in SomeService) ...

var responseMessage = new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(expectedResponse) };
Mock<HttpClientHandler> mockHandler = new Mock<HttpClientHandler>();
mockHandler.Protected()
    .Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.Is<HttpRequestMessage>(message => message.RequestUri == requestUri), ItExpr.IsAny<CancellationToken>())
    .ReturnsAsync(responseMessage);
HttpClient httpClient = new HttpClient(mockHandler.Object);

vs this ...

var httpClient = new Mock<IHttpClient>();
httpClient.Setup(x => x.GetStringAsync(It.IsAny<string>))
    .ReturnsAsync(response);

Does the code roughly look about right for both? (both have to be fairly accurate, before we can compare the two).

So to compare the two versions:
Disclaimer: this is untested, browser code. Also, I haven't used Moq in ages.

Current

public class SomeService(HttpClientHandler httpClientHandler= null)
{
    public async Foo GetSomethingFromTheInternetAsync()
    {
        ....
        using (var httpClient = _handler == null
                                  ? new HttpClient
                                  : new HttpClient(handler))
        {
            html = await httpClient.GetStringAsync("http://www.google.com.au");
        }
        ....
    } 
}

// Unit test...
// Arrange.
Uri requestUri = new Uri("http://google.com");
string expectedResponse = "Response text";
var responseMessage = new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(expectedResponse) };
Mock<HttpClientHandler> mockHandler = new Mock<HttpClientHandler>();
mockHandler.Protected()
    .Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.Is<HttpRequestMessage>(message => message.RequestUri == requestUri), ItExpr.IsAny<CancellationToken>())
    .ReturnsAsync(responseMessage);
HttpClient httpClient = new HttpClient(mockHandler.Object);
var someService = new SomeService(mockHandler.Object); // Injected...

// Act.
string result = await someService.GetSomethingFromTheInternetAsync();

// Assert.
Assert.AreEqual(expectedResponse, result);
httpClient.Verify(x => x.GetSomethingFromTheInternetAsync(), Times.Once);

versus offering a way to mock the API method directly.

Another way

public class SomeService(IHttpClient httpClient = null)
{
    public async Foo GetSomethingFromTheInternetAsync()
    {
        ....
        using (var httpClient = _httpClient ?? new HttpClient())
        {
            html = await httpClient.GetStringAsync("http://www.google.com.au");
        }
        ....
    } 
}

// Unit tests...
// Arrange.
var response = new Foo();
var httpClient = new Mock<IHttpClient>();
httpClient.Setup(x => x.GetStringAsync(It.IsAny<string>))
    .ReturnsAsync(response);
var service = new SomeService(httpClient.Object); // Injected.

// Act.
var result = await service.GetSomethingFromTheInternetAsync();

// Assert.
Assert.Equals("something", foo.Something);
httpClient.Verify(x => x.GetStringAsync(It.IsAny<string>()), Times.Once);

distilling that down, it mainly comes to this (excluding the slight differece in SomeService) ...

var responseMessage = new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(expectedResponse) };
Mock<HttpClientHandler> mockHandler = new Mock<HttpClientHandler>();
mockHandler.Protected()
    .Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.Is<HttpRequestMessage>(message => message.RequestUri == requestUri), ItExpr.IsAny<CancellationToken>())
    .ReturnsAsync(responseMessage);
HttpClient httpClient = new HttpClient(mockHandler.Object);

vs this ...

var httpClient = new Mock<IHttpClient>();
httpClient.Setup(x => x.GetStringAsync(It.IsAny<string>))
    .ReturnsAsync(response);

Does the code roughly look about right for both? (both have to be fairly accurate, before we can compare the two).

@luisrudge

This comment has been minimized.

Show comment
Hide comment
@luisrudge

luisrudge May 11, 2015

@sharwell so I have to run my code through all HttpClient.cs code in order to run my tests? How is that less coupled with HttpClient?

@sharwell so I have to run my code through all HttpClient.cs code in order to run my tests? How is that less coupled with HttpClient?

@raymens

This comment has been minimized.

Show comment
Hide comment
@raymens

raymens May 13, 2015

We heavily use HttpClient in some of our systems and I wouldn't call it a perfectly fine testable piece of tech. I really like the library but having some sort of Interface or any other improvement for testing would be a major improvement IMO.

Kudo's to @PureKrome for his library, but it would be better if it wasn't necessary :)

raymens commented May 13, 2015

We heavily use HttpClient in some of our systems and I wouldn't call it a perfectly fine testable piece of tech. I really like the library but having some sort of Interface or any other improvement for testing would be a major improvement IMO.

Kudo's to @PureKrome for his library, but it would be better if it wasn't necessary :)

@tucaz

This comment has been minimized.

Show comment
Hide comment
@tucaz

tucaz May 14, 2015

I just read all comments and learned a lot, but I have one question: why do you need to mock the HttpClient in your tests?

In most common scenarios functionality provided by HttpClient will already be wrapped in some class such as HtmlDownloader.DownloadFile(). Why don't just mock this class instead of its plumbing?

Another possible scenario that would require such mocking is when you have to parse a downloaded string and this piece of code requires testing for multiple different outputs. Even in this case, this functionality (parsing) could be extracted away to a class/method and tested without any mocks.

I'm not saying that the testability of it cannot be improved, neither implying that it shouldn't be. I just want to understand to learn some new stuff.

tucaz commented May 14, 2015

I just read all comments and learned a lot, but I have one question: why do you need to mock the HttpClient in your tests?

In most common scenarios functionality provided by HttpClient will already be wrapped in some class such as HtmlDownloader.DownloadFile(). Why don't just mock this class instead of its plumbing?

Another possible scenario that would require such mocking is when you have to parse a downloaded string and this piece of code requires testing for multiple different outputs. Even in this case, this functionality (parsing) could be extracted away to a class/method and tested without any mocks.

I'm not saying that the testability of it cannot be improved, neither implying that it shouldn't be. I just want to understand to learn some new stuff.

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome May 14, 2015

@tucaz Lets imagine I have a website that returns some information about yourself. Your age, name, birthday, etc. You also happen to have stocks in some companies.

You've said you own 100 units of AAA stock and 200 units of BBB stock? So, when we display your account details we should display how much money your stocks are worth.

To do this, we need to grab the current stock prices from another system. So to do this, we need to retrieve the stock's with a service, right? Lets to this...

Disclaimer: browser code...

public class StockService : IStockService
{
    private readonly IHttpClient _httpClient;

    public StockService(IHttpClient httpClient)
    {
        _httpClient = httpClient; // Optional.
    }

    public async Task<IEnumerable<StockResults>> GetStocksAsync(IEnumerable<string> stockCodes)
    {
        var queryString = ConvertStockCodeListIntoQuerystring();
        string jsonResult;
        using (var httpClient = _httpClient ?? new HttpClient())
        {
            jsonResult = await httpClient.GetStringAsync("http:\\www.stocksOnline.com\stockResults?" + queryString);
        }

        return JsonConvert.DeserializeObject<StockResult>(jsonResult);
    }
}

Ok, so here we have a simple service that says: "Go get me the stock prices for stocks AAA and BBB";

So, I need to test this out:

  • Given some legit stock names
  • Given some invalid/non-existant stock names
  • httpClient throws an exception (the internet blew up while waiting for a response)

and now I can easily test for those scenario's. I notice that when the httpClient blows up (throws an exception) .. this service blows up.. so maybe I need to add some logging and return null? donno .. but at least I can think about the problem and then handle it.

Of course - i 100% don't want to really hit the real web service during my normal unit tests. By injecting the mock, I can use that. Otherwise I can create a new instance and use that.

So i'm trying to establish that my suggestion is a hellava lot easier (to read/maintain/etc) that the current status quo.

In most common scenarios functionality provided by HttpClient will already be wrapped in some class such as HtmlDownloader.DownloadFile().

Why should we wrap HttpClient? It's already an abstraction over all the http underneath. It's a great library! I personally don't see what that would achieve?

@tucaz Lets imagine I have a website that returns some information about yourself. Your age, name, birthday, etc. You also happen to have stocks in some companies.

You've said you own 100 units of AAA stock and 200 units of BBB stock? So, when we display your account details we should display how much money your stocks are worth.

To do this, we need to grab the current stock prices from another system. So to do this, we need to retrieve the stock's with a service, right? Lets to this...

Disclaimer: browser code...

public class StockService : IStockService
{
    private readonly IHttpClient _httpClient;

    public StockService(IHttpClient httpClient)
    {
        _httpClient = httpClient; // Optional.
    }

    public async Task<IEnumerable<StockResults>> GetStocksAsync(IEnumerable<string> stockCodes)
    {
        var queryString = ConvertStockCodeListIntoQuerystring();
        string jsonResult;
        using (var httpClient = _httpClient ?? new HttpClient())
        {
            jsonResult = await httpClient.GetStringAsync("http:\\www.stocksOnline.com\stockResults?" + queryString);
        }

        return JsonConvert.DeserializeObject<StockResult>(jsonResult);
    }
}

Ok, so here we have a simple service that says: "Go get me the stock prices for stocks AAA and BBB";

So, I need to test this out:

  • Given some legit stock names
  • Given some invalid/non-existant stock names
  • httpClient throws an exception (the internet blew up while waiting for a response)

and now I can easily test for those scenario's. I notice that when the httpClient blows up (throws an exception) .. this service blows up.. so maybe I need to add some logging and return null? donno .. but at least I can think about the problem and then handle it.

Of course - i 100% don't want to really hit the real web service during my normal unit tests. By injecting the mock, I can use that. Otherwise I can create a new instance and use that.

So i'm trying to establish that my suggestion is a hellava lot easier (to read/maintain/etc) that the current status quo.

In most common scenarios functionality provided by HttpClient will already be wrapped in some class such as HtmlDownloader.DownloadFile().

Why should we wrap HttpClient? It's already an abstraction over all the http underneath. It's a great library! I personally don't see what that would achieve?

@tucaz

This comment has been minimized.

Show comment
Hide comment
@tucaz

tucaz May 14, 2015

@PureKrome your usage example is exactly what I mean by wrapping the functionality in a wrapping class.

I'm not sure I can understand what you are trying to test, but for me looks like you are testing the underlying webservice when what you really need to assert is that whoever is consuming it will be able to handle whatever comes back from the GetStockAsync method.

Please consider the following:

private IStockService stockService;

[Setup]
public void Setup()
{
    stockService = A.Fake<IStockService>();

    A.CallTo(() => stockService.GetStocksAsync(new [] { "Ticker1" }))
        .Returns(new StockResults[] { new StockResults { Price = 100m, Ticker = "Ticker1", Status = "OK" }});
    A.CallTo(() => stockService.GetStocksAsync(new [] { "Ticker2" }))
        .Returns(new StockResults[] { new StockResults { Price = 0m, Ticker = "Ticker2", Status = "NotFound" }});
    A.CallTo(() => stockService.GetStocksAsync(new [] { "Ticker3" }))
        .Throws(() => new InvalidOperationException("Some weird message"));
}

[Test]
public async void Get_Total_Stock_Quotes()
{
    var stockService = A.Fake<IStockService>();

    var total = await stockService.GetStockAsync(new [] { "Ticker1" });

    Assert.IsNotNull(total);
    Assert.IsGreaterThan(0, total.Sum(x => x.Price);
}

[Test]
public async void Hints_When_Ticker_Not_Found()
{
    var stockService = A.Fake<IStockService>();

    var total = await stockService.GetStockAsync(new [] { "Ticker2" });

    Assert.IsNotNull(total);
    Assert.AnyIs(x => x.Status == "NotFound");
}

[Test]
public async void Throws_InvalidOperationException_When_Error()
{
    var stockService = A.Fake<IStockService>();

    Assert.Throws(() => await stockService.GetStockAsync(new [] { "Ticker3" }), typeof(InvalidOperationException));
}

I'm proposing that you increase your abstraction one level up and deal with the actual business logic since there isn't anything that needs to be tested regarding the behavior of the GetStockAsync method.

The only scenario that I can think of needing to mock the HttpClient is when you need to test some retry logic depending on the response received from the server or something like that. In this case, you will be building a more complicated piece of code that will probably be extracted into a HttpClientWithRetryLogic class and not spread in every method you code that uses the HttpClient class.

If this is case, you can use the message handler as suggested or built a class to wrap all this retry logic because HttpClient will be a really small part of the important thing that your class does.

tucaz commented May 14, 2015

@PureKrome your usage example is exactly what I mean by wrapping the functionality in a wrapping class.

I'm not sure I can understand what you are trying to test, but for me looks like you are testing the underlying webservice when what you really need to assert is that whoever is consuming it will be able to handle whatever comes back from the GetStockAsync method.

Please consider the following:

private IStockService stockService;

[Setup]
public void Setup()
{
    stockService = A.Fake<IStockService>();

    A.CallTo(() => stockService.GetStocksAsync(new [] { "Ticker1" }))
        .Returns(new StockResults[] { new StockResults { Price = 100m, Ticker = "Ticker1", Status = "OK" }});
    A.CallTo(() => stockService.GetStocksAsync(new [] { "Ticker2" }))
        .Returns(new StockResults[] { new StockResults { Price = 0m, Ticker = "Ticker2", Status = "NotFound" }});
    A.CallTo(() => stockService.GetStocksAsync(new [] { "Ticker3" }))
        .Throws(() => new InvalidOperationException("Some weird message"));
}

[Test]
public async void Get_Total_Stock_Quotes()
{
    var stockService = A.Fake<IStockService>();

    var total = await stockService.GetStockAsync(new [] { "Ticker1" });

    Assert.IsNotNull(total);
    Assert.IsGreaterThan(0, total.Sum(x => x.Price);
}

[Test]
public async void Hints_When_Ticker_Not_Found()
{
    var stockService = A.Fake<IStockService>();

    var total = await stockService.GetStockAsync(new [] { "Ticker2" });

    Assert.IsNotNull(total);
    Assert.AnyIs(x => x.Status == "NotFound");
}

[Test]
public async void Throws_InvalidOperationException_When_Error()
{
    var stockService = A.Fake<IStockService>();

    Assert.Throws(() => await stockService.GetStockAsync(new [] { "Ticker3" }), typeof(InvalidOperationException));
}

I'm proposing that you increase your abstraction one level up and deal with the actual business logic since there isn't anything that needs to be tested regarding the behavior of the GetStockAsync method.

The only scenario that I can think of needing to mock the HttpClient is when you need to test some retry logic depending on the response received from the server or something like that. In this case, you will be building a more complicated piece of code that will probably be extracted into a HttpClientWithRetryLogic class and not spread in every method you code that uses the HttpClient class.

If this is case, you can use the message handler as suggested or built a class to wrap all this retry logic because HttpClient will be a really small part of the important thing that your class does.

@MrJul

This comment has been minimized.

Show comment
Hide comment
@MrJul

MrJul May 14, 2015

@tucaz Didn't you just test that your fake returns what you configured, effectively testing the mocking library? @PureKrome wants to test the concrete StockService implementation.

The newly introduced level of abstraction should be wrapping HttpClient calls to be able to properly test StockService. Chances are that wrapper will simply be delegating every call to HttpClient, and add unnecessary complexity to the application.

You can always add a new layer of abstraction to hide non-mockable types. What's criticized in this thread is that mocking HttpClient is possible, but should probably be easier / in line of what's usually done with other libraries.

MrJul commented May 14, 2015

@tucaz Didn't you just test that your fake returns what you configured, effectively testing the mocking library? @PureKrome wants to test the concrete StockService implementation.

The newly introduced level of abstraction should be wrapping HttpClient calls to be able to properly test StockService. Chances are that wrapper will simply be delegating every call to HttpClient, and add unnecessary complexity to the application.

You can always add a new layer of abstraction to hide non-mockable types. What's criticized in this thread is that mocking HttpClient is possible, but should probably be easier / in line of what's usually done with other libraries.

@tucaz

This comment has been minimized.

Show comment
Hide comment
@tucaz

tucaz May 14, 2015

@MrJul you are correct. That's why I said that what's been tested by his example is the webservice implementation itself and why I proposed (without code examples. My bad) that he mocks the IStockService so he can assert that the code consuming it is able to handle whatever the GetStockAsync returns.

That's what I think should be tested in the first place. Not the mocking framework (like I did) or the webservice implementation (like @PureKrome wants to do).

At the end of the day I can understand that it would be nice to have an IHttpClient interface, but fail to see any utility in these kinds of scenarios since IMO what's need to be tested is how consumers of the service abstraction (and therefore the webservice) can handle it's return appropriately.

tucaz commented May 14, 2015

@MrJul you are correct. That's why I said that what's been tested by his example is the webservice implementation itself and why I proposed (without code examples. My bad) that he mocks the IStockService so he can assert that the code consuming it is able to handle whatever the GetStockAsync returns.

That's what I think should be tested in the first place. Not the mocking framework (like I did) or the webservice implementation (like @PureKrome wants to do).

At the end of the day I can understand that it would be nice to have an IHttpClient interface, but fail to see any utility in these kinds of scenarios since IMO what's need to be tested is how consumers of the service abstraction (and therefore the webservice) can handle it's return appropriately.

@dasjestyr

This comment has been minimized.

Show comment
Hide comment
@dasjestyr

dasjestyr Dec 7, 2016

Is this really unit testing? Be honest now :)

I dunno. What does the test look like?

I think the statement is a bit of a strawman. The point is to mock the dependency which is the http client, so that the code can run in isolation from those dependencies... in other words, not actually make an http call. All we'd be doing is make sure that the expected methods on the interface were being called under the right conditions. Whether or not we should have chosen a lighter dependency seems to be a completely different discussion.

dasjestyr commented Dec 7, 2016

Is this really unit testing? Be honest now :)

I dunno. What does the test look like?

I think the statement is a bit of a strawman. The point is to mock the dependency which is the http client, so that the code can run in isolation from those dependencies... in other words, not actually make an http call. All we'd be doing is make sure that the expected methods on the interface were being called under the right conditions. Whether or not we should have chosen a lighter dependency seems to be a completely different discussion.

@damianh

This comment has been minimized.

Show comment
Hide comment
@damianh

damianh Dec 7, 2016

damianh commented Dec 7, 2016

@dasjestyr

This comment has been minimized.

Show comment
Hide comment
@dasjestyr

dasjestyr Dec 9, 2016

This is basically not following Unit Testing 101 -- getting unrelated dependency code out of your tests =).

Personally, I don't think that's the right interpretation of that particular practice. We're not trying to test the dependency, we're trying to test how our code interacts with that dependency, thus it is not unrelated. And, quite simply, If I can't run a unit test against my code without invoking the actual dependency, then that's not testing in isolation. For example, if that unit test was run on a build server without internet access, then the test would likely break -- hence the desire to mock it. You could remove the dependency on HttpClient, but that just means it's going to end up in some other class that that you write which makes the call -- maybe I'm talking about that class? You don't know because you never asked. So should my domain service have a dependency on HttpClient? No, of course not. It'd be abstracted to some other sort of abstract service and I'd mock that instead. Ok, so in the layer that injects whatever abstraction I come up with -- somewhere up the chain, something is going to know about HttpClient and that something is going to be wrapped by something else that I write, and that something else is something I'm still going to write a unit test for.

So, if my unit test for some kind of mediator is testing that a non-successful call to a service out on the internet results in my code rolling back some form of transaction vs. a successful call to a service resulting in the committing of that transaction, then I need to have control over the result of that call in order to set the conditions of that test so that I can assert that both scenarios are behaving correctly.

I could understand the opinion of some that HttpClient is not the correct level of abstraction, however I don't think anyone asked about the "level" of the consuming class, which is kind of presumptive. One could abstract the HTTP call to a service and interface that, but some of us still want to test that our "wrappers" for that HTTP call are behaving as expected, which means that we need control over the response. Currently, this can be achieved by giving HttpClient a fake handler which is what I'm doing now, and that's fine, but the point is that if you want that coverage, you're going to have to futz with HttpClient at some point to at least fake the response.

But, now let me backpedal. The more I look at it, it's far too fat for a mockable interface (which has been said). I think it may be a mental thing stemming from the name "Client". I'm not saying that I think it's wrong, but rather, it's a bit out of alignment with other "Client" implementations that one might see out there in the wild these days. We see a lot of "clients" out there today that are really service facades, so when one sees the name Http CLIENT, they may think the same thing and why would you not mock a service, right?

I see now that the handler is the part that matters to the code, so I'll be designing around that going forward.

dasjestyr commented Dec 9, 2016

This is basically not following Unit Testing 101 -- getting unrelated dependency code out of your tests =).

Personally, I don't think that's the right interpretation of that particular practice. We're not trying to test the dependency, we're trying to test how our code interacts with that dependency, thus it is not unrelated. And, quite simply, If I can't run a unit test against my code without invoking the actual dependency, then that's not testing in isolation. For example, if that unit test was run on a build server without internet access, then the test would likely break -- hence the desire to mock it. You could remove the dependency on HttpClient, but that just means it's going to end up in some other class that that you write which makes the call -- maybe I'm talking about that class? You don't know because you never asked. So should my domain service have a dependency on HttpClient? No, of course not. It'd be abstracted to some other sort of abstract service and I'd mock that instead. Ok, so in the layer that injects whatever abstraction I come up with -- somewhere up the chain, something is going to know about HttpClient and that something is going to be wrapped by something else that I write, and that something else is something I'm still going to write a unit test for.

So, if my unit test for some kind of mediator is testing that a non-successful call to a service out on the internet results in my code rolling back some form of transaction vs. a successful call to a service resulting in the committing of that transaction, then I need to have control over the result of that call in order to set the conditions of that test so that I can assert that both scenarios are behaving correctly.

I could understand the opinion of some that HttpClient is not the correct level of abstraction, however I don't think anyone asked about the "level" of the consuming class, which is kind of presumptive. One could abstract the HTTP call to a service and interface that, but some of us still want to test that our "wrappers" for that HTTP call are behaving as expected, which means that we need control over the response. Currently, this can be achieved by giving HttpClient a fake handler which is what I'm doing now, and that's fine, but the point is that if you want that coverage, you're going to have to futz with HttpClient at some point to at least fake the response.

But, now let me backpedal. The more I look at it, it's far too fat for a mockable interface (which has been said). I think it may be a mental thing stemming from the name "Client". I'm not saying that I think it's wrong, but rather, it's a bit out of alignment with other "Client" implementations that one might see out there in the wild these days. We see a lot of "clients" out there today that are really service facades, so when one sees the name Http CLIENT, they may think the same thing and why would you not mock a service, right?

I see now that the handler is the part that matters to the code, so I'll be designing around that going forward.

@dasjestyr

This comment has been minimized.

Show comment
Hide comment
@dasjestyr

dasjestyr Dec 9, 2016

Basically, I think maybe our desire to mock HttpClient is in a way related to the average developer's propensity to spin up new instances of HttpClient instead of reusing them which is terrible practice. In other words, we underestimate what HttpClient is and should focus on the handler instead. Also, the handler is abstract, so it can be easily mocked.

Ok, I'm sold. I no longer want the interface on HttpClient for mocking.

dasjestyr commented Dec 9, 2016

Basically, I think maybe our desire to mock HttpClient is in a way related to the average developer's propensity to spin up new instances of HttpClient instead of reusing them which is terrible practice. In other words, we underestimate what HttpClient is and should focus on the handler instead. Also, the handler is abstract, so it can be easily mocked.

Ok, I'm sold. I no longer want the interface on HttpClient for mocking.

@ppumkin

This comment has been minimized.

Show comment
Hide comment
@ppumkin

ppumkin Feb 15, 2017

After struggling for way to long I made my own IHttpHandler Interface and implemented everything for my use case. Makes my concrete code easier to read and tests can be mocked out without some creeazy ass stuff.

ppumkin commented Feb 15, 2017

After struggling for way to long I made my own IHttpHandler Interface and implemented everything for my use case. Makes my concrete code easier to read and tests can be mocked out without some creeazy ass stuff.

@jnm2

This comment has been minimized.

Show comment
Hide comment
Collaborator

jnm2 commented Feb 20, 2017

@abatishchev

This comment has been minimized.

Show comment
Hide comment
@abatishchev

abatishchev Apr 8, 2017

It's near impossible to share an instance of HttpClient imn any really application as soon as you need to send different HTTP header on each request (what is crucial when communicating with properly designed RESTful web services). Currently HttpRequestHeaders DefaultRequestHeaders is bound to the instance and not to a call on it effectively making it stateful. Instead {Method}Async() should accept it what would make HttpClient stateless and really reusable.

It's near impossible to share an instance of HttpClient imn any really application as soon as you need to send different HTTP header on each request (what is crucial when communicating with properly designed RESTful web services). Currently HttpRequestHeaders DefaultRequestHeaders is bound to the instance and not to a call on it effectively making it stateful. Instead {Method}Async() should accept it what would make HttpClient stateless and really reusable.

@richardszalay

This comment has been minimized.

Show comment
Hide comment
@richardszalay

richardszalay Apr 8, 2017

@abatishchev But you can specify headers on each HttpRequestMessage.

@abatishchev But you can specify headers on each HttpRequestMessage.

@abatishchev

This comment has been minimized.

Show comment
Hide comment
@abatishchev

abatishchev Apr 8, 2017

@richardszalay I don't say it's completely impossible, I say that HttpClient wasn't designed well for this purpose. None of {Method}Async() accept HttpRequestMethod, only SendAsync() does. But what's the purpose of the rest then?

abatishchev commented Apr 8, 2017

@richardszalay I don't say it's completely impossible, I say that HttpClient wasn't designed well for this purpose. None of {Method}Async() accept HttpRequestMethod, only SendAsync() does. But what's the purpose of the rest then?

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Apr 8, 2017

Collaborator

But what's the purpose of the rest then?

Meeting 99% of the needs.

Collaborator

jnm2 commented Apr 8, 2017

But what's the purpose of the rest then?

Meeting 99% of the needs.

@abatishchev

This comment has been minimized.

Show comment
Hide comment
@abatishchev

abatishchev Apr 8, 2017

Does it mean setting headers is 1% of use cases? I doubt.

Either way this won't be an issue if those methods鈥 had an overload accepting HttpResponseMessage.

Does it mean setting headers is 1% of use cases? I doubt.

Either way this won't be an issue if those methods鈥 had an overload accepting HttpResponseMessage.

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Apr 8, 2017

Collaborator

@abatishchev I don't doubt it, but either way, I'd write extension methods if I found myself in your scenario.

Collaborator

jnm2 commented Apr 8, 2017

@abatishchev I don't doubt it, but either way, I'd write extension methods if I found myself in your scenario.

@dasjestyr

This comment has been minimized.

Show comment
Hide comment
@dasjestyr

dasjestyr Apr 11, 2017

I toyed with the idea of maybe interfacing HttpMessageHandler and possibly HttpRequestMessage because I didn't like having to write fakes (vs. mocks). But the further down that rabbit hole you go, the more you realize that you'll be trying to fake actual data value objects (e.g. HttpContent) which is a futile exercise. So I think that designing your dependent classes to optionally take HttpMessageHandler as a ctor argument and using a fake for unit tests is the most appropriate route. I'd even argue that wrapping HttpClient is also a waste of time...

This will allow you to unit test your dependent class without actually making a call to the internet, which is what you want. And your fake can return pre-determined status codes and content so that you can test that your dependent code is processing them as expected, which is again, what you actually want.

dasjestyr commented Apr 11, 2017

I toyed with the idea of maybe interfacing HttpMessageHandler and possibly HttpRequestMessage because I didn't like having to write fakes (vs. mocks). But the further down that rabbit hole you go, the more you realize that you'll be trying to fake actual data value objects (e.g. HttpContent) which is a futile exercise. So I think that designing your dependent classes to optionally take HttpMessageHandler as a ctor argument and using a fake for unit tests is the most appropriate route. I'd even argue that wrapping HttpClient is also a waste of time...

This will allow you to unit test your dependent class without actually making a call to the internet, which is what you want. And your fake can return pre-determined status codes and content so that you can test that your dependent code is processing them as expected, which is again, what you actually want.

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome Apr 11, 2017

@dasjestyr Did you try creating an interface for HttpClient (which is like creating a wrapper for it) instead of interfaces for HttpMessageHandler or HttpRequestMessage ..

/me curious.

@dasjestyr Did you try creating an interface for HttpClient (which is like creating a wrapper for it) instead of interfaces for HttpMessageHandler or HttpRequestMessage ..

/me curious.

@dasjestyr

This comment has been minimized.

Show comment
Hide comment
@dasjestyr

dasjestyr Apr 11, 2017

@PureKrome I sketched out creating an interface for it and that's where I quickly realized that it was pointless. HttpClient really just abstracts a bunch of stuff that doesn't matter in the context of unit testing, and then calls the message handler (which was a point that was made a few times in this thread). I also tried creating a wrapper for it, and that was simply not worth the work required to implement it or propagate the practice (i.e. "yo, everyone do this instead of using HttpClient directly"). It's MUCH easier to simply focus on the handler, as it gives you everything you need and is literally comprised of a single method.

That said, I have created my own RestClient, but that solved a different problem which was providing a fluid request builder, but even that client accepts a message handler that can be used for unit testing or for implementing custom handlers that handle things like handler chains that solve cross-cutting concerns (e.g. logging, auth, retry logic, etc.) which is the way to go. That's not specific to my rest client, that's just a great use-case for setting the handler. I actually like the HttpClient interface in the Windows namespace much better for this reason, but I digress.

I think it could still be useful to interface the handler, however, but it would have to stop there. Your mocking framework can then be setup to return pre-determined instances of HttpResponseMessage.

dasjestyr commented Apr 11, 2017

@PureKrome I sketched out creating an interface for it and that's where I quickly realized that it was pointless. HttpClient really just abstracts a bunch of stuff that doesn't matter in the context of unit testing, and then calls the message handler (which was a point that was made a few times in this thread). I also tried creating a wrapper for it, and that was simply not worth the work required to implement it or propagate the practice (i.e. "yo, everyone do this instead of using HttpClient directly"). It's MUCH easier to simply focus on the handler, as it gives you everything you need and is literally comprised of a single method.

That said, I have created my own RestClient, but that solved a different problem which was providing a fluid request builder, but even that client accepts a message handler that can be used for unit testing or for implementing custom handlers that handle things like handler chains that solve cross-cutting concerns (e.g. logging, auth, retry logic, etc.) which is the way to go. That's not specific to my rest client, that's just a great use-case for setting the handler. I actually like the HttpClient interface in the Windows namespace much better for this reason, but I digress.

I think it could still be useful to interface the handler, however, but it would have to stop there. Your mocking framework can then be setup to return pre-determined instances of HttpResponseMessage.

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome Apr 11, 2017

Interesting. I've found (personal bias?) my helper library works great when using a (fake) concrete message handler .. vs.. some interface stuff on that lowish level.

I would still prefer not to have to write that library or use it, though :)

Interesting. I've found (personal bias?) my helper library works great when using a (fake) concrete message handler .. vs.. some interface stuff on that lowish level.

I would still prefer not to have to write that library or use it, though :)

@dasjestyr

This comment has been minimized.

Show comment
Hide comment
@dasjestyr

dasjestyr Apr 11, 2017

I don't see any problem with creating a small library to build fakes. I might do so when I'm bored with nothing else to do. All my http stuff is already abstracted and tested so I have no real use for it at the moment. I just don't see any value in wrapping the HttpClient for the purpose of unit testing. Faking the handler is all you really need. Extending functionality is a completely separate topic.

dasjestyr commented Apr 11, 2017

I don't see any problem with creating a small library to build fakes. I might do so when I'm bored with nothing else to do. All my http stuff is already abstracted and tested so I have no real use for it at the moment. I just don't see any value in wrapping the HttpClient for the purpose of unit testing. Faking the handler is all you really need. Extending functionality is a completely separate topic.

@abatishchev

This comment has been minimized.

Show comment
Hide comment
@abatishchev

abatishchev Apr 11, 2017

When the most of the codebase is tested using mocking interfaces, it's more convenient and consistent when the rest of the codebase is tested the same way. So I would like to see an interface IHttpClient. Like IFileSystemOperarions from ADL SDK or IDataFactoryManagementClient from ADF management SDK, etc.

When the most of the codebase is tested using mocking interfaces, it's more convenient and consistent when the rest of the codebase is tested the same way. So I would like to see an interface IHttpClient. Like IFileSystemOperarions from ADL SDK or IDataFactoryManagementClient from ADF management SDK, etc.

@dasjestyr

This comment has been minimized.

Show comment
Hide comment
@dasjestyr

dasjestyr Apr 11, 2017

I still think you're missing the point, which is HttpClient doesn't need to be mocked, only the handler. The real problem is the way that people look at HttpClient. It's not some random class that should be newed up whenever you think to call the internet. In fact, it's best practice to reuse the client across your entire application -- it's that big of a deal. Pull the two things apart, and it makes more sense.

Plus, your dependent classes shouldn't care about the HttpClient, only the data that gets returned by it -- which comes from the handler. Think of it this way: are you ever going to replace the HttpClient implementation with something else? Possible... but not likely. You never need to change the way the client works, so why bother abstracting it? The message handler is the variable. You'd want to change how the responses get handled, but not what the client does. Even the pipeline in WebAPI is focused on the handler (see: delegating handler). The more I say it, the more I start to think that .Net should make the client static and manage it for you... but I mean... whatever.

Remember what interfaces are for. They're not for testing -- it was just a clever way to leverage them. Creating interfaces solely for that purpose is ridiculous. Microsoft gave you what you need to decouple the message handling behavior, and it works perfectly for testing. Actually, HttpMesageHandler is abstract, so I think most mocking frameworks like Moq would still work with it.

dasjestyr commented Apr 11, 2017

I still think you're missing the point, which is HttpClient doesn't need to be mocked, only the handler. The real problem is the way that people look at HttpClient. It's not some random class that should be newed up whenever you think to call the internet. In fact, it's best practice to reuse the client across your entire application -- it's that big of a deal. Pull the two things apart, and it makes more sense.

Plus, your dependent classes shouldn't care about the HttpClient, only the data that gets returned by it -- which comes from the handler. Think of it this way: are you ever going to replace the HttpClient implementation with something else? Possible... but not likely. You never need to change the way the client works, so why bother abstracting it? The message handler is the variable. You'd want to change how the responses get handled, but not what the client does. Even the pipeline in WebAPI is focused on the handler (see: delegating handler). The more I say it, the more I start to think that .Net should make the client static and manage it for you... but I mean... whatever.

Remember what interfaces are for. They're not for testing -- it was just a clever way to leverage them. Creating interfaces solely for that purpose is ridiculous. Microsoft gave you what you need to decouple the message handling behavior, and it works perfectly for testing. Actually, HttpMesageHandler is abstract, so I think most mocking frameworks like Moq would still work with it.

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome Apr 11, 2017

Heh @dasjestyr - I too think you might have missed a major point of my discussion.

The fact that we (the developer) needs to learn so much about Message Handlers, etc .. to fake the response is my main point about all of this. Not so much about interfaces. Sure, I (personally) prefer interfaces to virtuals r wrappers with respect to testing (and therefore mocking) ... but those are implementation details.

I'm hoping the main gist of this epic thread is to highlight that .. when using HttpClient in an application, it's a PITA to test with it.

The status quo of "go learn the plumbing of HttpClient, which will lead you to HttpMessageHandler, etc" is a poor situation. We don't need to do this for many other libraries, etc.

So I was hoping something can be done to alleviate this PITA.

Yes, the PITA is an opinion - I'll totally admit to that. Some people don't think it's a PITA at all, etc.

The real problem is the way that people look at HttpClient. It's not some random class that should be newed up whenever you think to call the internet.

Agreed! But until fairly recently, this wasn't well known - which might lead to some lack of documentation or teaching or something.

Plus, your dependent classes shouldn't care about the HttpClient, only the data that gets returned by it

Yep - agreed.

which comes from the handler.

a what? huh? Oh -- now u're asking me to open up the hood and learn about the plumbing? ... Refer to above again and again.

Think of it this way: are you ever going to replace the HttpClient implementation with something else?

No.

.. etc ..

Now, I'm not meaning to troll, etc. So please don't think that I'm trying to be a jerk by always replying and repeating the same stuff over and over again.

I've been using my helper library for ages now which is just a glorified way of using a custom message handler. Great! It works and works great. I just think that this could be all exposed a bit nicer ... and that's what I'm hoping this thread really hits home at.

EDIT: formatting.

PureKrome commented Apr 11, 2017

Heh @dasjestyr - I too think you might have missed a major point of my discussion.

The fact that we (the developer) needs to learn so much about Message Handlers, etc .. to fake the response is my main point about all of this. Not so much about interfaces. Sure, I (personally) prefer interfaces to virtuals r wrappers with respect to testing (and therefore mocking) ... but those are implementation details.

I'm hoping the main gist of this epic thread is to highlight that .. when using HttpClient in an application, it's a PITA to test with it.

The status quo of "go learn the plumbing of HttpClient, which will lead you to HttpMessageHandler, etc" is a poor situation. We don't need to do this for many other libraries, etc.

So I was hoping something can be done to alleviate this PITA.

Yes, the PITA is an opinion - I'll totally admit to that. Some people don't think it's a PITA at all, etc.

The real problem is the way that people look at HttpClient. It's not some random class that should be newed up whenever you think to call the internet.

Agreed! But until fairly recently, this wasn't well known - which might lead to some lack of documentation or teaching or something.

Plus, your dependent classes shouldn't care about the HttpClient, only the data that gets returned by it

Yep - agreed.

which comes from the handler.

a what? huh? Oh -- now u're asking me to open up the hood and learn about the plumbing? ... Refer to above again and again.

Think of it this way: are you ever going to replace the HttpClient implementation with something else?

No.

.. etc ..

Now, I'm not meaning to troll, etc. So please don't think that I'm trying to be a jerk by always replying and repeating the same stuff over and over again.

I've been using my helper library for ages now which is just a glorified way of using a custom message handler. Great! It works and works great. I just think that this could be all exposed a bit nicer ... and that's what I'm hoping this thread really hits home at.

EDIT: formatting.

@richardszalay

This comment has been minimized.

Show comment
Hide comment
@richardszalay

richardszalay Apr 11, 2017

(I've only just noticed the grammar error in this issue's title and now I can't unsee it)

(I've only just noticed the grammar error in this issue's title and now I can't unsee it)

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome Apr 11, 2017

And that has been my real long game :)

Q.E.D.

(actually - so embarrassing 馃槉 )

EDIT: part of me doesn't want to edit it .. for nostalgia....

PureKrome commented Apr 11, 2017

And that has been my real long game :)

Q.E.D.

(actually - so embarrassing 馃槉 )

EDIT: part of me doesn't want to edit it .. for nostalgia....

@jnm2

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Apr 11, 2017

Collaborator

(I've only just noticed the grammar error in this issue's title and now I can't unsee it)

I know! Same!

Collaborator

jnm2 commented Apr 11, 2017

(I've only just noticed the grammar error in this issue's title and now I can't unsee it)

I know! Same!

@dasjestyr

This comment has been minimized.

Show comment
Hide comment
@dasjestyr

dasjestyr Apr 12, 2017

The fact that we (the developer) needs to learn so much about Message Handlers, etc .. to fake the response is my main point about all of this. Not so much about interfaces. Sure, I (personally) prefer interfaces to virtuals r wrappers with respect to testing (and therefore mocking) ... but those are implementation details.

wut?

Have you even looked at HttpMessageHandler? It's an abstract class that is literally a single method that takes an HttpRequestMessage and returns a HttpResponseMessage -- made to intercept behavior separate of all the low level transport junk which is exactly what you'd want in a unit test. To fake it, just implement it. The message that goes in is the one that you sent HttpClient, and the response that comes out is up to you. For example, if I want to know that my code is dealing with a json body correctly, then just have the response return a json body that you expect. If you want to see if it's handling 404 correctly, then have it return a 404. It doesn't get more basic than that. To use the handler, just send it in as a ctor argument for HttpClient. You don't need to pull any wires out or learn the internal workings of anything.

I'm hoping the main gist of this epic thread is to highlight that .. when using HttpClient in an application, it's a PITA to test with it.

And the main gist of what many people have pointed out is that you're doing it wrong (which is why it's a PITA, and I say this directly but respectfully) and demanding that HttpClient be interfaced for testing purposes is akin to creating features to compensate for bugs instead of addressing the root problem which is, in this case, that you're operating at the wrong level.

I think that HttpClient in the windows namespace actually did separate the concept of handler into filter, but it does the exact same thing. I think that handler/filter in that implementation is actually interfaced though which is kinda what I was suggesting earlier.

dasjestyr commented Apr 12, 2017

The fact that we (the developer) needs to learn so much about Message Handlers, etc .. to fake the response is my main point about all of this. Not so much about interfaces. Sure, I (personally) prefer interfaces to virtuals r wrappers with respect to testing (and therefore mocking) ... but those are implementation details.

wut?

Have you even looked at HttpMessageHandler? It's an abstract class that is literally a single method that takes an HttpRequestMessage and returns a HttpResponseMessage -- made to intercept behavior separate of all the low level transport junk which is exactly what you'd want in a unit test. To fake it, just implement it. The message that goes in is the one that you sent HttpClient, and the response that comes out is up to you. For example, if I want to know that my code is dealing with a json body correctly, then just have the response return a json body that you expect. If you want to see if it's handling 404 correctly, then have it return a 404. It doesn't get more basic than that. To use the handler, just send it in as a ctor argument for HttpClient. You don't need to pull any wires out or learn the internal workings of anything.

I'm hoping the main gist of this epic thread is to highlight that .. when using HttpClient in an application, it's a PITA to test with it.

And the main gist of what many people have pointed out is that you're doing it wrong (which is why it's a PITA, and I say this directly but respectfully) and demanding that HttpClient be interfaced for testing purposes is akin to creating features to compensate for bugs instead of addressing the root problem which is, in this case, that you're operating at the wrong level.

I think that HttpClient in the windows namespace actually did separate the concept of handler into filter, but it does the exact same thing. I think that handler/filter in that implementation is actually interfaced though which is kinda what I was suggesting earlier.

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome Apr 12, 2017

wut?
Have you even looked at HttpMessageHandler?

Initially no, because of this:

var payloadData = await _httpClient.GetStringAsync("https://......");

meaning, the exposed methods on HttpClient are really nice and make things really easy :) Yay! Fast, simple, works, WIN!

Ok - so now lets use it in a test ... and now we need to spend time learning what happens underneath .. which leads us to learning about HttpMessageHandler etc.

Again I keep saying this => this extra learning about the plumbing of HttpClient (i.e. HMH, etc) is a PITA. Once you have done that learning .. yes, I then know how to use it etc ... even though I also don't like to keep using it but need to, in order to mock remote calls to 3rd party endpoints. Sure, that's opinionated. We can only agree to disagree. So please - I know about HMH and how to use it.

And the main gist of what many people have pointed out is that you're doing it wrong

Yep - people disagree with me. No probs. Also - people agree me too. Again, no probs.

and demanding that HttpClient be interfaced for testing purposes is akin to creating features to compensate for bugs instead of addressing the root problem which is, in this case, that you're operating at the wrong level.

Ok - I respectfully disagree with you here (which is ok). Again, different opinions.

But srsly. This thread has gone on for so long. I've really moved on by now. I said my piece, some people say YES! some said NO! Nothing has changed and I've just ... accepted the status quo and rolled with everything.

I'm sorry you just don't accept my train of thought. I'm not a bad person and try not sound rude or mean. This was just me expressing how I felt while developing and how I believe others also felt. That's all.

wut?
Have you even looked at HttpMessageHandler?

Initially no, because of this:

var payloadData = await _httpClient.GetStringAsync("https://......");

meaning, the exposed methods on HttpClient are really nice and make things really easy :) Yay! Fast, simple, works, WIN!

Ok - so now lets use it in a test ... and now we need to spend time learning what happens underneath .. which leads us to learning about HttpMessageHandler etc.

Again I keep saying this => this extra learning about the plumbing of HttpClient (i.e. HMH, etc) is a PITA. Once you have done that learning .. yes, I then know how to use it etc ... even though I also don't like to keep using it but need to, in order to mock remote calls to 3rd party endpoints. Sure, that's opinionated. We can only agree to disagree. So please - I know about HMH and how to use it.

And the main gist of what many people have pointed out is that you're doing it wrong

Yep - people disagree with me. No probs. Also - people agree me too. Again, no probs.

and demanding that HttpClient be interfaced for testing purposes is akin to creating features to compensate for bugs instead of addressing the root problem which is, in this case, that you're operating at the wrong level.

Ok - I respectfully disagree with you here (which is ok). Again, different opinions.

But srsly. This thread has gone on for so long. I've really moved on by now. I said my piece, some people say YES! some said NO! Nothing has changed and I've just ... accepted the status quo and rolled with everything.

I'm sorry you just don't accept my train of thought. I'm not a bad person and try not sound rude or mean. This was just me expressing how I felt while developing and how I believe others also felt. That's all.

@dasjestyr

This comment has been minimized.

Show comment
Hide comment
@dasjestyr

dasjestyr Apr 13, 2017

I'm not at all offended. I originally hopped on this thread with the same opinion that the client should be mockable, but then some really good points were made about what HttpClient is so I sat and really thought about it, and then tried to challenge it on my own and eventually I came to the same conclusion which is that HttpClient is the wrong thing to mock and trying to do so is futile exercise that causes far more work than it's worth. Accepting that made life much easier. So I too have moved on. I'm just hoping that others will eventually give themselves a break and take it for what it is.

I'm not at all offended. I originally hopped on this thread with the same opinion that the client should be mockable, but then some really good points were made about what HttpClient is so I sat and really thought about it, and then tried to challenge it on my own and eventually I came to the same conclusion which is that HttpClient is the wrong thing to mock and trying to do so is futile exercise that causes far more work than it's worth. Accepting that made life much easier. So I too have moved on. I'm just hoping that others will eventually give themselves a break and take it for what it is.

@dasjestyr

This comment has been minimized.

Show comment
Hide comment
@dasjestyr

dasjestyr Apr 13, 2017

As an aside:

Initially no, because of this:

var payloadData = await _httpClient.GetStringAsync("https://......");

meaning, the exposed methods on HttpClient are really nice and make things really easy :) Yay! Fast, >simple, works, WIN!

I'd argue that in terms of SOLID, this interface is inappropriate anyway. IMO client, request, response are 3 different responsibilities. I can appreciate the convenience methods on the client, but it promotes tight coupling by combining requests and responses with the client, but that's personal preference. I have some extensions to HttpResponseMessage that accomplish the same thing but keep the responsibility of reading the response with the response message. In my experience, with large projects, "Simple" is never "Simple" and almost always ends in a BBOM. This however, is a completely different discussion :)

As an aside:

Initially no, because of this:

var payloadData = await _httpClient.GetStringAsync("https://......");

meaning, the exposed methods on HttpClient are really nice and make things really easy :) Yay! Fast, >simple, works, WIN!

I'd argue that in terms of SOLID, this interface is inappropriate anyway. IMO client, request, response are 3 different responsibilities. I can appreciate the convenience methods on the client, but it promotes tight coupling by combining requests and responses with the client, but that's personal preference. I have some extensions to HttpResponseMessage that accomplish the same thing but keep the responsibility of reading the response with the response message. In my experience, with large projects, "Simple" is never "Simple" and almost always ends in a BBOM. This however, is a completely different discussion :)

@davidsh

This comment has been minimized.

Show comment
Hide comment
@davidsh

davidsh Jul 10, 2017

Member

Now that we have a new repo specifically for design discussions, please continue the discussion in dotnet/designs#9 or open a new issue in https://github.com/dotnet/designs

Thanks.

Member

davidsh commented Jul 10, 2017

Now that we have a new repo specifically for design discussions, please continue the discussion in dotnet/designs#9 or open a new issue in https://github.com/dotnet/designs

Thanks.

@davidsh davidsh closed this Jul 10, 2017

@davidsh davidsh removed this from the Future milestone Jul 10, 2017

@AchotStepanian

This comment has been minimized.

Show comment
Hide comment
@AchotStepanian

AchotStepanian Jul 11, 2017

Maybe it could be considered below solution for testing purpose only:

public class GeoLocationServiceForTest : GeoLocationService, IGeoLocationService
{
public GeoLocationServiceForTest(something: something, HttpClient httpClient) : base(something)
{
base._httpClient = httpClient;
}
}

Maybe it could be considered below solution for testing purpose only:

public class GeoLocationServiceForTest : GeoLocationService, IGeoLocationService
{
public GeoLocationServiceForTest(something: something, HttpClient httpClient) : base(something)
{
base._httpClient = httpClient;
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment