Adding a way to initialize ServiceClient using provided HttpClient

[shahabhijeet]

1. Support for initializing ServiceClient using HttpClient
2. Added tests using customized clients
3. Added test for FakeClient
4. Fixed failing Operational Insights tests

[hovsepm]

using Microsoft.Rest.ClientRuntime.Tests.SvcClients;

License header? :)))

---

Refers to: src/ClientRuntime/Microsoft.Rest.ClientRuntime.Tests/SvcClientWithHttpClientTests.cs:1 in 2068ea9. [](commit_id = 2068ea9, deletion_comment = False)

[hovsepm]

using System;

License header? :)))

---

Refers to: src/ClientRuntime/Microsoft.Rest.ClientRuntime.Tests/SvcClients/ContosoServiceClient.cs:1 in 2068ea9. [](commit_id = 2068ea9, deletion_comment = False)

[hovsepm]

did you consider this issue? http://stackoverflow.com/questions/119506/virtual-member-call-in-a-constructor

[hovsepm]

please remove line breaks from 244 to 248

[cormacpayne]

@hovsepm any other comments?

[hovsepm]

License header?

[hovsepm]

For this one - I still consider it as a potential bug hole. We use Get and Set of the HttpClient property during ServiceClient constructor code. So we put all the responsibility on the developers -> they need to be careful and do not use any objects that are not yet initialized in the overwritten property.

1. if you walk over the call stack of ServiceClient ctr then HttpClient.Set method is called before any HttpClient.Get method call.
2. By overriding just Get you will lose whatever was Set in the base class constructor.
3. In your example in FabricamServiceClient class you completely ignore the HttpClient that was passed in the public FabricamServiceClient(HttpClient httpClient) constructor (because all your logic of initialization of HttpClient is happening in the HttpClient Get). Then why do you even expose a constructor that takes httpClient parameter?

[shahabhijeet]

@hovsepm
As mentioned in the description, there are two ways to solve this problem

1. Provide constructor overload with HttpClient parameter (more code churn, more paths to cover in tests)
2. Make the HttpClient virtual, so classes that inherit from ServiceClient can override their own HttpClient.

#2 The sole purpose of making it virtual is to allow derived classes to override whatever was set by default, in this case a default HttpClient.
#3 The Fabricam test itself is a proof for the scenario where passed in HttpClient can still be overridden.
If we go with the virtual route, we do not necessarily have to provide an overload for the constructor for our clients.
Let's talk more about your concern about potential bug. Could you provide a potential bug that I might have missed. The entire concern on Set is called in constructor is something I would like to understand better.
I think making it HttpClient property virtual is less risky and far less code churn than providing an overload.
If we go with the "making it virtual" route, we do not need any regenerating of code for the clients via autorest for new clients.

[hovsepm]

@shahabhijeet I do not see any real world scenario of the usefulness of having HttpClient as a virtual property on the base abstract ServiceClient class. Please revise/add better example of it's usefulness or remove "virtual". This is a code in the core component and we need to be careful not to introduce potential issues in dependent code.

[shahabhijeet]

Closing this PR and will open a new one. Again getting hit by git issues due to pull merges and unable to rebase to eliminate polluting git log/history.