-
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] Enable System.Data.Common tests #39463
Conversation
Tagging subscribers to this area: @roji, @ajcvickers |
src/libraries/System.Data.Common/tests/System/Data/Common/DbProviderFactoriesTests.cs
Outdated
Show resolved
Hide resolved
@steveisok is it possible to get a bit of context on why the DbProviderFactories and DataView APIs aren't supported on WASM? |
Not sure the official reason. I was carrying it over from mono/mono. I suspect the reason for |
@steveisok On the other hand, the SQLite ADO.NET provider already works on WASM, so any decisions here should not be based on whether or not the SQL Server ADO.NET provider will work. ADO.NET in general should work. |
@lewing Do you have any insight on this? |
@ajcvickers we don't want to break things needlessly but we want to make sure the use experience is good, can you help with guidance here? |
In principle, I don't see a reason not to allow using DbProviderFactories, DataView or anything else that's under System.Data - these APIs seem to be implemented in pure .NET (no native dependencies or similar), and as @ajcvickers they can be used just like on .NET Core to work with Sqlite. It's possible that mono simply has/had a somewhat outdated implementation of DbProviderFactories. There are also various other database providers out there, which may or may not work with WASM. I don't think we need to do anything in System.Data itself to block anything - whether a provider works or not should be their issue. |
Ok I think we'll roll back the PNSE and just disable the tests that require SQL Server. |
… were failing b/c of binaryformatter not being supported
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.
Looks good in general, just one comment.
src/libraries/System.Data.Common/tests/SuitePlatformDetection.cs
Outdated
Show resolved
Hide resolved
Disable the tests that require SQL Server. Co-authored-by: Steve Pfister <steve.pfister@microsoft.com> Co-authored-by: Maxim Lipnin <v-maxlip@microsoft.com>
Disable the tests that require SQL Server.