dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.18k stars 4.72k forks source link

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

Closed PureKrome closed 4 years ago

PureKrome commented 9 years ago

System.Net.Http has now been uploaded to the repo :smile: :tada: :balloon:

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?

dasjestyr commented 7 years ago

Well I mean fakes take a lot of extra work to set up and in many cases result in you writing a whole other class just for the test that doesn't really add a lot of value. Stubs are great because it'll allow the test to continue without a real implementation. Mocks are what I'm really interested because they can be used for assertions. For example, assert that the method on this interface's implementation was called x amount of times with this test's current setup.

I actually had to test that on one of my classes. To do it, I had to add an accumulator to the fake handler that incremented each time the SendAsync() method was called. That was a simple case, thankfully, but come on. Mocking is practically a standard at this point.

PureKrome commented 7 years ago

Yep - totally agree with you here πŸ‘ 🍰

lasseschou commented 7 years ago

Please, Microsoft, add the IHttpClient, period. OR be consistent and remove all your interfaces in the BCL - IList, IDictionary, ICollection, because they are "hard to version" anyway. Heck, also get rid of IEnumerable.

On a serious note, instead of arguing like "you can just write your own handler" (sure, but an interface is easier and cleaner) and "an interface introduces breaking change" (well, you do that in all .net versions anyway), simply ADD THE INTERFACE, and let developers decide how they want to test it.

jnm2 commented 7 years ago

@lasseschou IList, IDictionary, ICollection and IEnumerable are absolutely critical because of the wide variety of classes that implement them. IHttpClient would only be implemented by HttpClient- to be consistent with your logic, every single class in the BCL would have to be given an interface type because we might need to mock it. I would be sad to see this kind of clutter.

Unless there is a dramatic tradeoff, IHttpClient is the wrong abstraction level to be mocking anyhow. Your app will almost certainly not be exercising the full range of HttpClient's capabilities. I would suggest that you wrap HttpClient in your own service in terms of your app's specific requests and mock your own service.

PureKrome commented 7 years ago

@jnm2 - Out of interest, why do you think an IHttpClient is the wrong abstraction level to be mocking?

I'm not hating or trolling - honest Q.

(Like to learn, etc).

dasjestyr commented 7 years ago

I was gonna ask the same thing considering its pretty much a perfect level of abstraction seeing as it is a client.

jnm2 commented 7 years ago

It's not an absolute. If you're building a web browser, then I could see how it's 1:1 between what your app needs and what HttpClient abstracts. But if you're using HttpClient to talk to an API of some sort- anything with more specific expectations on top of HTTP- you're only going to need a fraction of what HttpClient can do. If you build a service which encapsulates the HttpClient transport mechanism and mock that service, the rest of your app can code against your service's API-specific interface abstraction, rather than coding against HttpClient.

Also, I subscribe to the "Never mock a type you don't own" principle, so YMMV. See also http://martinfowler.com/articles/mocksArentStubs.html comparing classical testing vs mocking.

lasseschou commented 7 years ago

The point here is that the HttpClient is such a great advancement over WebClient, XMLHttpRequest, that it was become the preferred way to do HTTP calls. It's clean and modern. I always use mockable adapters that talk directly to the HttpClient, so testing my code is easy. But I'd like to test those adapter classes as well. And that's when I discover: there's no IHttpClient. Perhaps you're right, it's not needed. Perhaps you think it's the wrong level of abstraction. But it would be really convenient not having to wrap it in yet another class in all projects. I just don't see any reason not to have a separate interface for such an important class.

Lasse Schou Founder & CEO

http://mouseflow.com lasseschou@mouseflow.com

https://www.facebook.com/mouseflowdotcom/ https://twitter.com/mouseflow https://www.linkedin.com/company/mouseflow

CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and may contain confidential, proprietary, and/or privileged information protected by law. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.

On 12 November 2016 at 14:57, Joseph Musser notifications@github.com wrote:

It's not an absolute. If you're building a web browser, then I could see how it's 1:1 between what your app needs and what HttpClient abstracts. But if you're using HttpClient to talk to an API of some sort- anything with more specific expectations on top of HTTP, you're only going to need a fraction of what HttpClient can do. If you build a service which encapsulates the HttpClient transport mechanism and mock that service, the rest of your app can code against your service's API-specific interface abstraction, rather than coding against HttpClient.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/corefx/issues/1624#issuecomment-260123552, or mute the thread https://github.com/notifications/unsubscribe-auth/ACnGPp3fSriEJCAjXeAqMZbBcTWcRm9Rks5q9cXlgaJpZM4EPBHy .

brphelps commented 7 years ago

I'm hearing some weird input that the HttpClient is "too flexible" to be the right layer to mock -- That seems more like a side effect of the decision to have one class have so many responsibilities and exposed features. But it's really independent of how people want to be testing the code.

I'm still really hopeful we fix this, if only because we're breaking encapsulation by expecting consumers to know these details of how HttpClient is implemented (In this case, to "know" that it's a wrapper that doesn't add meaningful functionality and wraps another class which they may or may not be using). Framework consumers shouldn't have to know anything about the implementation than exactly what interface is exposed to them, and a lot of the rationalization here is that we have a workaround and that deep familiarity with the class offers many not-well-encapsulated ways of mocking / stubbing functionality.

Fix please!

damianh commented 7 years ago

Just stumbled upon this so throwing my 2c in...

I think mocking HttpClient is folly.

Even as there is just a single SendAsync to mock, there are so many nuances to HttpResponseMessage, HttpResponseHeaders, HttpContentHeaders, etc, your mocks are going to get messy fast and are probably behaving wrong too.

Me? I use OwinHttpMessageHandler to

  1. Perform HTTP acceptance tests from the outside of a service.
  2. Inject Dummy HTTP services into components that want to make outbound HTTP requests to said services. That is, my component has an optional ctor parameter for HttpMessageHandler.

I'm sure there is an MVC6 handler that does similar....

Life is fine.

brphelps commented 7 years ago

@damianh: Why should people using dotnet core need to do anything with Owin to test a class like HttpClient? This might make a lot of sense for a particular use case (AspCore hosted on OWIN) but this doesn't seem to be that general.

This really seems to be conflating the concern of "what's the most general way to Unit Test HttpClient that is also consistent with user expectations" with "how do I integration test" :).

damianh commented 7 years ago

Oh testing HttpClient... thought the topic was testing things that have a dependency on HttpClient. Don't mind me. :)

On 5 Dec 2016 7:56 p.m., "brphelps" notifications@github.com wrote:

@damianh https://github.com/damianh: Why should people using dotnet core need to do anything with Owin to test a class like HttpClient? This might make a lot of sense for a particular use case (AspCore hosted on OWIN) but this doesn't seem to be that general.

This really seems to be conflating the concern of "what's the most general way to Unit Test HttpClient that is also consistent with user expectations" with "how do I integration test" :).

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/corefx/issues/1624#issuecomment-264942511, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgXJxunxBgFGLozOsisCoPqjMoch48ks5rFF5vgaJpZM4EPBHy .

brphelps commented 7 years ago

@damianh -- It is things that have a dependency, but your solution seems to involve a lot of other dependencies in the picture, too. I'm imagining a class library that happens to use a passed in HttpClient instance (or a DI'ed one)-- Where does Owin come into the picture?

damianh commented 7 years ago

It's not owin that is the interesting thing I was trying to show (which will also work on .net core / netstandard) so please don't focus on that.

The interesting thing is the HttpMessageHandler that make an in-process in-memory HTTP request against dummy HTTP endpoints.

Aspnet Core's TestServer, as did the katana one before it, works the same way.

So your class under test has an HttpClient DI'd where it has an HttpMessageHandler injected into it that directly. The testable and assertable surface is there.

Mocking, a specific form of test double, is something I'd still advise against for HttpClient. And thus, no need for an IHttpClient.

On 5 Dec 2016 8:15 p.m., "brphelps" notifications@github.com wrote:

@damianh https://github.com/damianh -- It is things that have a dependency, but your solution seems to involve a lot of other dependencies in the picture, too. I'm imagining a class library that happens to use a passed in HttpClient instance (or a DI'ed one)-- Where does Owin come into the picture?

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/corefx/issues/1624#issuecomment-264947538, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgXEESfQj9NnKLo8LucFLyajWtQXKBks5rFGK8gaJpZM4EPBHy .

brphelps commented 7 years ago

@damianh -- Totally understand what you're saying the interesting thing is -- but people don't need dummy HTTP endpoints with a smart mock-- e.g. @richardszalay's MockHttp library.

At least not for unit testing :).

damianh commented 7 years ago

It depends. Options are good :)

On 5 Dec 2016 8:39 p.m., "brphelps" notifications@github.com wrote:

@damianh https://github.com/damianh -- Totally understand what you're saying the interesting thing is -- but people don't need dummy HTTP endpoints with a smart mock-- e.g. @richardszalay https://github.com/richardszalay's MockHttp library.

At least not for unit testing :).

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/corefx/issues/1624#issuecomment-264954210, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgXOl4UEDGYCVLpbvwhueaK52VtjH6ks5rFGhlgaJpZM4EPBHy .

PureKrome commented 7 years ago

@damianh said: Options are good :)

Yep - can totally agree on that πŸ‘ Which, IMO .. we don't have right now 😒 **

I totally understand the rational for creating and injecting your own HMH. That's what I'm doing right now. Unfortunately, i have to create a 2nd ctor (or provide an optional ctor param) for unit testing concerns. Just think about that sentence again -> i'm creating an entirely possible USER OPTION ... which is really only to enable a unit test to do stuff. An end-user/consumer of my class will never really use that ctor option ... which to me smells.

Back to the point about options -> right now I don't feel like we have one. We have to learn about the internals of HttpClient. Literally open up the bonnet and look under the hood. (I was lucky enough to get a TL;DR; from Sir Fowler The Awesome ℒ️ which cut my research time right down).

So - with your point about being no-interface, can you please provide some examples of when an interface would be a bad thing, please? etc. Remember -> I've always been promoting an interface as an option for small/simple to medium examples ... not 100% of all examples.

I'm not trying to troll or attack you Damian - just trying to understand.

* Ninja Edit: technically, we actually do have options -> we can do nothing / create our own wrappers / etc. I was meaning: not many options that simplify things, out-of-the-box*.

damianh commented 7 years ago

Hey @PureKrome , no worries just sharing experience.

The term 'Unit Test' has been used multiple times and let's think about this for a minute. So we're on the same page, I'm assuming we have a FooClass that has a ctor dependency on HttpClient (or HMH) and we want to mock HttpClient.

What we are really saying here is "FooClass can make an HTTP request using to any URL (protocol / host / resource), using any method, any content type, any transfer-encoding, any content-encoding, any request headers, and can handle any response content-type, any status code, cookies, redirects, transfer encoding, cache control headers, etc as well as network timeouts, task cancelled exceptions etc.".

HttpClient is like a God Object; you can do a lot with it. HTTP is also a remote service call reaching out beyond your process.

Is this really unit testing? Be honest now :)

** Ninja Edit too: If you want to make FooClass unit testable where it wanted to get some json from a server, it would have a ctor dependency on Func<CancellationToken, Task<JObject>> or similar.

damianh commented 7 years ago

can you please provide some examples of when an interface would be a bad thing, please

Didn't say that! I said IHttpClient would not be useful because A) it's won't be substituted with an alternative concrete implementation and B) the God Object concern as mentioned.

Another example? IAssembly.

brphelps commented 7 years ago

@damianh -- The "God Class" status isn't a concern to callers. It's not the callers' responsibility to make sure HttpClient is / isn't a god class. They know that it's the object they use to make HTTP calls :).

So, assuming that we accept that the .NET framework exposed a God class to us... how can we reduce our concern? How can we best encapsulate it? That's easy -- Use a traditional mocking framework (Moq is a great example) and completely isolate the HttpClient implementation, because as people consuming it, we just don't care how it's internally designed. We only care about how we use it. The problem here is that the conventional approach doesn't work, and as a result, you see people coming up with more and more interesting solutions to solve the problem :).

Some edits for grammar and clarity :D

PureKrome commented 7 years ago

Totally agree @damianh that HttpClient is like some uber God Class.

I also don't see why I need to know about all the internal's again because there can be so many purmutations to what goes in, what comes out.

For example: I want to get the stocks from some API. https://api.stock-r-us.com/aapl. Result is some JSON paylod.

so, things i might screw up when trying to call that endpoint:

So I understand that if I want to test for these things, the HMH is a great way to do this because it just hijacks the real HMH and substitutes what I've said, to return.

But -- what about if I'm wanting (at my own risk) to ignore all of these things and just say:
Imagine I want to call and endpoint, everything is OK and I get my nice json payload result.
Should I still learn and worry about all of this stuff, to do something as simple as this?

Or ... is this a poor way of thinking. We should never be thinking like this because it too hacky? too cheap? too prone to errors?

Most of the time I just call an endpoint. I don't think about things like headers or cache-control or network errors. I usually just think about the VERB + URL (and PAYLOAD, if required) and then the RESULT. I have error handling around this to Catch-All-The-Things :tm: and then admit defeat because something Bad happened :(

So i'm either looking at the problem wrong (ie. i'm still can't shake my n00b status) -or- this God Class is causing us a heap of unit testing grief.

HTTP is also a remote service call reaching out beyond your process. Is this really unit testing? Be honest now :)

To me, HttpClient is a dependant service. I don't care what the service really does, I classify it as something that does some stuff which is external to my codebase. Therefore, I don't want it doing stuff with an external-power so I want to control that, hijack it and determine the results of those service-calls. I'm still trying to test my unit of work. My little piece of logic. Sure it might depend on other stuff. It's still a unit test if I can control the entire universe this piece of logic exists within and I don't have rely/integrate on other services.

** Another edit: Or maybe - if HttpClient is actually a God Object, maybe the conversation should be around reducing it to something ... better? Or maybe, it's not a God Object at all? (just trying to open the debate up)..

richardszalay commented 7 years ago

Unfortunately, i have to create a 2nd ctor (or provide an optional ctor param) for unit testing concerns. Just think about that sentence again -> i'm creating an entirely possible USER OPTION ... which is really only to enable a unit test to do stuff. An end-user/consumer of my class will never really use that ctor option ... which to me smells

I'm not sure what you're trying to say here:

"I don't like having a second ctor that accepts HttpMessageHandler" ... so have your tests wrap it in an HttpClient

"I want to unit test without inverting my dependency on HttpClient" ... tough?

brphelps commented 7 years ago

@richardszalay -- I think I get the sentiment that @PureKrome doesn't want to change his code in a way that doesn't improve design just to support an overly opinionated test strategy. Don't get me wrong, I'm not complaining about your library, the only reason I want a better solution is because I have a higher expectation of dotnet core to not require it =).

richardszalay commented 7 years ago

I'm not thinking of this in terms of MockHttp, but testing/mocking in general. Inverted dependencies is a fundamental necessity for unit testing in .NET, and that usually involves ctor-injection. I'm not clear on what the proposed alternative would be.

Surely if HttpClient implemented IHttpClient, there would still be a constructor that accepted it...

PureKrome commented 7 years ago

Surely if HttpClient implemented IHttpClient, there would still be a constructor that accepted it...

Agreed. I was "crossing streams" in my thinking as I literally just woke up and was struggling to get my single cell in my brain to come alive.

Ignore my 'ctor' comment plz and continue with the discussion, if anyone hasn't already left in boredom πŸ˜„

Thaoden commented 7 years ago

Can we take a step back to the question if using HttpClient as a dependency really can lead to Unit Testing.

Let's say I have a class FooClass that gets a dependency constructor injected. I now want to Unit Test the methods on FooClass, meaning I specify the input to my method, I isolate FooClass as best as I can from its environment and I check the outcome against an expected value. In all this pipeline, I wouldn't want to care about the entrails of my dependent object; it would be best for me if I could just specify the kind of environment that my FooClass should live in: I would want to mock the dependency on my system under test in that way that it behaves exactly as I need it to for my tests to work.

Now currently I have to care about the way HttpClient is implemented because I have to specify a dependency (HttpMessageHandler) for my injected dependency (HttpClient) for my system under test. Instead of just mocking out my direct dependency, I have to create a concrete implementation of it with a mocked chained dependency.

brphelps commented 7 years ago

Theme: I can't mock HttpClient without knowing how its internals work, and I also have to rely on its implementation to not do anything surprising since I can't completely bypass the internals of the class via mocking. This is basically not following Unit Testing 101 -- getting unrelated dependency code out of your tests =).

You can see the theme present in many different comments:

"To me, HttpClient is a dependant service. I don't care what the service really does, I classify it as something that does some stuff which is external to my codebase. " -- PureKrome

"Now currently I have to care about the way HttpClient is implemented because I have to specify a dependency (HttpMessageHandler) for my injected dependency (HttpClient) for my system under test. " -- Thaoden

"I was gonna ask the same thing considering its pretty much a perfect level of abstraction seeing as it is a client." -- dasjestyr

"The HttpMessageHandler is effectively the "implementation". You typically create an HttpClient, passing the handler into the constuctor, and then have your service layer depend on HttpClient." -- richardszalay

" But the key issue will remain - you will need to define a fake Handler and insert it below the HttpClient object." -- SidharthNabar

"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" -- ericstj

"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." -- KrzysztofCwalina

"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." -- ctolkien

dasjestyr commented 7 years ago

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 commented 7 years ago

HttpClient is the worst dependency ever. It literally states that the dependee can "call the Internet". Sure y'all be mocking TCP next. Heh

On 7 Dec 2016 5:34 a.m., "Jeremy Stafford" notifications@github.com wrote:

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. 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.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/corefx/issues/1624#issuecomment-265353526, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgXPL39vnUbWrUuUBtfmbjhk5IBkxHks5rFjdqgaJpZM4EPBHy .

dasjestyr commented 7 years ago

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 7 years ago

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.

p10tyr commented 7 years ago

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 commented 7 years ago

Came across this today: http://www.davesquared.net/2011/04/dont-mock-types-you-dont-own.html

abatishchev commented 7 years ago

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 commented 7 years ago

@abatishchev But you can specify headers on each HttpRequestMessage.

abatishchev commented 7 years ago

@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 commented 7 years ago

But what's the purpose of the rest then?

Meeting 99% of the needs.

abatishchev commented 7 years ago

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 commented 7 years ago

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

dasjestyr commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

PureKrome commented 7 years ago

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 commented 7 years ago

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

I know! Same!