WinHttpHandler: HttpClientHandlerTestBase.AllowAllCertificates should not be static

[antonfirsov]

The default value of this property is true. Tests like like PlatformHandler_HttpClientHandler_ServerCertificates_Http2_Test are altering this global state, which may cause subsequently executed tests to fail because their prerequisites are not met:

runtime/src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/PlatformHandlerTest.cs

Lines 275 to 282 in 1d9e50c

	public sealed class PlatformHandler_HttpClientHandler_ServerCertificates_Http2_Test : HttpClientHandler_ServerCertificates_Test
	{
	protected override Version UseVersion => HttpVersion20.Value;
	public PlatformHandler_HttpClientHandler_ServerCertificates_Http2_Test(ITestOutputHelper output) : base(output) {
	AllowAllCertificates = false;
	}
	}

This is the root cause of the failures I discovered in #48704 (comment), and a prerequisite to proceed with that PR, since the new tests are incorrect there (although they are all skipped on today's CI machines, so we can't see the failures in the PR).

/cc @geoffkizer @wfurt

Update

Instead of changing the static property to an instance one, allowAllHttp2Certificates is now a parameter of the general, shared overload of CreateHttpClientHandler(...). This way individual test classes can implement their mechanism to manage this parameter, so far the only one is HttpClientHandler_ServerCertificates_Test.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The default value of this property is true. Tests like like PlatformHandler_HttpClientHandler_ServerCertificates_Http2_Test are altering this global state, which may cause subsequently executed tests to fail because their prerequisites are not met:

runtime/src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/PlatformHandlerTest.cs

Lines 275 to 282 in 1d9e50c

	public sealed class PlatformHandler_HttpClientHandler_ServerCertificates_Http2_Test : HttpClientHandler_ServerCertificates_Test
	{
	protected override Version UseVersion => HttpVersion20.Value;
	public PlatformHandler_HttpClientHandler_ServerCertificates_Http2_Test(ITestOutputHelper output) : base(output) {
	AllowAllCertificates = false;
	}
	}

This is the root cause of the failures I discovered in #48704 (comment), and a prerequisite to proceed with that PR, since the new tests are incorrect there (although they are all skipped on today's CI machines, so we can't see the failures in the PR).

/cc @geoffkizer @wfurt

Author:	antonfirsov
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

@dotnet/ncl this is ready for review again. Instead of changing the static property to an instance one, allowAllHttp2Certificates is now a parameter of the general, shared overload of CreateHttpClientHandler(...). This way individual test classes can implement their mechanism to manage this parameter, so far the only one is HttpClientHandler_ServerCertificates_Test.

[antonfirsov]

Inner and outerloop test failures are unrelated: #45868, #40798, #46439, #41606 and similar.

[wfurt]

LGTM.

While this looks ok and solves particular problem I'm wondering about the tests.
AllowAllCertificates generally skips all TLS cert validation so I'm curious why this is set only for platform handler and why only for HTTP2. I would expect that certificate processing is not dependent on HTTP protocol version.

[antonfirsov]

AllowAllCertificates generally skips all TLS cert validation so I'm curious why this is set only for platform handler and why only for HTTP2.

I was wondering about the same. @scalablecory @geoffkizer any thoughts on this?

Gonna merge this to unblock #48704.