Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate, expand, and clean up some HTTP proxy tests #48687

Merged
merged 17 commits into from
Feb 28, 2021

Conversation

geoffkizer
Copy link
Contributor

Move some proxy-related tests into HttpClientHandlerTest.Proxy.cs
Expand some tests to include more variations and be consistent about the variations they test
Re-enable a proxy test that's been disabled for a while and is hopefully no longer flaky
Remove checks for HTTP3 as they are not needed because this class is not included in HTTP3 tests
Various minor cleanup

Contributes to #30205

@ghost
Copy link

ghost commented Feb 24, 2021

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

Issue Details

Move some proxy-related tests into HttpClientHandlerTest.Proxy.cs
Expand some tests to include more variations and be consistent about the variations they test
Re-enable a proxy test that's been disabled for a while and is hopefully no longer flaky
Remove checks for HTTP3 as they are not needed because this class is not included in HTTP3 tests
Various minor cleanup

Contributes to #30205

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt
Copy link
Member

wfurt commented Feb 24, 2021

Seems like some platforms do not like the change:

HttpClientHandlerTest.Proxy.cs(181,49): error CS8400: (NETCORE_ENGINEERING_TELEMETRY=Build) Feature 'not pattern' is not available in C# 8.0. Please use language version 9.0 or greater.

@geoffkizer
Copy link
Contributor Author

Strange. These are caused by using "is not null". We definitely use this in the product, so I'm not sure why it would be disallowed in tests, and apparently only on certain platforms... @stephentoub any idea what's going on here?

Regardless, I'll fix it.

@stephentoub
Copy link
Member

any idea what's going on here?

Whatever project/target this is set in is probably being compiled with LangVersion set to a version prior to is not null being supported.

@stephentoub
Copy link
Member

@geoffkizer
Copy link
Contributor Author

Thanks. Is that intentional/necessary?

@stephentoub
Copy link
Member

I don't know why it's needed. We generally set LangVersion to preview or latest for the whole repo. It was added here:
188243a#diff-a53289c23b6308f9f138e1fa9d47f9bb2cb403b4dd73892f623fcc5ea94a3790R6
Maybe @alnikola knows.

I'd say just delete it and see what if anything fails to compile.

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@geoffkizer
Copy link
Contributor Author

@wfurt can you review when you have a minute?

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@geoffkizer geoffkizer merged commit a2cc299 into dotnet:master Feb 28, 2021
@geoffkizer geoffkizer deleted the proxytests2 branch February 28, 2021 04:08
@ghost ghost locked as resolved and limited conversation to collaborators Mar 30, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants