HTTP2: Many HTTP2 remote server tests are not actually using HTTP2

[geoffkizer]

I noticed this in PostScenarioTest.cs (where all tests are affected) and HttpClientHandler_Decompression_Test (where the two remote server tests are affected) in particular, but this issue probably exists in other remote server tests scattered throughout other files as well.

These tests take a Uri argument that enables them to run against both HTTP/1.1 and HTTP/2 remote endpoints. However, even when running against the HTTP/2 remote endpoint, only HTTP/1.1 is used. This is because the default request version is 1.1. Presumably we broke all these tests when we changed the default request version.

Some similar HTTP/2 remote endpoint tests, such as the ones in HttpClientHandlerTest.cs, do end up using HTTP/2, but the way they do this is kinda weird. This class has a derived test class (SocketsHttpHandlerTest_HttpClientHandlerTest_Http2) to enable HTTP/2 loopback tests, which sets UseHttp2 = true and causes the LoopbackServerFactory to create an Http2LoopbackServer. But this also causes CreateHttpClient to set the default request version to 2.0. So when run from the derived class for HTTP/2 loopback support, we actually use HTTP/2 for the remote HTTP/2 endpoint, and we get the desired coverage. At the same time, though, we're also running a different derivation of the test (in class SocketsHttpHandler_HttpClientHandlerTest) against the remote HTTP/2 endpoint with HTTP/2 disabled.

So for example, the very simple SendAsync_SimpleGet_Success test gets run 9 times: the cross product of

(PlatformHandler, SocketsHttpHandler, SocketsHttpHandler with HTTP/2 enabled)

and

(HTTP/1.1 remote endpoint, HTTPS/1.1 remote endpoint, HTTP/2 endpoint)

Several of the resulting variations end up being redundant; there's no point running with HTTP/2 enabled against an HTTP/1.1 endpoint, or with HTTP/1.1 against an HTTP/2 endpoint. And on top of that, we're not actually running PlatformHandler with HTTP/2 at all, though by itself I'm not sure we really care about that. It would be nice to remove the redundant variations just to reduce test time.

It would also be nice if these tests would validate that they use HTTP/2 when they expect to use HTTP/2, though it's not clear to me how to do this in a simple manner, given the issues described above.

Regardless of those issues, we need to get these tests working against HTTP/2. The POST tests in particular are quite valuable. The easiest way to do this would be to just do what HttpClientHandlerTest does and live with the redundancy.

Longer term, we should look at cleaning this all up: (a) removing the redundancy; (b) verifying that we're actually using the version we expect; (c) enabling remote HTTP2 tests for platform handlers; etc.

[geoffkizer]

Related to the test redundancy mentioned above, there's another type of test redundancy as well:

There are some classes (notably, HttpClientHandlerTest.cs) where we have a mix of tests using

(a) HTTP/1.1 loopback server (many of these should be converted to generic loopback server, but not all)
(b) Generic loopback server
(c) Remote endpoints

These classes get derived three times -- PlatformHandler, SocketsHandler with HTTP/1.1, and SocketsHandler with HTTP/2.

Thus tests in category (a) are getting run twice for SocketsHandler with HTTP/1.1.

I think the right long-term fix is to separate the types of tests listed above by class. That is, a single class should have only one type of test. Then, we can derive them correctly so that the proper test variants are run for each type of test. And each should probably derive from its own base class, to help enforce this properly.

[geoffkizer]

Actually, I think the easiest and safest fix here is to just always enable HTTP2 in HttpClientHandlerTestBase.CreateHttpClient.

Whether or not we actually do HTTP2 is controlled by the endpoint we are testing against, either a loopback server or a remote endpoint.

[karelz]

@geoffkizer do you think we can get the PRs merged by EOW?

[geoffkizer]

Hopefully, yes.

[karelz]

It was fixed in master (3.0) before last integration into release/3.0 branch.