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.

Sign up for GitHub

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

Jump to bottom

[wasm] Modify System.Net.NetworkInformation to throw PNSE #38928

Merged
merged 6 commits into from
Jul 10, 2020
Merged

[wasm] Modify System.Net.NetworkInformation to throw PNSE #38928

merged 6 commits into from
Jul 10, 2020

Conversation

MaximLipnin
Copy link
Contributor

Part of #38422.

@ghost
Copy link

ghost commented Jul 8, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

stephentoub
stephentoub reviewed Jul 8, 2020
View reviewed changes
src/libraries/System.Net.NetworkInformation/tests/FunctionalTests/NetworkInterfaceBasicTest.cs Outdated
@@ -21,6 +21,7 @@ public NetworkInterfaceBasicTest(ITestOutputHelper output)
}

[Fact]
[PlatformSpecific(~TestPlatforms.Browser)] // throws System.IO.DirectoryNotFoundException : Could not find a part of the path '/etc/resolv.conf'
Copy link
Member

Choose a reason for hiding this comment

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

Is this an acceptable experience, getting a DirectoryNotFoundException for /etc/resolv.conf ?

Copy link
Member

Choose a reason for hiding this comment

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

note that /etc/resolv.conf is generally present but not required for name resolution to work on Unix.
I'm wondering if it would make sense to fix either the test or production code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to file an issue?

@akoeplinger
Copy link
Member

I think almost none of the APIs in this assembly can be implemented on Browser/WebAssembly as we don't have access to the network cards.

The only exceptions I can think of are NetworkInterface.GetIsNetworkAvailable() and NetworkChange.NetworkAvailabilityChanged which could probably be implemented on top of https://developer.mozilla.org/en-US/docs/Web/API/NavigatorOnLine/onLine

My vote would be to add PNSE for the whole assembly and file a tracking issue to eventually implement those two.

@stephentoub
Copy link
Member

My vote would be to add PNSE for the whole assembly and file a tracking issue to eventually implement those two.

That sounds fine to me.

note that /etc/resolv.conf is generally present but not required for name resolution to work on Unix.

Sounds like something we should fix separately from the wasm effort.

@MaximLipnin
Copy link
Contributor Author

file a tracking issue to eventually implement those two.

I filed the issue for that: #38988

@akoeplinger akoeplinger mentioned this pull request Jul 9, 2020
Open
11 tasks
@MaximLipnin MaximLipnin force-pushed the wasm_System.Net.NetworkInformation branch from aee1d4c to cc3c3e9 Compare July 9, 2020 11:10
@steveisok steveisok mentioned this pull request Jul 9, 2020
Closed
99 tasks
@MaximLipnin MaximLipnin changed the title [wasm] Enable System.Net.NetworkInformation test suite [wasm] Modify System.Net.NetworkInformation to throw PNSE Jul 9, 2020
@akoeplinger akoeplinger merged commit c2b042d into dotnet:master Jul 10, 2020
@MaximLipnin MaximLipnin deleted the wasm_System.Net.NetworkInformation branch July 10, 2020 11:28
safern
safern reviewed Jul 10, 2020
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM

safern
safern reviewed Jul 10, 2020
View reviewed changes
src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs
@@ -187,6 +188,7 @@ public void ValidateDeserializationOfObjectWithGenericTypeWhichGenericArgumentHa
[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/34008", TestPlatforms.Linux, TargetFrameworkMonikers.Netcoreapp, TestRuntimes.Mono)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/34753", TestPlatforms.Windows, TargetFrameworkMonikers.Netcoreapp, TestRuntimes.Mono)]
[PlatformSpecific(~TestPlatforms.Browser)]
Copy link
Member

Choose a reason for hiding this comment

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

@akoeplinger this is one of the tests we would end up disabling after BinaryFormatter is killed on WASM.

Maybe we want to update to use conditional with PlatformDetecion.IsBinaryFormatterSupported

Copy link
Member

Choose a reason for hiding this comment

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

@safern yeah, feel free to send a follow up PR for that :)

src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs
@@ -49,6 +49,7 @@ public void SerializeHugeObjectGraphs(int limit)
[SkipOnCoreClr("Takes too long on Checked", RuntimeConfiguration.Checked)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/34008", TestPlatforms.Linux, TargetFrameworkMonikers.Netcoreapp, TestRuntimes.Mono)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/34753", TestPlatforms.Windows, TargetFrameworkMonikers.Netcoreapp, TestRuntimes.Mono)]
[PlatformSpecific(~TestPlatforms.Browser)]
Copy link
Member

Choose a reason for hiding this comment

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

This as well.

@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@wfurt wfurt wfurt left review comments

@safern safern safern left review comments

@steveisok steveisok steveisok approved these changes

@mdh1418 mdh1418 Awaiting requested review from mdh1418

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

Assignees
No one assigned
Labels
area-System.Net
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@MaximLipnin @akoeplinger @stephentoub @steveisok @wfurt @safern @karelz @Dotnet-GitSync-Bot