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

Tests in System.Data.Common crash on uapaot #21453

Closed
alexperovich opened this issue May 1, 2017 · 15 comments · Fixed by dotnet/corefx#21648
Closed

Tests in System.Data.Common crash on uapaot #21453

alexperovich opened this issue May 1, 2017 · 15 comments · Fixed by dotnet/corefx#21648
Assignees
Milestone

Comments

@alexperovich
Copy link
Member

The following tests crash on uapaot:

  • System.Data.Tests.SqlTypes.SqlXmlTest.Constructor2_Stream_Unicode
  • System.Data.Tests.SqlTypes.SqlXmlTest.Constructor2_Stream_Empty
  • System.Data.Tests.SqlTypes.SqlXmlTest.Constructor3
  • System.Data.Tests.SqlTypes.SqlXmlTest.Constructor3_XmlReader_Empty
  • System.Data.Tests.SqlTypes.SqlXmlTest.CreateReader_Stream_Unicode
  • System.Data.Tests.SqlTypes.SqlXmlTest.SqlXml_fromXmlReader_CreateReaderTest
  • System.Data.Tests.SqlTypes.SqlXmlTest.SqlXml_fromZeroLengthStream_CreateReaderTest
  • System.Data.Tests.SqlTypes.SqlXmlTest.SqlXml_fromZeroLengthXmlReader_CreateReaderTest_withFragment
@sjoerdverweij
Copy link

I can fix SqlDateTimeTest (not a contributor yet)

@danmoseley
Copy link
Member

danmoseley commented Jun 16, 2017

@sjoerdverweij missed your post. That would be welcome except there is no way for ocmmunity folk to run uapaot tests... perhaps you'd like to make a different contribution?

@saurabh500
Copy link
Contributor

saurabh500 commented Jun 29, 2017

These tests still fail while running with TargetGroup=uapaot

@saurabh500 saurabh500 reopened this Jun 29, 2017
@saurabh500
Copy link
Contributor

saurabh500 commented Jun 29, 2017

So far we have the following findings:

  1. The Internal method XmlReader.CreateSqlReader cannot be accessed without modifying the rd.xml file.
    The modifications suggested are
<Assembly Name="System.Private.Xml">
      <Namespace Name="System.Xml">
        <!-- XmlQualifiedName is well known to the serializers and must not be explicitly specified to sg.exe -->
        <Type Name="XmlQualifiedName" DataContractSerializer="Excluded" DataContractJsonSerializer="Excluded"/>               
                <Type Name="XmlReader" Dynamic="Required" />
      </Namespace>

Add this to System.Private.Xml.csproj
<BlockReflectionAttribute>false</BlockReflectionAttribute>

  1. Make the method public and make System.Data.Common depend on System.Private.Xml to access the public method instead of accessing the method via reflection.

@safern safern assigned saurabh500 and unassigned danmoseley Jun 30, 2017
@safern
Copy link
Member

safern commented Jun 30, 2017

@saurabh500 could you please take a look and fix this?

@saurabh500
Copy link
Contributor

@safern are you talking about reverting the PR which enable this test or a real fix?
I am working with different folks to get a fix in.

I am stuck at figuring out how to make System.Data.COmmon build with System.Private.Xml instead of the System.Xml.ReaderWriter. Any clues?

@safern
Copy link
Member

safern commented Jun 30, 2017

I'm talking about a real fix. Will not revert the PR as the DateTimeTest seems to be fixed. I'm just confirming locally.

I am stuck at figuring out how to make System.Data.COmmon build with System.Private.Xml instead of the System.Xml.ReaderWriter. Any clues?

Not that come to my mind now but @joperezr might know.

@safern
Copy link
Member

safern commented Jun 30, 2017

System.Data.Tests.SqlTypes.SqlDateTimeTest.GetTypeTest is indeed fixed. I don't see it failing anymore. Updating the issue description

@saurabh500
Copy link
Contributor

saurabh500 commented Jun 30, 2017

A little context here : I have already tried using ProjectReference to include the System.Private.Xml.csproj in the System.Data.Common.csproj (after removing the System.Xml.ReaderWriter) dependency. However I get compilation errors for all the Xml namespaces.
This specifically needs to be done for 'uapaot'

@joperezr
Copy link
Member

I am stuck at figuring out how to make System.Data.COmmon build with System.Private.Xml instead of the System.Xml.ReaderWriter

Well, System.Xml.ReaderWriter is a contract assembly and System.Private.Xml is the implementation of that contract. Do you need an internal type or something that only exists at runtime for this? If so, you basically have to move all of your dependencies to be implementation assemblies instead. For an example of this, you can take a look at the implementation project of System.Collections, which builds against implementations instead of contracts. I can help you accomplish this offline if you'd like.

@saurabh500
Copy link
Contributor

@joperezr I need to change System.Private.Xml to expose an internal function for uapaot. That's why I need to change this dependency to compile System.Data.Common against System.Private.Xml.csproj

I don't think I need to change all my dependencies to implementation. I could verify it by adding

<ItemGroup>
    <ProjectReference Include="..\..\System.Private.Xml\src\System.Private.Xml.csproj" />
  </ItemGroup>

and compiling with msbuild /p:TargetGroup=uapaot

However the problems arise when I use a Condition on the above ItemGroup

<ItemGroup Condition="'$(TargetGroup)' == 'uapaot' and '$(TargetGroup)' != 'netfx'">
    <ProjectReference Include="..\..\System.Private.Xml\src\System.Private.Xml.csproj" />
  </ItemGroup>

Msbuild /v:d /p:TargetGroup=uapaot shows the TG being set to uap instead of uapaot
As a result, the csproj is not referenced. What else do I need to change to make sure that System.Data.Common can have a target group of uapaot instead of a fallback to uap?

@saurabh500
Copy link
Contributor

The plan is to make the changes so that they are specific only to uapaot.

@joperezr
Copy link
Member

joperezr commented Jul 6, 2017

I believe that you have fixed this by looking at the email thread we have going on, but let me know if I can be of any help in here.

@safern
Copy link
Member

safern commented Jul 11, 2017

@saurabh500 you closed this from your PR but I didn't see that you re-enabled the tests in your PR. Could you please re-enabled them?

@saurabh500
Copy link
Contributor

Ah ok. I was working with a fork before you disabled the tests again. So in my workspace I always had the tests enabled. I should have rebased.
PR at dotnet/corefx#22111

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the UWP6.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants