Autorest: AutoRest and integration testing (Microsoft.AspNetCore.TestHost.TestServer)

Created on 22 Dec 2017 · 9Comments · Source: Azure/autorest

To connect to the test server, you need to use HttpClient created by the CreateClient method. However, it's not currently possible to initialize the generated API with HttpClient.

The base class - ServiceClient - provides a protected constructor accepting HttpClient, but it's not exposed in the generated API.

Source

votrubac

Most helpful comment

I'm very surprised to see @olydis write this:

reusing clients is not a common scenario for customers

If you look at the docs for HttpClient, you will find this:

HttpClient is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads

So the thing you have described as "not a common scenario" is the very thing that the HttpClient documentation says you are supposed to do.

It's understandable that in some scenarios sharing may not be possible because there is a requirement for differently-configured handlers or settings. But that sounds more like a fundamental design error in AutoRest. This is an important scenario because failure to use a shared HttpClient violates its documented usage model. Therefore AutoRest should make this possible; the fact that it is designed in a way that makes it hard or sometimes even impossible is therefore a design bug. It seems like it should be AutoRest's job (or failing that, the job of any client SDK that uses AutoRest) to provide the necessary clarity over who disposes what.

idg10 on 22 May 2018

👍6

All 9 comments

the generated client classes are partial, so you could provide such constructor yourself, right? 🙂
Maybe I'm missing a detail, but that's the way I think I've seen testing happen in the past.

olydis on 22 Dec 2017

@olydis yes, I added a new constructor in a custom partial implementation; thanks for confirming that this is as expected/designed.

I am wondering what is a downside of including such constructor into the template. It sounds like a common use case when you need to re-use HttpClient, and have a test harness to make sure your generated API is up-to-date.

votrubac on 27 Dec 2017

I'll gladly discuss this with the others but I assume that the idea was to focus on user (=customer) experience:
1) HttpClients are initialized with custom delegating handlers and settings, so passing custom clients isn't guaranteed to work (so let's not expose a constructor that may yield an unusable client)
2) reusing clients is not a common scenario for customers and often not even possible (due to different delegating handlers/"HTTP pipelines" required for different SDKs), also unclear who disposes what, etc.

For testing, what you say of course makes perfect sense, but compared to the decreased UX, adding that constructor doesn't add much. At the same time, extending the class as you did will keep on working with future auto-generated clients, i.e. should be a one-time cost per client. 🙂

olydis on 29 Dec 2017

👎1

@olydis Thanks for the explanations. It's good to know that your decisions are customer-focused. Few observations:

It is a one-time cost to create a partial class, but figuring it out is time-consuming if you are new to AutoRest. I could not find any documentation/guidance, and we were about to go with an alternative.
I think that the crux of the problem is that HttpClient does not expose it's HttpClientHandler, and TestServer can only give you HttpClient with a preset handler.
I think that dealing with client and delegating handlers is quite an advanced use case. By the same token, you should not expose them by the default (one-time cost to do it in a partial class). Presence of constructors accepting these classes and absence of a "simple" HttpClient was confusing to me as I was onboarding.
NSwag gives you the option to provide either Uri or HttpClient in the constructor. It's assumed that the caller would set all needed handlers for the provided HTTP client.

votrubac on 3 Jan 2018

👍2

I'm very surprised to see @olydis write this:

reusing clients is not a common scenario for customers

If you look at the docs for HttpClient, you will find this:

HttpClient is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads

So the thing you have described as "not a common scenario" is the very thing that the HttpClient documentation says you are supposed to do.

idg10 on 22 May 2018

👍6

Well, we abandoned AutoRest for this reason and several more. Our whole org migrated to NSwag.

votrubac on 22 May 2018

🚀1

I'm not a big fan of base classes for this sort of thing in the first place.

The next generation of the C# generator removes all of these limitations; The HttpClient will only ever get instantiated by the client if the end user doesn't provide one.

fearthecowboy on 22 May 2018

Using the latest autorest utility 2.0.4283.

There is now a new constructor signature where you can inject httpclient and also boolean value to not dispose the injected httpclient. Looks like this issue is fixed.