-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Implement straightforward ServicePoint(Manager) properties in HttpWebRequest #94664
Implement straightforward ServicePoint(Manager) properties in HttpWebRequest #94664
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsI'm going to create an issue with a list of properties that are not implemented. Implemented properties from ServicePoint:
Implemented properties from ServicePointManager:
|
/azp list |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it's impossible to implement functional tests for this. How do you test/validate the changes?
src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
I'm testing these options manually, but I'll try to write some tests for this. |
src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally looks good to me.
We should perhaps add at least some get/set tests so we exercise the new code path.
What issue(s) does this fixes/contributes to? Could you put them to the description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for the tests as well, some coverage would be nice.
src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Requests/src/System/Net/ServicePoint/ServicePointManager.cs
Show resolved
Hide resolved
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs
Outdated
Show resolved
Hide resolved
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
@dotnet/ncl This is ready for final review, I've tested implemented features through Wireshark, and it's working. |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Product code LGTM, a few suggestions for the tests.
Co-authored-by: Anton Firszov <[email protected]>
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % comments.
Good improvement.
Co-authored-by: Miha Zupan <[email protected]>
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Outerloop failures unrelated |
I'm going to create an issue with a list of properties that are not implemented.
I'm still working on the
AllowWriteStreamBuffering
property, I'll create a follow-up PR for this.Implemented properties from ServicePoint: