Add TestServer support for GraphQLHttpClient #354

[andreyleskov]

This PR makes GraphQLHttpClient usable with TestServer, targeting #354

Usage

  TestServer testServer; //needs initialization
  GraphQLHttpClientOptions options; //needs initialization
  IGraphQLWebsocketJsonSerializer serializer;  //needs initialization

  var graphQLHttpClient = testServer.CreateGraphQLHttpClient(options,serializer);

Implementation
GraphQLHttpClient and GraphQLHttpWebSocket have a new internal constructor with a new factory parameter for connected WebSocket creation.

Func<Uri, CancellationToken,Task<WebSocket>> connectedWebSocketFactory

This parameter is used to provide TestWebSocket instance from TestServer.
There is a new project GraphQL.Client.TestHost with the extensions method to configure this factory for a given TestServer.

Discussion

1. Need to check if net461 build is ok, as I checked it only with Mono under MacOS, would be nice to check with full .Net.
2. There is CanHandleConnectionTimeout integration test that I could not make work for TestServer. Seems like TestServer does not drop WebSocket connection on dispose. I could be wrong. Any help or suggestions are welcome. As a shortcut, I moved this test out of scope for TestServer
3. Way to inject WebSocket into GraphQLHttpClient. Now it is dictated by TestServer's WebSocketClient, and, maybe, there is a better way other than GraphQLHttpClient internal constructor requiring complex factory setup and separate project usage.
4. Tests stability, as mentioned in Unstable Integration Tests #161. I can see the same behavior for TestServer and think it is caused by synchronization inside tests themselves working with the same server instance. This issue is not targeted by the PR, but it makes this hypothesis check easier a bit by allowing to run a dedicated TestServer for each test.

[rose-a]

Hi, just wanted to let you know that im currently looking into your MR...

The results from your new tests and those from the legacy ones are the same (which is good), but they are also noteably faster. I suspect the errors mentioned in #161 are the result of a threading issue with the test classes for the IObservables, since the trigger for the observable happens in the same thread in which the expected result is awaited, which is a totally untypical use of System.Reactive. They're also sporadic (it's not always the same test that fails), which makes it even harder to find them.
I'm having the same issues in other projects im working on where I'm trying to test observables in a similar way.

I think the injection of a websocket factory might actually be a nice improvement, since it would allow us to move the dependency on System.Net.WebSockets.Client.Managed and all that conditional #if NETFRAMEWORK stuff to a separate package which is only ever needed when targeting Windows 7.

[rose-a]

@andreyleskov please rebase your branch on release-v4

already done

[sungam3r]

@rose-a @andreyleskov regarding 4. Tests stability:

I recommend you ALWAYS use ConfigureAwait(false) in library code especially when using XUnit package as a test framework because of Xunit.Sdk.MaxConcurrencySyncContext. In my case maximumConcurrencyLevel was 6 and some tests randomly failed. So please look through entire codebase and add ConfigureAwait(false) to awaitable calls. Perhaps it will help.

[andreyleskov]

Thanks a lot for the comment, it makes sense from my point of view. This PR is following the existing await style and focuses on just WebSockets. I assume repository owners prefer to do repository-wide changes as await style in a dedicated PR.

[rose-a]

im on to this in in #370...

[sungam3r]

Suggested change

	public abstract class BaseWithTimeout:Base
	public abstract class BaseWithTimeout : Base

[andreyleskov]

Hey guys, can I do smth to make this PR move? Please let me know

[jerry2007]

Hello, what about this PR. I would like to use this...
@rose-a ????

[alexander-jesner-AP]

Hi, @rose-a, is it possible to merge this PR? If not, how can we help to push it towards being mergeable?

[tomachristian]

This would have been a lifesaver...