-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
WASM: Fix System.Net.Primitives and tests #39748
WASM: Fix System.Net.Primitives and tests #39748
Conversation
Add HostInformationPal and InterfaceInfoPal implementations for Browser. In CookiePortTest.cs use example.com instead of localhost for the tests since Browser uses `localhost` as the `Environment.MachineName` and that changes the test values. Allows the tests to run on WebAssembly: ``` System.Net.Primitives.Pal.Tests: Tests run: 60, Errors: 0, Failures: 0, Skipped: 0. Time: 0.15872s System.Net.Primitives.Functional.Tests: Tests run: 2620, Errors: 0, Failures: 0, Skipped: 1. Time: 3.145804s ```
Tagging subscribers to this area: @dotnet/ncl |
The perf job failures are due to #38138 |
Assert.Equal(1, cookies.Count); | ||
} | ||
|
||
[Fact] | ||
public void Port_SpaceDelimiter_PortSet() | ||
{ | ||
CookieCollection cookies = _cc.GetCookies(new Uri("http://localhost:110/path")); | ||
CookieCollection cookies = _cc.GetCookies(new Uri("http://example.com:110/path")); |
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.
why this needs to be changed?
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.
@wfurt See the PR description:
In CookiePortTest.cs use example.com instead of localhost for the tests since Browser uses localhost as the Environment.MachineName and that changes the test values.
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.
The part I don't understand is why that would impact cookies. We don't do any networking here AFAIK, right?
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.
@wfurt we don't do networking but we still read the domain name (which on netcore on Unix generally means the machine name) here:
runtime/src/libraries/System.Net.Primitives/src/System/Net/CookieContainer.cs
Lines 139 to 145 in ea82629
private static string CreateFqdnMyDomain() | |
{ | |
string domain = HostInformation.DomainName; | |
return domain != null && domain.Length > 1 ? | |
'.' + domain : | |
string.Empty; | |
} |
That is later on used for this check:
runtime/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs
Lines 430 to 434 in ea82629
// First quick check is for pushing a cookie into the local domain. | |
if (isLocalDomain && string.Equals(localDomain, domain, StringComparison.OrdinalIgnoreCase)) | |
{ | |
valid = true; | |
} |
Since localDomain
and domain
are both localhost
we're going into that branch, which doesn't happen when the machine/domain name is set to any other value.
The result is that instead of getting this cookie once in the tests I'm getting it twice which fails the test:
Cookie:
name = value1
Domain: localhost
Path: /path
Port: "80 110,1050, 1090 ,1100"
Secure: False
When issued: 07/22/2020 17:46:32
Expires: 01/01/0001 00:00:00 (expired? False)
Don't save: False
Comment:
Uri for comments:
Version: RFC 2109
Do you think it could be a product bug?
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.
thanks for explanation. This is not my primary are and I'll ask around.
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
Add HostInformationPal and InterfaceInfoPal implementations for Browser. In CookiePortTest.cs use example.com instead of localhost for the tests since Browser uses `localhost` as the `Environment.MachineName` and that changes the test values. Allows the tests to run on WebAssembly: ``` System.Net.Primitives.Pal.Tests: Tests run: 60, Errors: 0, Failures: 0, Skipped: 0. Time: 0.15872s System.Net.Primitives.Functional.Tests: Tests run: 2620, Errors: 0, Failures: 0, Skipped: 1. Time: 3.145804s ```
Add HostInformationPal and InterfaceInfoPal implementations for Browser.
In CookiePortTest.cs use example.com instead of localhost for the tests since Browser uses
localhost
as theEnvironment.MachineName
and that changes the test values.Allows the tests to run on WebAssembly: