Skip to content

[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT - #75730

Merged
jkotas merged 6 commits into
dotnet:mainfrom
mdh1418:properly_skip_xslcompiledtransform
Sep 23, 2022
Merged

[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT#75730
jkotas merged 6 commits into
dotnet:mainfrom
mdh1418:properly_skip_xslcompiledtransform

Conversation

@mdh1418

@mdh1418mdh1418 commented Sep 15, 2022•
edited
Loading

Copy link
Copy Markdown
Member

Fixes#75699

In effort to consolidate System.Private.Xml Test projects, the test suite exclusions within https://github.com/dotnet/runtime/blob/main/src/libraries/tests.proj weren't being honored because the suites no longer existed.

Instead, to skip the tests, [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] is being added at the class level to skip tests where Reflection Emit is needed.

This PR skips tests within XslCompiledTransformAp and XmlSerializerTests

@ghost

ghost commented Sep 15, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghostghost assigned mdh1418Sep 15, 2022
@mdh1418

mdh1418 commented Sep 15, 2022

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

azure-pipelinesBot commented Sep 15, 2022

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky
MichalStrehovsky reviewed Sep 16, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj Outdated
Comment on lines +624 to +625
<ItemGroup Condition="(('$(TargetOS)' != 'tvOS' and '$(TargetOS)' != 'iOS') or '$(RunDisablediOSTests)' == 'true') and
('$(TestNativeAot)' != 'true' or '$(RunDisabledNativeAotTests)' == 'true')" >

@MichalStrehovskyMichalStrehovskySep 16, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It might be better to put [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] on the individual test classes in these files. It's more consistent with how we do things and more futureproof.

@mdh1418

mdh1418 commented Sep 16, 2022

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

azure-pipelinesBot commented Sep 16, 2022

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@filipnavarafilipnavara mentioned this pull request Sep 16, 2022
Merged
stephentoub
stephentoub approved these changes Sep 19, 2022
View reviewed changes

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM if it fixes the problem.

@filipnavara

filipnavara commented Sep 19, 2022•
edited
Loading

Copy link
Copy Markdown
Member

It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.

@mdh1418
mdh1418 marked this pull request as ready for review September 19, 2022 16:30
@mdh1418

mdh1418 commented Sep 19, 2022

Copy link
Copy Markdown
MemberAuthor

This PR was initially only targeting XslCompiledTransformApi tests, so I didn't have a look at all the other test suites that were removed and where the respective TestExclusions weren't honored, but I'll look at the failing XmlSerializerTests tests. Would it make sense to make PRs on a test suite by test suite basis, or trying to tackle them all in this one PR?

@filipnavara

filipnavara commented Sep 19, 2022

Copy link
Copy Markdown
Member

I don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines. In fact I started getting them locally for the main branch when building the NativeAOT tests. That's why I started watching this PR since it resolves a sizeable amount of the failures. I appreciate that you started looking into it even if it was not for the same underlying reason.

Btw, you should run the extras pipelines (/azp run runtime-extra-platforms) once you are reasonably sure that the tests from the previous run are now marked correctly.

@MichalStrehovsky

MichalStrehovsky commented Sep 19, 2022

Copy link
Copy Markdown
Member

don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines

The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.

@filipnavara

filipnavara commented Sep 20, 2022

Copy link
Copy Markdown
Member

The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.

Ah, that explains it. I can understand that it was missed as part of a PR if the extras pipelines were not run but I am quite surprised that there was no issue filed for it based on the rolling builds.

@stephentoub

stephentoub commented Sep 20, 2022

Copy link
Copy Markdown
Member

#75699 (comment)

@filipnavara

filipnavara commented Sep 20, 2022

Copy link
Copy Markdown
Member

#75699 (comment)

Missed that one (or rather it was not filed yet when I first hit the issue). Thanks.

@mdh1418
mdh1418force-pushed the properly_skip_xslcompiledtransform branch from 00e9588 to 966cfeaCompareSeptember 20, 2022 04:54
@mdh1418

mdh1418 commented Sep 20, 2022

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

azure-pipelinesBot commented Sep 20, 2022

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@mdh1418

mdh1418 commented Sep 20, 2022

Copy link
Copy Markdown
MemberAuthor

It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.

I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.
Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).

I looked at the comment in tests.proj and it looks like System.XmlSerializer is just not supported on NativeAOT right now.

I also noticed some test failures in two more suites that were removed System.Xml.Xsl.XslCompiledTransformApi.Tests.csproj however there doesn't seem to be a test suite skip in tests.proj, so not sure how these were skipped in the past. They were failing with

System.Xml.XmlSchemaValidatorApiTests.VerifyException : Error while loading assembly
at System.Xml.XmlSchemaValidatorApiTests.ExceptionVerifier..ctor(String assemblyName, ExceptionVerificationFlags flags, ITestOutputHelper output) + 0x5fd
at System.Xml.XmlSchemaValidatorApiTests.TCInitialize..ctor(ITestOutputHelper output) + 0x48
at System.Private.Xml.Tests!<BaseAddress>+0x12ad20d
at System.Reflection.DynamicInvokeInfo.Invoke(Object thisPtr, IntPtr methodToCall, Object[] parameters, BinderBundle binderBundle, Boolean wrapInTargetInvocationException) + 0x150

@filipnavara

filipnavara commented Sep 20, 2022

Copy link
Copy Markdown
Member

I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.
Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).

It's a bit misleading but if you look closely at the stack trace the inner exception is still PlatformNotSupportedException due to unsupported Reflection Emit.

@mdh1418
[tests] Condition XmlSerializerTests on ReflectionEmit
835d515
Tests fail from trimming and MakeGeneric, and XmlSerializer is not supported with NativeAOT
@mdh1418
[tests] Condition problematic XmlSchemaValidatorApiTests classes on R…
103d596
…eflectionEmit
@mdh1418
mdh1418force-pushed the properly_skip_xslcompiledtransform branch from 966cfea to 103d596CompareSeptember 21, 2022 17:11
@mdh1418

mdh1418 commented Sep 21, 2022

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

azure-pipelinesBot commented Sep 21, 2022

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@filipnavarafilipnavara mentioned this pull request Sep 23, 2022
Closed
@jkotas

jkotas commented Sep 23, 2022

Copy link
Copy Markdown
Member

This is not fixing the whole problem, but it is still an improvement. Merging.

@jkotas

jkotas commented Sep 23, 2022

Copy link
Copy Markdown
Member

Actually, I do not think that the condition on the Schema and XmlSerializer test is right (it should not be ReflectionEmit). The Xslt one is good, so let's merge that one at least.

@jkotas
jkotas merged commit 74c589f into dotnet:mainSep 23, 2022
@ghostghost locked as resolved and limited conversation to collaborators Oct 24, 2022
@mdh1418
mdh1418 deleted the properly_skip_xslcompiledtransform branch October 28, 2022 02:18
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@stephentoubstephentoubstephentoub approved these changes

Assignees

@mdh1418mdh1418

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[iOS] XslCompiledTransform tests failing on devices

5 participants

@mdh1418@filipnavara@MichalStrehovsky@stephentoub@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT by mdh1418 · Pull Request #75730 · dotnet/runtime · GitHub
Skip to content
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT - #75730
Merged
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Sep 23, 2022
Merged
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT#75730
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Conversation
@mdh1418
@mdh1418mdh1418
commented
Sep 15, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#75699
In effort to consolidate System.Private.Xml Test projects, the test suite exclusions within https://github.com/dotnet/runtime/blob/main/src/libraries/tests.proj weren't being honored because the suites no longer existed.
Instead, to skip the tests, [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] is being added at the class level to skip tests where Reflection Emit is needed.
This PR skips tests within XslCompiledTransformAp and XmlSerializerTests
@ghost
ghost
commented
Sep 15, 2022

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@ghostghost
assigned mdh1418Sep 15, 2022
@mdh1418
mdh1418
commented
Sep 15, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
MichalStrehovsky
MichalStrehovsky
reviewed
Sep 16, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj
Outdated


Comment on lines

+624
 to 
+625

 <ItemGroup Condition="(('$(TargetOS)' != 'tvOS' and '$(TargetOS)' != 'iOS') or '$(RunDisablediOSTests)' == 'true') and
 ('$(TestNativeAot)' != 'true' or '$(RunDisabledNativeAotTests)' == 'true')" >
@MichalStrehovskyMichalStrehovskySep 16, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It might be better to put [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] on the individual test classes in these files. It's more consistent with how we do things and more futureproof.
@mdh1418
mdh1418
commented
Sep 16, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 16, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 16, 2022
 Merged

stephentoub
stephentoub
approved these changes
Sep 19, 2022
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM if it fixes the problem.
@filipnavara
filipnavara
commented
Sep 19, 2022•

edited

Loading

Copy link


Copy Markdown

Member
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
@mdh1418
mdh1418
marked this pull request as ready for review
September 19, 2022 16:30
@mdh1418
mdh1418
commented
Sep 19, 2022

Copy link


Copy Markdown

MemberAuthor
This PR was initially only targeting XslCompiledTransformApi tests, so I didn't have a look at all the other test suites that were removed and where the respective TestExclusions weren't honored, but I'll look at the failing XmlSerializerTests tests. Would it make sense to make PRs on a test suite by test suite basis, or trying to tackle them all in this one PR?
@filipnavara
filipnavara
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
I don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines. In fact I started getting them locally for the main branch when building the NativeAOT tests. That's why I started watching this PR since it resolves a sizeable amount of the failures. I appreciate that you started looking into it even if it was not for the same underlying reason.
Btw, you should run the extras pipelines (/azp run runtime-extra-platforms) once you are reasonably sure that the tests from the previous run are now marked correctly.
@MichalStrehovsky
MichalStrehovsky
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
Ah, that explains it. I can understand that it was missed as part of a PR if the extras pipelines were not run but I am quite surprised that there was no issue filed for it based on the rolling builds.
@stephentoub
stephentoub
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
Missed that one (or rather it was not filed yet when I first hit the issue). Thanks.
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
00e9588 to
966cfeaCompareSeptember 20, 2022 04:54
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 20, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
I looked at the comment in tests.proj and it looks like System.XmlSerializer is just not supported on NativeAOT right now.
I also noticed some test failures in two more suites that were removed System.Xml.Xsl.XslCompiledTransformApi.Tests.csproj however there doesn't seem to be a test suite skip in tests.proj, so not sure how these were skipped in the past. They were failing with
System.Xml.XmlSchemaValidatorApiTests.VerifyException : Error while loading assembly
at System.Xml.XmlSchemaValidatorApiTests.ExceptionVerifier..ctor(String assemblyName, ExceptionVerificationFlags flags, ITestOutputHelper output) + 0x5fd
at System.Xml.XmlSchemaValidatorApiTests.TCInitialize..ctor(ITestOutputHelper output) + 0x48
at System.Private.Xml.Tests!<BaseAddress>+0x12ad20d
at System.Reflection.DynamicInvokeInfo.Invoke(Object thisPtr, IntPtr methodToCall, Object[] parameters, BinderBundle binderBundle, Boolean wrapInTargetInvocationException) + 0x150

@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
It's a bit misleading but if you look closely at the stack trace the inner exception is still PlatformNotSupportedException due to unsupported Reflection Emit.
@mdh1418
[tests] Condition XmlSerializerTests on ReflectionEmit
835d515
Tests fail from trimming and MakeGeneric, and XmlSerializer is not supported with NativeAOT
@mdh1418
[tests] Condition problematic XmlSchemaValidatorApiTests classes on R…
103d596
…eflectionEmit
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
966cfea to
103d596CompareSeptember 21, 2022 17:11
@mdh1418
mdh1418
commented
Sep 21, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 21, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 23, 2022
 Closed

@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
This is not fixing the whole problem, but it is still an improvement. Merging.
@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
Actually, I do not think that the condition on the Schema and XmlSerializer test is right (it should not be ReflectionEmit). The Xslt one is good, so let's merge that one at least.
@jkotas
jkotas
merged commit 74c589f
into
dotnet:mainSep 23, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 24, 2022
@mdh1418
mdh1418
deleted the

properly_skip_xslcompiledtransform

branch
October 28, 2022 02:18
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@stephentoubstephentoubstephentoub approved these changes

Assignees

@mdh1418mdh1418

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[iOS] XslCompiledTransform tests failing on devices


5 participants

@mdh1418@filipnavara@MichalStrehovsky@stephentoub@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT by mdh1418 · Pull Request #75730 · dotnet/runtime · GitHub
Skip to content
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT - #75730
Merged
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Sep 23, 2022
Merged
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT#75730
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Conversation
@mdh1418
@mdh1418mdh1418
commented
Sep 15, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#75699
In effort to consolidate System.Private.Xml Test projects, the test suite exclusions within https://github.com/dotnet/runtime/blob/main/src/libraries/tests.proj weren't being honored because the suites no longer existed.
Instead, to skip the tests, [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] is being added at the class level to skip tests where Reflection Emit is needed.
This PR skips tests within XslCompiledTransformAp and XmlSerializerTests
@ghost
ghost
commented
Sep 15, 2022

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@ghostghost
assigned mdh1418Sep 15, 2022
@mdh1418
mdh1418
commented
Sep 15, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
MichalStrehovsky
MichalStrehovsky
reviewed
Sep 16, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj
Outdated


Comment on lines

+624
 to 
+625

 <ItemGroup Condition="(('$(TargetOS)' != 'tvOS' and '$(TargetOS)' != 'iOS') or '$(RunDisablediOSTests)' == 'true') and
 ('$(TestNativeAot)' != 'true' or '$(RunDisabledNativeAotTests)' == 'true')" >
@MichalStrehovskyMichalStrehovskySep 16, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It might be better to put [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] on the individual test classes in these files. It's more consistent with how we do things and more futureproof.
@mdh1418
mdh1418
commented
Sep 16, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 16, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 16, 2022
 Merged

stephentoub
stephentoub
approved these changes
Sep 19, 2022
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM if it fixes the problem.
@filipnavara
filipnavara
commented
Sep 19, 2022•

edited

Loading

Copy link


Copy Markdown

Member
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
@mdh1418
mdh1418
marked this pull request as ready for review
September 19, 2022 16:30
@mdh1418
mdh1418
commented
Sep 19, 2022

Copy link


Copy Markdown

MemberAuthor
This PR was initially only targeting XslCompiledTransformApi tests, so I didn't have a look at all the other test suites that were removed and where the respective TestExclusions weren't honored, but I'll look at the failing XmlSerializerTests tests. Would it make sense to make PRs on a test suite by test suite basis, or trying to tackle them all in this one PR?
@filipnavara
filipnavara
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
I don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines. In fact I started getting them locally for the main branch when building the NativeAOT tests. That's why I started watching this PR since it resolves a sizeable amount of the failures. I appreciate that you started looking into it even if it was not for the same underlying reason.
Btw, you should run the extras pipelines (/azp run runtime-extra-platforms) once you are reasonably sure that the tests from the previous run are now marked correctly.
@MichalStrehovsky
MichalStrehovsky
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
Ah, that explains it. I can understand that it was missed as part of a PR if the extras pipelines were not run but I am quite surprised that there was no issue filed for it based on the rolling builds.
@stephentoub
stephentoub
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
Missed that one (or rather it was not filed yet when I first hit the issue). Thanks.
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
00e9588 to
966cfeaCompareSeptember 20, 2022 04:54
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 20, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
I looked at the comment in tests.proj and it looks like System.XmlSerializer is just not supported on NativeAOT right now.
I also noticed some test failures in two more suites that were removed System.Xml.Xsl.XslCompiledTransformApi.Tests.csproj however there doesn't seem to be a test suite skip in tests.proj, so not sure how these were skipped in the past. They were failing with
System.Xml.XmlSchemaValidatorApiTests.VerifyException : Error while loading assembly
at System.Xml.XmlSchemaValidatorApiTests.ExceptionVerifier..ctor(String assemblyName, ExceptionVerificationFlags flags, ITestOutputHelper output) + 0x5fd
at System.Xml.XmlSchemaValidatorApiTests.TCInitialize..ctor(ITestOutputHelper output) + 0x48
at System.Private.Xml.Tests!<BaseAddress>+0x12ad20d
at System.Reflection.DynamicInvokeInfo.Invoke(Object thisPtr, IntPtr methodToCall, Object[] parameters, BinderBundle binderBundle, Boolean wrapInTargetInvocationException) + 0x150

@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
It's a bit misleading but if you look closely at the stack trace the inner exception is still PlatformNotSupportedException due to unsupported Reflection Emit.
@mdh1418
[tests] Condition XmlSerializerTests on ReflectionEmit
835d515
Tests fail from trimming and MakeGeneric, and XmlSerializer is not supported with NativeAOT
@mdh1418
[tests] Condition problematic XmlSchemaValidatorApiTests classes on R…
103d596
…eflectionEmit
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
966cfea to
103d596CompareSeptember 21, 2022 17:11
@mdh1418
mdh1418
commented
Sep 21, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 21, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 23, 2022
 Closed

@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
This is not fixing the whole problem, but it is still an improvement. Merging.
@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
Actually, I do not think that the condition on the Schema and XmlSerializer test is right (it should not be ReflectionEmit). The Xslt one is good, so let's merge that one at least.
@jkotas
jkotas
merged commit 74c589f
into
dotnet:mainSep 23, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 24, 2022
@mdh1418
mdh1418
deleted the

properly_skip_xslcompiledtransform

branch
October 28, 2022 02:18
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@stephentoubstephentoubstephentoub approved these changes

Assignees

@mdh1418mdh1418

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[iOS] XslCompiledTransform tests failing on devices


5 participants

@mdh1418@filipnavara@MichalStrehovsky@stephentoub@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT by mdh1418 · Pull Request #75730 · dotnet/runtime · GitHub
Skip to content
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT - #75730
Merged
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Sep 23, 2022
Merged
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT#75730
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Conversation
@mdh1418
@mdh1418mdh1418
commented
Sep 15, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#75699
In effort to consolidate System.Private.Xml Test projects, the test suite exclusions within https://github.com/dotnet/runtime/blob/main/src/libraries/tests.proj weren't being honored because the suites no longer existed.
Instead, to skip the tests, [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] is being added at the class level to skip tests where Reflection Emit is needed.
This PR skips tests within XslCompiledTransformAp and XmlSerializerTests
@ghost
ghost
commented
Sep 15, 2022

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@ghostghost
assigned mdh1418Sep 15, 2022
@mdh1418
mdh1418
commented
Sep 15, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
MichalStrehovsky
MichalStrehovsky
reviewed
Sep 16, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj
Outdated


Comment on lines

+624
 to 
+625

 <ItemGroup Condition="(('$(TargetOS)' != 'tvOS' and '$(TargetOS)' != 'iOS') or '$(RunDisablediOSTests)' == 'true') and
 ('$(TestNativeAot)' != 'true' or '$(RunDisabledNativeAotTests)' == 'true')" >
@MichalStrehovskyMichalStrehovskySep 16, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It might be better to put [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] on the individual test classes in these files. It's more consistent with how we do things and more futureproof.
@mdh1418
mdh1418
commented
Sep 16, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 16, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 16, 2022
 Merged

stephentoub
stephentoub
approved these changes
Sep 19, 2022
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM if it fixes the problem.
@filipnavara
filipnavara
commented
Sep 19, 2022•

edited

Loading

Copy link


Copy Markdown

Member
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
@mdh1418
mdh1418
marked this pull request as ready for review
September 19, 2022 16:30
@mdh1418
mdh1418
commented
Sep 19, 2022

Copy link


Copy Markdown

MemberAuthor
This PR was initially only targeting XslCompiledTransformApi tests, so I didn't have a look at all the other test suites that were removed and where the respective TestExclusions weren't honored, but I'll look at the failing XmlSerializerTests tests. Would it make sense to make PRs on a test suite by test suite basis, or trying to tackle them all in this one PR?
@filipnavara
filipnavara
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
I don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines. In fact I started getting them locally for the main branch when building the NativeAOT tests. That's why I started watching this PR since it resolves a sizeable amount of the failures. I appreciate that you started looking into it even if it was not for the same underlying reason.
Btw, you should run the extras pipelines (/azp run runtime-extra-platforms) once you are reasonably sure that the tests from the previous run are now marked correctly.
@MichalStrehovsky
MichalStrehovsky
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
Ah, that explains it. I can understand that it was missed as part of a PR if the extras pipelines were not run but I am quite surprised that there was no issue filed for it based on the rolling builds.
@stephentoub
stephentoub
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
Missed that one (or rather it was not filed yet when I first hit the issue). Thanks.
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
00e9588 to
966cfeaCompareSeptember 20, 2022 04:54
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 20, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
I looked at the comment in tests.proj and it looks like System.XmlSerializer is just not supported on NativeAOT right now.
I also noticed some test failures in two more suites that were removed System.Xml.Xsl.XslCompiledTransformApi.Tests.csproj however there doesn't seem to be a test suite skip in tests.proj, so not sure how these were skipped in the past. They were failing with
System.Xml.XmlSchemaValidatorApiTests.VerifyException : Error while loading assembly
at System.Xml.XmlSchemaValidatorApiTests.ExceptionVerifier..ctor(String assemblyName, ExceptionVerificationFlags flags, ITestOutputHelper output) + 0x5fd
at System.Xml.XmlSchemaValidatorApiTests.TCInitialize..ctor(ITestOutputHelper output) + 0x48
at System.Private.Xml.Tests!<BaseAddress>+0x12ad20d
at System.Reflection.DynamicInvokeInfo.Invoke(Object thisPtr, IntPtr methodToCall, Object[] parameters, BinderBundle binderBundle, Boolean wrapInTargetInvocationException) + 0x150

@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
It's a bit misleading but if you look closely at the stack trace the inner exception is still PlatformNotSupportedException due to unsupported Reflection Emit.
@mdh1418
[tests] Condition XmlSerializerTests on ReflectionEmit
835d515
Tests fail from trimming and MakeGeneric, and XmlSerializer is not supported with NativeAOT
@mdh1418
[tests] Condition problematic XmlSchemaValidatorApiTests classes on R…
103d596
…eflectionEmit
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
966cfea to
103d596CompareSeptember 21, 2022 17:11
@mdh1418
mdh1418
commented
Sep 21, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 21, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 23, 2022
 Closed

@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
This is not fixing the whole problem, but it is still an improvement. Merging.
@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
Actually, I do not think that the condition on the Schema and XmlSerializer test is right (it should not be ReflectionEmit). The Xslt one is good, so let's merge that one at least.
@jkotas
jkotas
merged commit 74c589f
into
dotnet:mainSep 23, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 24, 2022
@mdh1418
mdh1418
deleted the

properly_skip_xslcompiledtransform

branch
October 28, 2022 02:18
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@stephentoubstephentoubstephentoub approved these changes

Assignees

@mdh1418mdh1418

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[iOS] XslCompiledTransform tests failing on devices


5 participants

@mdh1418@filipnavara@MichalStrehovsky@stephentoub@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT by mdh1418 · Pull Request #75730 · dotnet/runtime · GitHub
Skip to content
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT - #75730
Merged
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Sep 23, 2022
Merged
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT#75730
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Conversation
@mdh1418
@mdh1418mdh1418
commented
Sep 15, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#75699
In effort to consolidate System.Private.Xml Test projects, the test suite exclusions within https://github.com/dotnet/runtime/blob/main/src/libraries/tests.proj weren't being honored because the suites no longer existed.
Instead, to skip the tests, [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] is being added at the class level to skip tests where Reflection Emit is needed.
This PR skips tests within XslCompiledTransformAp and XmlSerializerTests
@ghost
ghost
commented
Sep 15, 2022

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@ghostghost
assigned mdh1418Sep 15, 2022
@mdh1418
mdh1418
commented
Sep 15, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
MichalStrehovsky
MichalStrehovsky
reviewed
Sep 16, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj
Outdated


Comment on lines

+624
 to 
+625

 <ItemGroup Condition="(('$(TargetOS)' != 'tvOS' and '$(TargetOS)' != 'iOS') or '$(RunDisablediOSTests)' == 'true') and
 ('$(TestNativeAot)' != 'true' or '$(RunDisabledNativeAotTests)' == 'true')" >
@MichalStrehovskyMichalStrehovskySep 16, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It might be better to put [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] on the individual test classes in these files. It's more consistent with how we do things and more futureproof.
@mdh1418
mdh1418
commented
Sep 16, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 16, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 16, 2022
 Merged

stephentoub
stephentoub
approved these changes
Sep 19, 2022
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM if it fixes the problem.
@filipnavara
filipnavara
commented
Sep 19, 2022•

edited

Loading

Copy link


Copy Markdown

Member
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
@mdh1418
mdh1418
marked this pull request as ready for review
September 19, 2022 16:30
@mdh1418
mdh1418
commented
Sep 19, 2022

Copy link


Copy Markdown

MemberAuthor
This PR was initially only targeting XslCompiledTransformApi tests, so I didn't have a look at all the other test suites that were removed and where the respective TestExclusions weren't honored, but I'll look at the failing XmlSerializerTests tests. Would it make sense to make PRs on a test suite by test suite basis, or trying to tackle them all in this one PR?
@filipnavara
filipnavara
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
I don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines. In fact I started getting them locally for the main branch when building the NativeAOT tests. That's why I started watching this PR since it resolves a sizeable amount of the failures. I appreciate that you started looking into it even if it was not for the same underlying reason.
Btw, you should run the extras pipelines (/azp run runtime-extra-platforms) once you are reasonably sure that the tests from the previous run are now marked correctly.
@MichalStrehovsky
MichalStrehovsky
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
Ah, that explains it. I can understand that it was missed as part of a PR if the extras pipelines were not run but I am quite surprised that there was no issue filed for it based on the rolling builds.
@stephentoub
stephentoub
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
Missed that one (or rather it was not filed yet when I first hit the issue). Thanks.
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
00e9588 to
966cfeaCompareSeptember 20, 2022 04:54
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 20, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
I looked at the comment in tests.proj and it looks like System.XmlSerializer is just not supported on NativeAOT right now.
I also noticed some test failures in two more suites that were removed System.Xml.Xsl.XslCompiledTransformApi.Tests.csproj however there doesn't seem to be a test suite skip in tests.proj, so not sure how these were skipped in the past. They were failing with
System.Xml.XmlSchemaValidatorApiTests.VerifyException : Error while loading assembly
at System.Xml.XmlSchemaValidatorApiTests.ExceptionVerifier..ctor(String assemblyName, ExceptionVerificationFlags flags, ITestOutputHelper output) + 0x5fd
at System.Xml.XmlSchemaValidatorApiTests.TCInitialize..ctor(ITestOutputHelper output) + 0x48
at System.Private.Xml.Tests!<BaseAddress>+0x12ad20d
at System.Reflection.DynamicInvokeInfo.Invoke(Object thisPtr, IntPtr methodToCall, Object[] parameters, BinderBundle binderBundle, Boolean wrapInTargetInvocationException) + 0x150

@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
It's a bit misleading but if you look closely at the stack trace the inner exception is still PlatformNotSupportedException due to unsupported Reflection Emit.
@mdh1418
[tests] Condition XmlSerializerTests on ReflectionEmit
835d515
Tests fail from trimming and MakeGeneric, and XmlSerializer is not supported with NativeAOT
@mdh1418
[tests] Condition problematic XmlSchemaValidatorApiTests classes on R…
103d596
…eflectionEmit
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
966cfea to
103d596CompareSeptember 21, 2022 17:11
@mdh1418
mdh1418
commented
Sep 21, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 21, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 23, 2022
 Closed

@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
This is not fixing the whole problem, but it is still an improvement. Merging.
@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
Actually, I do not think that the condition on the Schema and XmlSerializer test is right (it should not be ReflectionEmit). The Xslt one is good, so let's merge that one at least.
@jkotas
jkotas
merged commit 74c589f
into
dotnet:mainSep 23, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 24, 2022
@mdh1418
mdh1418
deleted the

properly_skip_xslcompiledtransform

branch
October 28, 2022 02:18
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@stephentoubstephentoubstephentoub approved these changes

Assignees

@mdh1418mdh1418

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[iOS] XslCompiledTransform tests failing on devices


5 participants

@mdh1418@filipnavara@MichalStrehovsky@stephentoub@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT by mdh1418 · Pull Request #75730 · dotnet/runtime · GitHub
Skip to content
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT - #75730
Merged
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Sep 23, 2022
Merged
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT#75730
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Conversation
@mdh1418
@mdh1418mdh1418
commented
Sep 15, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#75699
In effort to consolidate System.Private.Xml Test projects, the test suite exclusions within https://github.com/dotnet/runtime/blob/main/src/libraries/tests.proj weren't being honored because the suites no longer existed.
Instead, to skip the tests, [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] is being added at the class level to skip tests where Reflection Emit is needed.
This PR skips tests within XslCompiledTransformAp and XmlSerializerTests
@ghost
ghost
commented
Sep 15, 2022

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@ghostghost
assigned mdh1418Sep 15, 2022
@mdh1418
mdh1418
commented
Sep 15, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
MichalStrehovsky
MichalStrehovsky
reviewed
Sep 16, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj
Outdated


Comment on lines

+624
 to 
+625

 <ItemGroup Condition="(('$(TargetOS)' != 'tvOS' and '$(TargetOS)' != 'iOS') or '$(RunDisablediOSTests)' == 'true') and
 ('$(TestNativeAot)' != 'true' or '$(RunDisabledNativeAotTests)' == 'true')" >
@MichalStrehovskyMichalStrehovskySep 16, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It might be better to put [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] on the individual test classes in these files. It's more consistent with how we do things and more futureproof.
@mdh1418
mdh1418
commented
Sep 16, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 16, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 16, 2022
 Merged

stephentoub
stephentoub
approved these changes
Sep 19, 2022
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM if it fixes the problem.
@filipnavara
filipnavara
commented
Sep 19, 2022•

edited

Loading

Copy link


Copy Markdown

Member
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
@mdh1418
mdh1418
marked this pull request as ready for review
September 19, 2022 16:30
@mdh1418
mdh1418
commented
Sep 19, 2022

Copy link


Copy Markdown

MemberAuthor
This PR was initially only targeting XslCompiledTransformApi tests, so I didn't have a look at all the other test suites that were removed and where the respective TestExclusions weren't honored, but I'll look at the failing XmlSerializerTests tests. Would it make sense to make PRs on a test suite by test suite basis, or trying to tackle them all in this one PR?
@filipnavara
filipnavara
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
I don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines. In fact I started getting them locally for the main branch when building the NativeAOT tests. That's why I started watching this PR since it resolves a sizeable amount of the failures. I appreciate that you started looking into it even if it was not for the same underlying reason.
Btw, you should run the extras pipelines (/azp run runtime-extra-platforms) once you are reasonably sure that the tests from the previous run are now marked correctly.
@MichalStrehovsky
MichalStrehovsky
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
Ah, that explains it. I can understand that it was missed as part of a PR if the extras pipelines were not run but I am quite surprised that there was no issue filed for it based on the rolling builds.
@stephentoub
stephentoub
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
Missed that one (or rather it was not filed yet when I first hit the issue). Thanks.
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
00e9588 to
966cfeaCompareSeptember 20, 2022 04:54
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 20, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
I looked at the comment in tests.proj and it looks like System.XmlSerializer is just not supported on NativeAOT right now.
I also noticed some test failures in two more suites that were removed System.Xml.Xsl.XslCompiledTransformApi.Tests.csproj however there doesn't seem to be a test suite skip in tests.proj, so not sure how these were skipped in the past. They were failing with
System.Xml.XmlSchemaValidatorApiTests.VerifyException : Error while loading assembly
at System.Xml.XmlSchemaValidatorApiTests.ExceptionVerifier..ctor(String assemblyName, ExceptionVerificationFlags flags, ITestOutputHelper output) + 0x5fd
at System.Xml.XmlSchemaValidatorApiTests.TCInitialize..ctor(ITestOutputHelper output) + 0x48
at System.Private.Xml.Tests!<BaseAddress>+0x12ad20d
at System.Reflection.DynamicInvokeInfo.Invoke(Object thisPtr, IntPtr methodToCall, Object[] parameters, BinderBundle binderBundle, Boolean wrapInTargetInvocationException) + 0x150

@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
It's a bit misleading but if you look closely at the stack trace the inner exception is still PlatformNotSupportedException due to unsupported Reflection Emit.
@mdh1418
[tests] Condition XmlSerializerTests on ReflectionEmit
835d515
Tests fail from trimming and MakeGeneric, and XmlSerializer is not supported with NativeAOT
@mdh1418
[tests] Condition problematic XmlSchemaValidatorApiTests classes on R…
103d596
…eflectionEmit
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
966cfea to
103d596CompareSeptember 21, 2022 17:11
@mdh1418
mdh1418
commented
Sep 21, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 21, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 23, 2022
 Closed

@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
This is not fixing the whole problem, but it is still an improvement. Merging.
@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
Actually, I do not think that the condition on the Schema and XmlSerializer test is right (it should not be ReflectionEmit). The Xslt one is good, so let's merge that one at least.
@jkotas
jkotas
merged commit 74c589f
into
dotnet:mainSep 23, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 24, 2022
@mdh1418
mdh1418
deleted the

properly_skip_xslcompiledtransform

branch
October 28, 2022 02:18
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@stephentoubstephentoubstephentoub approved these changes

Assignees

@mdh1418mdh1418

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[iOS] XslCompiledTransform tests failing on devices


5 participants

@mdh1418@filipnavara@MichalStrehovsky@stephentoub@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT by mdh1418 · Pull Request #75730 · dotnet/runtime · GitHub
Skip to content
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT - #75730
Merged
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Sep 23, 2022
Merged
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT#75730
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Conversation
@mdh1418
@mdh1418mdh1418
commented
Sep 15, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#75699
In effort to consolidate System.Private.Xml Test projects, the test suite exclusions within https://github.com/dotnet/runtime/blob/main/src/libraries/tests.proj weren't being honored because the suites no longer existed.
Instead, to skip the tests, [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] is being added at the class level to skip tests where Reflection Emit is needed.
This PR skips tests within XslCompiledTransformAp and XmlSerializerTests
@ghost
ghost
commented
Sep 15, 2022

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@ghostghost
assigned mdh1418Sep 15, 2022
@mdh1418
mdh1418
commented
Sep 15, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
MichalStrehovsky
MichalStrehovsky
reviewed
Sep 16, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj
Outdated


Comment on lines

+624
 to 
+625

 <ItemGroup Condition="(('$(TargetOS)' != 'tvOS' and '$(TargetOS)' != 'iOS') or '$(RunDisablediOSTests)' == 'true') and
 ('$(TestNativeAot)' != 'true' or '$(RunDisabledNativeAotTests)' == 'true')" >
@MichalStrehovskyMichalStrehovskySep 16, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It might be better to put [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] on the individual test classes in these files. It's more consistent with how we do things and more futureproof.
@mdh1418
mdh1418
commented
Sep 16, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 16, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 16, 2022
 Merged

stephentoub
stephentoub
approved these changes
Sep 19, 2022
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM if it fixes the problem.
@filipnavara
filipnavara
commented
Sep 19, 2022•

edited

Loading

Copy link


Copy Markdown

Member
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
@mdh1418
mdh1418
marked this pull request as ready for review
September 19, 2022 16:30
@mdh1418
mdh1418
commented
Sep 19, 2022

Copy link


Copy Markdown

MemberAuthor
This PR was initially only targeting XslCompiledTransformApi tests, so I didn't have a look at all the other test suites that were removed and where the respective TestExclusions weren't honored, but I'll look at the failing XmlSerializerTests tests. Would it make sense to make PRs on a test suite by test suite basis, or trying to tackle them all in this one PR?
@filipnavara
filipnavara
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
I don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines. In fact I started getting them locally for the main branch when building the NativeAOT tests. That's why I started watching this PR since it resolves a sizeable amount of the failures. I appreciate that you started looking into it even if it was not for the same underlying reason.
Btw, you should run the extras pipelines (/azp run runtime-extra-platforms) once you are reasonably sure that the tests from the previous run are now marked correctly.
@MichalStrehovsky
MichalStrehovsky
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
Ah, that explains it. I can understand that it was missed as part of a PR if the extras pipelines were not run but I am quite surprised that there was no issue filed for it based on the rolling builds.
@stephentoub
stephentoub
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
Missed that one (or rather it was not filed yet when I first hit the issue). Thanks.
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
00e9588 to
966cfeaCompareSeptember 20, 2022 04:54
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 20, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
I looked at the comment in tests.proj and it looks like System.XmlSerializer is just not supported on NativeAOT right now.
I also noticed some test failures in two more suites that were removed System.Xml.Xsl.XslCompiledTransformApi.Tests.csproj however there doesn't seem to be a test suite skip in tests.proj, so not sure how these were skipped in the past. They were failing with
System.Xml.XmlSchemaValidatorApiTests.VerifyException : Error while loading assembly
at System.Xml.XmlSchemaValidatorApiTests.ExceptionVerifier..ctor(String assemblyName, ExceptionVerificationFlags flags, ITestOutputHelper output) + 0x5fd
at System.Xml.XmlSchemaValidatorApiTests.TCInitialize..ctor(ITestOutputHelper output) + 0x48
at System.Private.Xml.Tests!<BaseAddress>+0x12ad20d
at System.Reflection.DynamicInvokeInfo.Invoke(Object thisPtr, IntPtr methodToCall, Object[] parameters, BinderBundle binderBundle, Boolean wrapInTargetInvocationException) + 0x150

@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
It's a bit misleading but if you look closely at the stack trace the inner exception is still PlatformNotSupportedException due to unsupported Reflection Emit.
@mdh1418
[tests] Condition XmlSerializerTests on ReflectionEmit
835d515
Tests fail from trimming and MakeGeneric, and XmlSerializer is not supported with NativeAOT
@mdh1418
[tests] Condition problematic XmlSchemaValidatorApiTests classes on R…
103d596
…eflectionEmit
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
966cfea to
103d596CompareSeptember 21, 2022 17:11
@mdh1418
mdh1418
commented
Sep 21, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 21, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 23, 2022
 Closed

@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
This is not fixing the whole problem, but it is still an improvement. Merging.
@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
Actually, I do not think that the condition on the Schema and XmlSerializer test is right (it should not be ReflectionEmit). The Xslt one is good, so let's merge that one at least.
@jkotas
jkotas
merged commit 74c589f
into
dotnet:mainSep 23, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 24, 2022
@mdh1418
mdh1418
deleted the

properly_skip_xslcompiledtransform

branch
October 28, 2022 02:18
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@stephentoubstephentoubstephentoub approved these changes

Assignees

@mdh1418mdh1418

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[iOS] XslCompiledTransform tests failing on devices


5 participants

@mdh1418@filipnavara@MichalStrehovsky@stephentoub@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT by mdh1418 · Pull Request #75730 · dotnet/runtime · GitHub
Skip to content
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT - #75730
Merged
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Sep 23, 2022
Merged
[tests] Skip XslCompiledTransformApi tests on iOS, tvOS, and when testing native AOT#75730
jkotas merged 6 commits into
dotnet:mainfrom 
mdh1418:properly_skip_xslcompiledtransform
Conversation
@mdh1418
@mdh1418mdh1418
commented
Sep 15, 2022•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#75699
In effort to consolidate System.Private.Xml Test projects, the test suite exclusions within https://github.com/dotnet/runtime/blob/main/src/libraries/tests.proj weren't being honored because the suites no longer existed.
Instead, to skip the tests, [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] is being added at the class level to skip tests where Reflection Emit is needed.
This PR skips tests within XslCompiledTransformAp and XmlSerializerTests
@ghost
ghost
commented
Sep 15, 2022

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@ghostghost
assigned mdh1418Sep 15, 2022
@mdh1418
mdh1418
commented
Sep 15, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 15, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
MichalStrehovsky
MichalStrehovsky
reviewed
Sep 16, 2022
View reviewed changes
Comment threadsrc/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj
Outdated


Comment on lines

+624
 to 
+625

 <ItemGroup Condition="(('$(TargetOS)' != 'tvOS' and '$(TargetOS)' != 'iOS') or '$(RunDisablediOSTests)' == 'true') and
 ('$(TestNativeAot)' != 'true' or '$(RunDisabledNativeAotTests)' == 'true')" >
@MichalStrehovskyMichalStrehovskySep 16, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It might be better to put [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] on the individual test classes in these files. It's more consistent with how we do things and more futureproof.
@mdh1418
mdh1418
commented
Sep 16, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 16, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 16, 2022
 Merged

stephentoub
stephentoub
approved these changes
Sep 19, 2022
View reviewed changes
@stephentoubstephentoub
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM if it fixes the problem.
@filipnavara
filipnavara
commented
Sep 19, 2022•

edited

Loading

Copy link


Copy Markdown

Member
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
@mdh1418
mdh1418
marked this pull request as ready for review
September 19, 2022 16:30
@mdh1418
mdh1418
commented
Sep 19, 2022

Copy link


Copy Markdown

MemberAuthor
This PR was initially only targeting XslCompiledTransformApi tests, so I didn't have a look at all the other test suites that were removed and where the respective TestExclusions weren't honored, but I'll look at the failing XmlSerializerTests tests. Would it make sense to make PRs on a test suite by test suite basis, or trying to tackle them all in this one PR?
@filipnavara
filipnavara
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
I don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines. In fact I started getting them locally for the main branch when building the NativeAOT tests. That's why I started watching this PR since it resolves a sizeable amount of the failures. I appreciate that you started looking into it even if it was not for the same underlying reason.
Btw, you should run the extras pipelines (/azp run runtime-extra-platforms) once you are reasonably sure that the tests from the previous run are now marked correctly.
@MichalStrehovsky
MichalStrehovsky
commented
Sep 19, 2022

Copy link


Copy Markdown

Member
don't know what triggered it but my totally unrelated PR started seeing the same errors on the NativeAOT pipelines
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
The test projects (mentioned in the diffs to src/libraries/tests.proj in this PR) got deleted and folded into another project that is not excluded. So tests we weren't even compiling before started failing.
Ah, that explains it. I can understand that it was missed as part of a PR if the extras pipelines were not run but I am quite surprised that there was no issue filed for it based on the rolling builds.
@stephentoub
stephentoub
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
#75699 (comment)
Missed that one (or rather it was not filed yet when I first hit the issue). Thanks.
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
00e9588 to
966cfeaCompareSeptember 20, 2022 04:54
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 20, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@mdh1418
mdh1418
commented
Sep 20, 2022

Copy link


Copy Markdown

MemberAuthor
It fixes half of the problem. Other half of the NativeAOT tests still fail and need similar treatment.
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
I looked at the comment in tests.proj and it looks like System.XmlSerializer is just not supported on NativeAOT right now.
I also noticed some test failures in two more suites that were removed System.Xml.Xsl.XslCompiledTransformApi.Tests.csproj however there doesn't seem to be a test suite skip in tests.proj, so not sure how these were skipped in the past. They were failing with
System.Xml.XmlSchemaValidatorApiTests.VerifyException : Error while loading assembly
at System.Xml.XmlSchemaValidatorApiTests.ExceptionVerifier..ctor(String assemblyName, ExceptionVerificationFlags flags, ITestOutputHelper output) + 0x5fd
at System.Xml.XmlSchemaValidatorApiTests.TCInitialize..ctor(ITestOutputHelper output) + 0x48
at System.Private.Xml.Tests!<BaseAddress>+0x12ad20d
at System.Reflection.DynamicInvokeInfo.Invoke(Object thisPtr, IntPtr methodToCall, Object[] parameters, BinderBundle binderBundle, Boolean wrapInTargetInvocationException) + 0x150

@filipnavara
filipnavara
commented
Sep 20, 2022

Copy link


Copy Markdown

Member
I looked at the test failures more closely, and it doesn't look obviously System.Reflection.Emit to me.

Seeing errors like System.InvalidOperationException : There is an error in XML document (2, 14).
It's a bit misleading but if you look closely at the stack trace the inner exception is still PlatformNotSupportedException due to unsupported Reflection Emit.
@mdh1418
[tests] Condition XmlSerializerTests on ReflectionEmit
835d515
Tests fail from trimming and MakeGeneric, and XmlSerializer is not supported with NativeAOT
@mdh1418
[tests] Condition problematic XmlSchemaValidatorApiTests classes on R…
103d596
…eflectionEmit
@mdh1418
mdh1418force-pushed
the
properly_skip_xslcompiledtransform
branch
from
966cfea to
103d596CompareSeptember 21, 2022 17:11
@mdh1418
mdh1418
commented
Sep 21, 2022

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Sep 21, 2022

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@filipnavarafilipnavara
mentioned this pull request
Sep 23, 2022
 Closed

@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
This is not fixing the whole problem, but it is still an improvement. Merging.
@jkotas
jkotas
commented
Sep 23, 2022

Copy link


Copy Markdown

Member
Actually, I do not think that the condition on the Schema and XmlSerializer test is right (it should not be ReflectionEmit). The Xslt one is good, so let's merge that one at least.
@jkotas
jkotas
merged commit 74c589f
into
dotnet:mainSep 23, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 24, 2022
@mdh1418
mdh1418
deleted the

properly_skip_xslcompiledtransform

branch
October 28, 2022 02:18
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@stephentoubstephentoubstephentoub approved these changes

Assignees

@mdh1418mdh1418

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[iOS] XslCompiledTransform tests failing on devices


5 participants

@mdh1418@filipnavara@MichalStrehovsky@stephentoub@jkotas