Convert all tests under baseservices to the merged test infrastructure - #91560

Merged
trylek merged 31 commits into
dotnet:mainfrom
trylek:MergeBaseServices
Oct 30, 2023
Merged

Convert all tests under baseservices to the merged test infrastructure#91560
trylek merged 31 commits into
dotnet:mainfrom
trylek:MergeBaseServices

Conversation

@trylek

@trylektrylek commented Sep 4, 2023

Copy link
Copy Markdown
Member

This change converts all remaining tests under baseservices to use the merged test model.
Apart from a few tiny tweaks I found a somewhat funny case in the test

baseservices/RuntimeConfiguration/TestConfig.csproj

This test had two problematic characteristics w.r.t. test merging - it was launching itself as
a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally
didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"
app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were
actually superfluous because the test uses two additional attributes (ConfigProperty and
EnvVar) to mark the "interesting" methods.

Thanks

Tomas

/cc @dotnet/runtime-infrastructure

@ghostghost assigned trylekSep 4, 2023
@ghost

ghost commented Sep 4, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata
See info in area-owners.md if you want to be subscribed.

Issue Details

This change converts all remaining tests under baseservices to use the merged test model.
Apart from a few tiny tweaks I found a somewhat funny case in the test

baseservices/RuntimeConfiguration/TestConfig.csproj

This test had two problematic characteristics w.r.t. test merging - it was launching itself as
a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally
didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"
app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were
actually superfluous because the test uses two additional attributes (ConfigProperty and
EnvVar) to mark the "interesting" methods.

Thanks

Tomas

/cc @dotnet/runtime-infrastructure

Author:trylek
Assignees:trylek
Labels:

area-System.Reflection.Metadata

Milestone:-

@markples

markples commented Sep 5, 2023

Copy link
Copy Markdown
Contributor

How have you checked whether all tests have been preserved across the conversion? It seems like the changes to how the internal test harness work could impact the number of reported tests and make this difficult. (If so, do you have recommendations on how to review this with that in mind?)

@markples

markples commented Sep 5, 2023

Copy link
Copy Markdown
Contributor

For the tests that define a Main, I believe that BuildAsStandalone will be broken since it will try to generate an entry point. I think setting ReferenceXUnitWrapperGenerator to false will fix this.

@trylek

trylek commented Sep 5, 2023

Copy link
Copy Markdown
MemberAuthor

Well, in the local testing I have so far just monitored the number of tests; in fact even that has changed slightly as I split one or two tests to multiple [Fact]s (I tried to clearly separate those individual changes from the bulk conversions as individual commits on the PR). I'm still working on fixing the BasicTestWithMcj test, once I'm done with that, I think I'll compare the output xml files, I think the last time I wrote a one-off managed app for the purpose, I need to take a look if I still have it around but it's about half an hour's work to write.

@trylek

trylek commented Sep 5, 2023

Copy link
Copy Markdown
MemberAuthor

Thanks for the suggestion regarding ReferenceXUnitWrapperGenerator, that should make it nicely visible which tests are problematic for some future cleanup wave.

markples
markples reviewed Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/Directory.Build.targets Outdated
markples
markples reviewed Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfig.csproj Outdated
markples
markples reviewed Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfigTester.csproj Outdated
markples
markples reviewed Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/ilasm_ildasm/regression/vswhidbey305155/305155.cs Outdated
@stevehartersteveharter added the area-Infrastructure-coreclr Only use for closed issues label Sep 6, 2023
@ghost

ghost commented Sep 6, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

This change converts all remaining tests under baseservices to use the merged test model.
Apart from a few tiny tweaks I found a somewhat funny case in the test

baseservices/RuntimeConfiguration/TestConfig.csproj

This test had two problematic characteristics w.r.t. test merging - it was launching itself as
a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally
didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"
app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were
actually superfluous because the test uses two additional attributes (ConfigProperty and
EnvVar) to mark the "interesting" methods.

Thanks

Tomas

/cc @dotnet/runtime-infrastructure

Author:trylek
Assignees:trylek
Labels:

area-System.Reflection.Metadata, area-Infrastructure-coreclr

Milestone:-

@trylek

trylek commented Sep 6, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

@markples - I have stood up the xml differ and I have managed to convince myself that the results are fine:

Left-only tests (5 total):
--------------------------
baseservices\exceptions\exceptioninterop\ExceptionInterop_ro\ExceptionInterop_ro
baseservices\exceptions\exceptioninterop\ExceptionInterop\ExceptionInterop
baseservices\exceptions\simple\ParallelCrashMainThread\ParallelCrashMainThread
baseservices\exceptions\simple\ParallelCrashWorkerThreads\ParallelCrashWorkerThreads
baseservices\TieredCompilation\BasicTestWithMcj\RunBasicTestWithMcj
Right-only tests (24 total):
----------------------------
_ExceptionInterop_ro::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ExceptionInterop::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThread()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThreadAndWorkerThreads()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashWorkerThreads()
baseservices\exceptions\stackoverflow\stackoverflow\stackoverflow
baseservices\exceptions\stackoverflow\stackoverflow3\stackoverflow3
baseservices\exceptions\StackTracePreserve\StackTracePreserveTests\StackTracePreserveTests
baseservices\exceptions\unhandled\unhandledTester\unhandledTester
baseservices\finalization\CriticalFinalizer\CriticalFinalizer
baseservices\ilasm_ildasm\regression\vswhidbey305155\305155\305155
baseservices\mono\runningmono\runningmono
baseservices\multidimmarray\enum\enum
baseservices\RuntimeConfiguration\TestConfigTester\TestConfigTester
baseservices\varargs\varargsupport_r\varargsupport_r
baseservices\varargs\varargsupport\varargsupport

There are several interesting observations based on this diff.

  • We have an ugly discontinuity regarding test naming based on whether a given test project contains one or multiple [Fact] clauses - when there are multiple Fact clauses, we drop the test path information and just name the test based on the method. I think we should probably concatenate these two or concatenate the test dll / script path with the method name or something, the completely different representation of tests with multiple test cases is confusing and makes it harder for developers to correlate their test failures to the source / binary code.

  • We have a subtle difference with regard to reporting tests blocked in issues.targets. The legacy infrastructure filtered them out at the msbuild level so that they never made it into the XUnit wrappers and simply aren't reported in any manner in the results. With the merged infrastructure we filter them out in a different manner and in the report we end up reporting them as "skipped". I think that's actually more appropriate, the only downside is that today there's no indication in the xml results file that the test in question was skipped due to an issues.targets clause; I have filed CoreCLR test infra: improve "Skip" messages for tests blocked in issues.targets #91562 to fix that. In the above list, that pertains to the "Right-only tests" 305155, runningmono, enum, varargsupport_r and varargsupport.

  • For stackoverflow, stackoverflow3 or unhandled, I believe these need fixing in the merged wrapper generator. These projects are marked as BuildOnly and shouldn't be actually run, the xml result file pretends it has run the corresponding cmd / sh script but that is nonsense, there's no cmd / sh script getting generated for these. I'll follow up with Jeremy on fixing this, I'm not sure to what extent it's blocking for this particular PR. (In practice the projects actually getting run are stackoverflowtester and unhandledTester introduced as part of this PR.)

Thanks

Tomas

@trylek

trylek commented Sep 6, 2023

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Sep 6, 2023

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

@build-analysisbuild-analysisBot mentioned this pull request Sep 7, 2023
Closed
@markples

markples commented Sep 8, 2023

Copy link
Copy Markdown
Contributor

Thanks Tomas!

  • re: discontinuity in naming: I believe that we ended up in this position because we had a legacy naming system (path/project) which couldn't differentiate between multiple tests in a project, and the "new" way is closer to (or the same as?) standard xunit naming. The discontinuity was kept to minimize the number of tests that were renamed as part of adding merged test groups. However, with the eventual hope of merging the builds too, perhaps moving more towards xunit-style (always use it) would have less eventual churn that going back to incorporating paths? It is important for now that the assembly name is there (though it appears to be?) because we don't dedup class/test names.
  • agreed on your analysis and new work item
  • I believe that these are being caused by the RequiresProcessIsolation attribute, for which the processing doesn't check for BuildOnly, etc. Is this needed in the final iteration? If not, then instead of excluding BuildOnly, perhaps Dir.build.targets could consider this an error.
    • It seems odd that the wrapper is reporting it as run when it doesn't exist. It looks like it is this code, which appears to be necessary but could be a point where tests are discarded incorrectly.

@trylek

trylek commented Oct 12, 2023

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@trylek

trylek commented Oct 12, 2023

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-extra-platforms

@azure-pipelines

azure-pipelinesBot commented Oct 12, 2023

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

1 similar comment
@azure-pipelines

azure-pipelinesBot commented Oct 12, 2023

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

@trylek

trylek commented Oct 12, 2023

Copy link
Copy Markdown
MemberAuthor

@markples - I have rebased this change against the latest main and I believe I have addressed all your pending PR feedback, can you please take another look when you have a chance?

Thanks Tomas

@build-analysisbuild-analysisBot mentioned this pull request Oct 13, 2023
Closed
@trylek
Fix visibility in CriticalFinalizer test
2fe3767
@trylek
Simplify RuntimeConfiguration/TestConfig
e0bacc0
@trylek
Clean up TieredCompilation tests
65d2a7a
@trylek
Convert istypeequivalent to use ConditionalFact clauses
6897201
@trylek
Fix visibility in RuntimeHelpersTests
c72d876
@trylek
Add CoreCLRTestLibrary as a dependency of istypeequivalent
573730a
@trylek
Fix merged behavior of test448035
c43b1d7
@trylek
Fix entrypoint in 305155
c2d27d4
@trylek
Modify TestConfig to use a separate TestConfigTester app
e41e8f2
@trylek
Additional fixes to TestConfig / TestConfigTester
cbf2417
@trylek
Mechanically merge all remaining tests under baseservices
d30f9c6
@trylek
Fix BasicTestWithMcj, address initial Mark's PR feedback
02ffa17
@trylek
Remove superfluous OutputType=Library annotations per Marks' PR feedback
c9854b2
@trylek
Fix the baseservices/exceptions/unhandled test
02e17cf
@trylek
Fix stackoverflow3 and unhandled exception tests
be2a499
@trylek
Remove unnecessary check from Directory.Build.targets
2d85c95
@trylek
Fix stackoverflowtester per Mark's PR feedback
38e77a4
@trylek

trylek commented Oct 29, 2023

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Oct 29, 2023

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

@trylek
trylek merged commit 1a19c09 into dotnet:mainOct 30, 2023
@trylek
trylek deleted the MergeBaseServices branch October 30, 2023 01:12
gbalykov added a commit to gbalykov/runtime that referenced this pull request Oct 31, 2023
@gbalykov
Tests build fix with BuildAsStandalone
1cdfc1b
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before dotnet#91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@gbalykovgbalykov mentioned this pull request Oct 31, 2023
Merged
@jtschusterjtschuster mentioned this pull request Nov 1, 2023
Closed
trylek pushed a commit that referenced this pull request Nov 1, 2023
@gbalykov
Tests build fix with BuildAsStandalone (#94208)
194fc5f
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before #91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@BruceForstallBruceForstall mentioned this pull request Nov 6, 2023
Closed
@ghostghost locked as resolved and limited conversation to collaborators Nov 29, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ivdiazsaivdiazsaAwaiting requested review from ivdiazsa
1 more reviewer
@markplesmarkplesmarkples approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@trylektrylek

Labels

area-Infrastructure-coreclrOnly use for closed issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@trylek@markples@steveharter
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Convert all tests under baseservices to the merged test infrastructure - #91560
Merged
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Oct 30, 2023
Merged
Convert all tests under baseservices to the merged test infrastructure#91560
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Conversation
@trylek
@trylektrylek
commented
Sep 4, 2023

Copy link


Copy Markdown

Member
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
@ghostghost
assigned trylekSep 4, 2023
@ghost
ghost
commented
Sep 4, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata
Milestone:-
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
How have you checked whether all tests have been preserved across the conversion? It seems like the changes to how the internal test harness work could impact the number of reported tests and make this difficult. (If so, do you have recommendations on how to review this with that in mind?)
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
For the tests that define a Main, I believe that BuildAsStandalone will be broken since it will try to generate an entry point. I think setting ReferenceXUnitWrapperGenerator to false will fix this.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Well, in the local testing I have so far just monitored the number of tests; in fact even that has changed slightly as I split one or two tests to multiple [Fact]s (I tried to clearly separate those individual changes from the bulk conversions as individual commits on the PR). I'm still working on fixing the BasicTestWithMcj test, once I'm done with that, I think I'll compare the output xml files, I think the last time I wrote a one-off managed app for the purpose, I need to take a look if I still have it around but it's about half an hour's work to write.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the suggestion regarding ReferenceXUnitWrapperGenerator, that should make it nicely visible which tests are problematic for some future cleanup wave.
markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/Directory.Build.targets
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfig.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfigTester.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/ilasm_ildasm/regression/vswhidbey305155/305155.cs
Outdated

@stevehartersteveharter
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Sep 6, 2023
@ghost
ghost
commented
Sep 6, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @hoyosjs

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata, area-Infrastructure-coreclr
Milestone:-
@trylek
trylek
commented
Sep 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@markples - I have stood up the xml differ and I have managed to convince myself that the results are fine:
Left-only tests (5 total):
--------------------------
baseservices\exceptions\exceptioninterop\ExceptionInterop_ro\ExceptionInterop_ro
baseservices\exceptions\exceptioninterop\ExceptionInterop\ExceptionInterop
baseservices\exceptions\simple\ParallelCrashMainThread\ParallelCrashMainThread
baseservices\exceptions\simple\ParallelCrashWorkerThreads\ParallelCrashWorkerThreads
baseservices\TieredCompilation\BasicTestWithMcj\RunBasicTestWithMcj
Right-only tests (24 total):
----------------------------
_ExceptionInterop_ro::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ExceptionInterop::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThread()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThreadAndWorkerThreads()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashWorkerThreads()
baseservices\exceptions\stackoverflow\stackoverflow\stackoverflow
baseservices\exceptions\stackoverflow\stackoverflow3\stackoverflow3
baseservices\exceptions\StackTracePreserve\StackTracePreserveTests\StackTracePreserveTests
baseservices\exceptions\unhandled\unhandledTester\unhandledTester
baseservices\finalization\CriticalFinalizer\CriticalFinalizer
baseservices\ilasm_ildasm\regression\vswhidbey305155\305155\305155
baseservices\mono\runningmono\runningmono
baseservices\multidimmarray\enum\enum
baseservices\RuntimeConfiguration\TestConfigTester\TestConfigTester
baseservices\varargs\varargsupport_r\varargsupport_r
baseservices\varargs\varargsupport\varargsupport

There are several interesting observations based on this diff.
  • We have an ugly discontinuity regarding test naming based on whether a given test project contains one or multiple [Fact] clauses - when there are multiple Fact clauses, we drop the test path information and just name the test based on the method. I think we should probably concatenate these two or concatenate the test dll / script path with the method name or something, the completely different representation of tests with multiple test cases is confusing and makes it harder for developers to correlate their test failures to the source / binary code.
  • We have a subtle difference with regard to reporting tests blocked in issues.targets. The legacy infrastructure filtered them out at the msbuild level so that they never made it into the XUnit wrappers and simply aren't reported in any manner in the results. With the merged infrastructure we filter them out in a different manner and in the report we end up reporting them as "skipped". I think that's actually more appropriate, the only downside is that today there's no indication in the xml results file that the test in question was skipped due to an issues.targets clause; I have filed CoreCLR test infra: improve "Skip" messages for tests blocked in issues.targets #91562 to fix that. In the above list, that pertains to the "Right-only tests" 305155, runningmono, enum, varargsupport_r and varargsupport.
  • For stackoverflow, stackoverflow3 or unhandled, I believe these need fixing in the merged wrapper generator. These projects are marked as BuildOnly and shouldn't be actually run, the xml result file pretends it has run the corresponding cmd / sh script but that is nonsense, there's no cmd / sh script getting generated for these. I'll follow up with Jeremy on fixing this, I'm not sure to what extent it's blocking for this particular PR. (In practice the projects actually getting run are stackoverflowtester and unhandledTester introduced as part of this PR.)
Thanks
Tomas
@trylek
trylek
commented
Sep 6, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 6, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 7, 2023

Closed

@markples
markples
commented
Sep 8, 2023

Copy link


Copy Markdown

Contributor
Thanks Tomas!
  • re: discontinuity in naming: I believe that we ended up in this position because we had a legacy naming system (path/project) which couldn't differentiate between multiple tests in a project, and the "new" way is closer to (or the same as?) standard xunit naming. The discontinuity was kept to minimize the number of tests that were renamed as part of adding merged test groups. However, with the eventual hope of merging the builds too, perhaps moving more towards xunit-style (always use it) would have less eventual churn that going back to incorporating paths? It is important for now that the assembly name is there (though it appears to be?) because we don't dedup class/test names.
  • agreed on your analysis and new work item
  • I believe that these are being caused by the RequiresProcessIsolation attribute, for which the processing doesn't check for BuildOnly, etc. Is this needed in the final iteration? If not, then instead of excluding BuildOnly, perhaps Dir.build.targets could consider this an error.

    • It seems odd that the wrapper is reporting it as run when it doesn't exist. It looks like it is this code, which appears to be necessary but could be a point where tests are discarded incorrectly.
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
@markples - I have rebased this change against the latest main and I believe I have addressed all your pending PR feedback, can you please take another look when you have a chance?
Thanks Tomas
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 13, 2023

Closed

@trylek
Fix visibility in CriticalFinalizer test
2fe3767
@trylek
Simplify RuntimeConfiguration/TestConfig
e0bacc0
@trylek
Clean up TieredCompilation tests
65d2a7a
@trylek
Convert istypeequivalent to use ConditionalFact clauses
6897201
@trylek
Fix visibility in RuntimeHelpersTests
c72d876
@trylek
Add CoreCLRTestLibrary as a dependency of istypeequivalent
573730a
@trylek
Fix merged behavior of test448035
c43b1d7
@trylek
Fix entrypoint in 305155
c2d27d4
@trylek
Modify TestConfig to use a separate TestConfigTester app
e41e8f2
@trylek
Additional fixes to TestConfig / TestConfigTester
cbf2417
@trylek
Mechanically merge all remaining tests under baseservices
d30f9c6
@trylek
Fix BasicTestWithMcj, address initial Mark's PR feedback
02ffa17
@trylek
Remove superfluous OutputType=Library annotations per Marks' PR feedback
c9854b2
@trylek
Fix the baseservices/exceptions/unhandled test
02e17cf
@trylek
Fix stackoverflow3 and unhandled exception tests
be2a499
@trylek
Remove unnecessary check from Directory.Build.targets
2d85c95
@trylek
Fix stackoverflowtester per Mark's PR feedback
38e77a4
@trylek
trylek
commented
Oct 29, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 29, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
merged commit 1a19c09
into
dotnet:mainOct 30, 2023
@trylek
trylek
deleted the

MergeBaseServices

branch
October 30, 2023 01:12
gbalykov added a commit
to gbalykov/runtime
that referenced
this pull request
Oct 31, 2023
@gbalykov
Tests build fix with BuildAsStandalone
1cdfc1b
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before dotnet#91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@gbalykovgbalykov
mentioned this pull request
Oct 31, 2023
 Merged

@jtschusterjtschuster
mentioned this pull request
Nov 1, 2023
 Closed

trylek pushed a commit
that referenced
this pull request
Nov 1, 2023
@gbalykov
Tests build fix with BuildAsStandalone (#94208)
194fc5f
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before #91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@BruceForstallBruceForstall
mentioned this pull request
Nov 6, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 29, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ivdiazsaivdiazsaAwaiting requested review from ivdiazsa

1 more reviewer

@markplesmarkplesmarkples approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@trylektrylek

Labels

area-Infrastructure-coreclrOnly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@trylek@markples@steveharter

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Convert all tests under baseservices to the merged test infrastructure - #91560
Merged
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Oct 30, 2023
Merged
Convert all tests under baseservices to the merged test infrastructure#91560
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Conversation
@trylek
@trylektrylek
commented
Sep 4, 2023

Copy link


Copy Markdown

Member
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
@ghostghost
assigned trylekSep 4, 2023
@ghost
ghost
commented
Sep 4, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata
Milestone:-
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
How have you checked whether all tests have been preserved across the conversion? It seems like the changes to how the internal test harness work could impact the number of reported tests and make this difficult. (If so, do you have recommendations on how to review this with that in mind?)
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
For the tests that define a Main, I believe that BuildAsStandalone will be broken since it will try to generate an entry point. I think setting ReferenceXUnitWrapperGenerator to false will fix this.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Well, in the local testing I have so far just monitored the number of tests; in fact even that has changed slightly as I split one or two tests to multiple [Fact]s (I tried to clearly separate those individual changes from the bulk conversions as individual commits on the PR). I'm still working on fixing the BasicTestWithMcj test, once I'm done with that, I think I'll compare the output xml files, I think the last time I wrote a one-off managed app for the purpose, I need to take a look if I still have it around but it's about half an hour's work to write.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the suggestion regarding ReferenceXUnitWrapperGenerator, that should make it nicely visible which tests are problematic for some future cleanup wave.
markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/Directory.Build.targets
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfig.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfigTester.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/ilasm_ildasm/regression/vswhidbey305155/305155.cs
Outdated

@stevehartersteveharter
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Sep 6, 2023
@ghost
ghost
commented
Sep 6, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @hoyosjs

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata, area-Infrastructure-coreclr
Milestone:-
@trylek
trylek
commented
Sep 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@markples - I have stood up the xml differ and I have managed to convince myself that the results are fine:
Left-only tests (5 total):
--------------------------
baseservices\exceptions\exceptioninterop\ExceptionInterop_ro\ExceptionInterop_ro
baseservices\exceptions\exceptioninterop\ExceptionInterop\ExceptionInterop
baseservices\exceptions\simple\ParallelCrashMainThread\ParallelCrashMainThread
baseservices\exceptions\simple\ParallelCrashWorkerThreads\ParallelCrashWorkerThreads
baseservices\TieredCompilation\BasicTestWithMcj\RunBasicTestWithMcj
Right-only tests (24 total):
----------------------------
_ExceptionInterop_ro::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ExceptionInterop::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThread()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThreadAndWorkerThreads()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashWorkerThreads()
baseservices\exceptions\stackoverflow\stackoverflow\stackoverflow
baseservices\exceptions\stackoverflow\stackoverflow3\stackoverflow3
baseservices\exceptions\StackTracePreserve\StackTracePreserveTests\StackTracePreserveTests
baseservices\exceptions\unhandled\unhandledTester\unhandledTester
baseservices\finalization\CriticalFinalizer\CriticalFinalizer
baseservices\ilasm_ildasm\regression\vswhidbey305155\305155\305155
baseservices\mono\runningmono\runningmono
baseservices\multidimmarray\enum\enum
baseservices\RuntimeConfiguration\TestConfigTester\TestConfigTester
baseservices\varargs\varargsupport_r\varargsupport_r
baseservices\varargs\varargsupport\varargsupport

There are several interesting observations based on this diff.
  • We have an ugly discontinuity regarding test naming based on whether a given test project contains one or multiple [Fact] clauses - when there are multiple Fact clauses, we drop the test path information and just name the test based on the method. I think we should probably concatenate these two or concatenate the test dll / script path with the method name or something, the completely different representation of tests with multiple test cases is confusing and makes it harder for developers to correlate their test failures to the source / binary code.
  • We have a subtle difference with regard to reporting tests blocked in issues.targets. The legacy infrastructure filtered them out at the msbuild level so that they never made it into the XUnit wrappers and simply aren't reported in any manner in the results. With the merged infrastructure we filter them out in a different manner and in the report we end up reporting them as "skipped". I think that's actually more appropriate, the only downside is that today there's no indication in the xml results file that the test in question was skipped due to an issues.targets clause; I have filed CoreCLR test infra: improve "Skip" messages for tests blocked in issues.targets #91562 to fix that. In the above list, that pertains to the "Right-only tests" 305155, runningmono, enum, varargsupport_r and varargsupport.
  • For stackoverflow, stackoverflow3 or unhandled, I believe these need fixing in the merged wrapper generator. These projects are marked as BuildOnly and shouldn't be actually run, the xml result file pretends it has run the corresponding cmd / sh script but that is nonsense, there's no cmd / sh script getting generated for these. I'll follow up with Jeremy on fixing this, I'm not sure to what extent it's blocking for this particular PR. (In practice the projects actually getting run are stackoverflowtester and unhandledTester introduced as part of this PR.)
Thanks
Tomas
@trylek
trylek
commented
Sep 6, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 6, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 7, 2023

Closed

@markples
markples
commented
Sep 8, 2023

Copy link


Copy Markdown

Contributor
Thanks Tomas!
  • re: discontinuity in naming: I believe that we ended up in this position because we had a legacy naming system (path/project) which couldn't differentiate between multiple tests in a project, and the "new" way is closer to (or the same as?) standard xunit naming. The discontinuity was kept to minimize the number of tests that were renamed as part of adding merged test groups. However, with the eventual hope of merging the builds too, perhaps moving more towards xunit-style (always use it) would have less eventual churn that going back to incorporating paths? It is important for now that the assembly name is there (though it appears to be?) because we don't dedup class/test names.
  • agreed on your analysis and new work item
  • I believe that these are being caused by the RequiresProcessIsolation attribute, for which the processing doesn't check for BuildOnly, etc. Is this needed in the final iteration? If not, then instead of excluding BuildOnly, perhaps Dir.build.targets could consider this an error.

    • It seems odd that the wrapper is reporting it as run when it doesn't exist. It looks like it is this code, which appears to be necessary but could be a point where tests are discarded incorrectly.
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
@markples - I have rebased this change against the latest main and I believe I have addressed all your pending PR feedback, can you please take another look when you have a chance?
Thanks Tomas
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 13, 2023

Closed

@trylek
Fix visibility in CriticalFinalizer test
2fe3767
@trylek
Simplify RuntimeConfiguration/TestConfig
e0bacc0
@trylek
Clean up TieredCompilation tests
65d2a7a
@trylek
Convert istypeequivalent to use ConditionalFact clauses
6897201
@trylek
Fix visibility in RuntimeHelpersTests
c72d876
@trylek
Add CoreCLRTestLibrary as a dependency of istypeequivalent
573730a
@trylek
Fix merged behavior of test448035
c43b1d7
@trylek
Fix entrypoint in 305155
c2d27d4
@trylek
Modify TestConfig to use a separate TestConfigTester app
e41e8f2
@trylek
Additional fixes to TestConfig / TestConfigTester
cbf2417
@trylek
Mechanically merge all remaining tests under baseservices
d30f9c6
@trylek
Fix BasicTestWithMcj, address initial Mark's PR feedback
02ffa17
@trylek
Remove superfluous OutputType=Library annotations per Marks' PR feedback
c9854b2
@trylek
Fix the baseservices/exceptions/unhandled test
02e17cf
@trylek
Fix stackoverflow3 and unhandled exception tests
be2a499
@trylek
Remove unnecessary check from Directory.Build.targets
2d85c95
@trylek
Fix stackoverflowtester per Mark's PR feedback
38e77a4
@trylek
trylek
commented
Oct 29, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 29, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
merged commit 1a19c09
into
dotnet:mainOct 30, 2023
@trylek
trylek
deleted the

MergeBaseServices

branch
October 30, 2023 01:12
gbalykov added a commit
to gbalykov/runtime
that referenced
this pull request
Oct 31, 2023
@gbalykov
Tests build fix with BuildAsStandalone
1cdfc1b
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before dotnet#91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@gbalykovgbalykov
mentioned this pull request
Oct 31, 2023
 Merged

@jtschusterjtschuster
mentioned this pull request
Nov 1, 2023
 Closed

trylek pushed a commit
that referenced
this pull request
Nov 1, 2023
@gbalykov
Tests build fix with BuildAsStandalone (#94208)
194fc5f
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before #91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@BruceForstallBruceForstall
mentioned this pull request
Nov 6, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 29, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ivdiazsaivdiazsaAwaiting requested review from ivdiazsa

1 more reviewer

@markplesmarkplesmarkples approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@trylektrylek

Labels

area-Infrastructure-coreclrOnly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@trylek@markples@steveharter

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Convert all tests under baseservices to the merged test infrastructure - #91560
Merged
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Oct 30, 2023
Merged
Convert all tests under baseservices to the merged test infrastructure#91560
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Conversation
@trylek
@trylektrylek
commented
Sep 4, 2023

Copy link


Copy Markdown

Member
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
@ghostghost
assigned trylekSep 4, 2023
@ghost
ghost
commented
Sep 4, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata
Milestone:-
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
How have you checked whether all tests have been preserved across the conversion? It seems like the changes to how the internal test harness work could impact the number of reported tests and make this difficult. (If so, do you have recommendations on how to review this with that in mind?)
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
For the tests that define a Main, I believe that BuildAsStandalone will be broken since it will try to generate an entry point. I think setting ReferenceXUnitWrapperGenerator to false will fix this.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Well, in the local testing I have so far just monitored the number of tests; in fact even that has changed slightly as I split one or two tests to multiple [Fact]s (I tried to clearly separate those individual changes from the bulk conversions as individual commits on the PR). I'm still working on fixing the BasicTestWithMcj test, once I'm done with that, I think I'll compare the output xml files, I think the last time I wrote a one-off managed app for the purpose, I need to take a look if I still have it around but it's about half an hour's work to write.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the suggestion regarding ReferenceXUnitWrapperGenerator, that should make it nicely visible which tests are problematic for some future cleanup wave.
markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/Directory.Build.targets
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfig.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfigTester.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/ilasm_ildasm/regression/vswhidbey305155/305155.cs
Outdated

@stevehartersteveharter
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Sep 6, 2023
@ghost
ghost
commented
Sep 6, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @hoyosjs

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata, area-Infrastructure-coreclr
Milestone:-
@trylek
trylek
commented
Sep 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@markples - I have stood up the xml differ and I have managed to convince myself that the results are fine:
Left-only tests (5 total):
--------------------------
baseservices\exceptions\exceptioninterop\ExceptionInterop_ro\ExceptionInterop_ro
baseservices\exceptions\exceptioninterop\ExceptionInterop\ExceptionInterop
baseservices\exceptions\simple\ParallelCrashMainThread\ParallelCrashMainThread
baseservices\exceptions\simple\ParallelCrashWorkerThreads\ParallelCrashWorkerThreads
baseservices\TieredCompilation\BasicTestWithMcj\RunBasicTestWithMcj
Right-only tests (24 total):
----------------------------
_ExceptionInterop_ro::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ExceptionInterop::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThread()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThreadAndWorkerThreads()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashWorkerThreads()
baseservices\exceptions\stackoverflow\stackoverflow\stackoverflow
baseservices\exceptions\stackoverflow\stackoverflow3\stackoverflow3
baseservices\exceptions\StackTracePreserve\StackTracePreserveTests\StackTracePreserveTests
baseservices\exceptions\unhandled\unhandledTester\unhandledTester
baseservices\finalization\CriticalFinalizer\CriticalFinalizer
baseservices\ilasm_ildasm\regression\vswhidbey305155\305155\305155
baseservices\mono\runningmono\runningmono
baseservices\multidimmarray\enum\enum
baseservices\RuntimeConfiguration\TestConfigTester\TestConfigTester
baseservices\varargs\varargsupport_r\varargsupport_r
baseservices\varargs\varargsupport\varargsupport

There are several interesting observations based on this diff.
  • We have an ugly discontinuity regarding test naming based on whether a given test project contains one or multiple [Fact] clauses - when there are multiple Fact clauses, we drop the test path information and just name the test based on the method. I think we should probably concatenate these two or concatenate the test dll / script path with the method name or something, the completely different representation of tests with multiple test cases is confusing and makes it harder for developers to correlate their test failures to the source / binary code.
  • We have a subtle difference with regard to reporting tests blocked in issues.targets. The legacy infrastructure filtered them out at the msbuild level so that they never made it into the XUnit wrappers and simply aren't reported in any manner in the results. With the merged infrastructure we filter them out in a different manner and in the report we end up reporting them as "skipped". I think that's actually more appropriate, the only downside is that today there's no indication in the xml results file that the test in question was skipped due to an issues.targets clause; I have filed CoreCLR test infra: improve "Skip" messages for tests blocked in issues.targets #91562 to fix that. In the above list, that pertains to the "Right-only tests" 305155, runningmono, enum, varargsupport_r and varargsupport.
  • For stackoverflow, stackoverflow3 or unhandled, I believe these need fixing in the merged wrapper generator. These projects are marked as BuildOnly and shouldn't be actually run, the xml result file pretends it has run the corresponding cmd / sh script but that is nonsense, there's no cmd / sh script getting generated for these. I'll follow up with Jeremy on fixing this, I'm not sure to what extent it's blocking for this particular PR. (In practice the projects actually getting run are stackoverflowtester and unhandledTester introduced as part of this PR.)
Thanks
Tomas
@trylek
trylek
commented
Sep 6, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 6, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 7, 2023

Closed

@markples
markples
commented
Sep 8, 2023

Copy link


Copy Markdown

Contributor
Thanks Tomas!
  • re: discontinuity in naming: I believe that we ended up in this position because we had a legacy naming system (path/project) which couldn't differentiate between multiple tests in a project, and the "new" way is closer to (or the same as?) standard xunit naming. The discontinuity was kept to minimize the number of tests that were renamed as part of adding merged test groups. However, with the eventual hope of merging the builds too, perhaps moving more towards xunit-style (always use it) would have less eventual churn that going back to incorporating paths? It is important for now that the assembly name is there (though it appears to be?) because we don't dedup class/test names.
  • agreed on your analysis and new work item
  • I believe that these are being caused by the RequiresProcessIsolation attribute, for which the processing doesn't check for BuildOnly, etc. Is this needed in the final iteration? If not, then instead of excluding BuildOnly, perhaps Dir.build.targets could consider this an error.

    • It seems odd that the wrapper is reporting it as run when it doesn't exist. It looks like it is this code, which appears to be necessary but could be a point where tests are discarded incorrectly.
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
@markples - I have rebased this change against the latest main and I believe I have addressed all your pending PR feedback, can you please take another look when you have a chance?
Thanks Tomas
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 13, 2023

Closed

@trylek
Fix visibility in CriticalFinalizer test
2fe3767
@trylek
Simplify RuntimeConfiguration/TestConfig
e0bacc0
@trylek
Clean up TieredCompilation tests
65d2a7a
@trylek
Convert istypeequivalent to use ConditionalFact clauses
6897201
@trylek
Fix visibility in RuntimeHelpersTests
c72d876
@trylek
Add CoreCLRTestLibrary as a dependency of istypeequivalent
573730a
@trylek
Fix merged behavior of test448035
c43b1d7
@trylek
Fix entrypoint in 305155
c2d27d4
@trylek
Modify TestConfig to use a separate TestConfigTester app
e41e8f2
@trylek
Additional fixes to TestConfig / TestConfigTester
cbf2417
@trylek
Mechanically merge all remaining tests under baseservices
d30f9c6
@trylek
Fix BasicTestWithMcj, address initial Mark's PR feedback
02ffa17
@trylek
Remove superfluous OutputType=Library annotations per Marks' PR feedback
c9854b2
@trylek
Fix the baseservices/exceptions/unhandled test
02e17cf
@trylek
Fix stackoverflow3 and unhandled exception tests
be2a499
@trylek
Remove unnecessary check from Directory.Build.targets
2d85c95
@trylek
Fix stackoverflowtester per Mark's PR feedback
38e77a4
@trylek
trylek
commented
Oct 29, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 29, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
merged commit 1a19c09
into
dotnet:mainOct 30, 2023
@trylek
trylek
deleted the

MergeBaseServices

branch
October 30, 2023 01:12
gbalykov added a commit
to gbalykov/runtime
that referenced
this pull request
Oct 31, 2023
@gbalykov
Tests build fix with BuildAsStandalone
1cdfc1b
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before dotnet#91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@gbalykovgbalykov
mentioned this pull request
Oct 31, 2023
 Merged

@jtschusterjtschuster
mentioned this pull request
Nov 1, 2023
 Closed

trylek pushed a commit
that referenced
this pull request
Nov 1, 2023
@gbalykov
Tests build fix with BuildAsStandalone (#94208)
194fc5f
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before #91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@BruceForstallBruceForstall
mentioned this pull request
Nov 6, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 29, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ivdiazsaivdiazsaAwaiting requested review from ivdiazsa

1 more reviewer

@markplesmarkplesmarkples approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@trylektrylek

Labels

area-Infrastructure-coreclrOnly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@trylek@markples@steveharter

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Convert all tests under baseservices to the merged test infrastructure - #91560
Merged
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Oct 30, 2023
Merged
Convert all tests under baseservices to the merged test infrastructure#91560
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Conversation
@trylek
@trylektrylek
commented
Sep 4, 2023

Copy link


Copy Markdown

Member
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
@ghostghost
assigned trylekSep 4, 2023
@ghost
ghost
commented
Sep 4, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata
Milestone:-
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
How have you checked whether all tests have been preserved across the conversion? It seems like the changes to how the internal test harness work could impact the number of reported tests and make this difficult. (If so, do you have recommendations on how to review this with that in mind?)
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
For the tests that define a Main, I believe that BuildAsStandalone will be broken since it will try to generate an entry point. I think setting ReferenceXUnitWrapperGenerator to false will fix this.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Well, in the local testing I have so far just monitored the number of tests; in fact even that has changed slightly as I split one or two tests to multiple [Fact]s (I tried to clearly separate those individual changes from the bulk conversions as individual commits on the PR). I'm still working on fixing the BasicTestWithMcj test, once I'm done with that, I think I'll compare the output xml files, I think the last time I wrote a one-off managed app for the purpose, I need to take a look if I still have it around but it's about half an hour's work to write.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the suggestion regarding ReferenceXUnitWrapperGenerator, that should make it nicely visible which tests are problematic for some future cleanup wave.
markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/Directory.Build.targets
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfig.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfigTester.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/ilasm_ildasm/regression/vswhidbey305155/305155.cs
Outdated

@stevehartersteveharter
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Sep 6, 2023
@ghost
ghost
commented
Sep 6, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @hoyosjs

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata, area-Infrastructure-coreclr
Milestone:-
@trylek
trylek
commented
Sep 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@markples - I have stood up the xml differ and I have managed to convince myself that the results are fine:
Left-only tests (5 total):
--------------------------
baseservices\exceptions\exceptioninterop\ExceptionInterop_ro\ExceptionInterop_ro
baseservices\exceptions\exceptioninterop\ExceptionInterop\ExceptionInterop
baseservices\exceptions\simple\ParallelCrashMainThread\ParallelCrashMainThread
baseservices\exceptions\simple\ParallelCrashWorkerThreads\ParallelCrashWorkerThreads
baseservices\TieredCompilation\BasicTestWithMcj\RunBasicTestWithMcj
Right-only tests (24 total):
----------------------------
_ExceptionInterop_ro::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ExceptionInterop::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThread()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThreadAndWorkerThreads()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashWorkerThreads()
baseservices\exceptions\stackoverflow\stackoverflow\stackoverflow
baseservices\exceptions\stackoverflow\stackoverflow3\stackoverflow3
baseservices\exceptions\StackTracePreserve\StackTracePreserveTests\StackTracePreserveTests
baseservices\exceptions\unhandled\unhandledTester\unhandledTester
baseservices\finalization\CriticalFinalizer\CriticalFinalizer
baseservices\ilasm_ildasm\regression\vswhidbey305155\305155\305155
baseservices\mono\runningmono\runningmono
baseservices\multidimmarray\enum\enum
baseservices\RuntimeConfiguration\TestConfigTester\TestConfigTester
baseservices\varargs\varargsupport_r\varargsupport_r
baseservices\varargs\varargsupport\varargsupport

There are several interesting observations based on this diff.
  • We have an ugly discontinuity regarding test naming based on whether a given test project contains one or multiple [Fact] clauses - when there are multiple Fact clauses, we drop the test path information and just name the test based on the method. I think we should probably concatenate these two or concatenate the test dll / script path with the method name or something, the completely different representation of tests with multiple test cases is confusing and makes it harder for developers to correlate their test failures to the source / binary code.
  • We have a subtle difference with regard to reporting tests blocked in issues.targets. The legacy infrastructure filtered them out at the msbuild level so that they never made it into the XUnit wrappers and simply aren't reported in any manner in the results. With the merged infrastructure we filter them out in a different manner and in the report we end up reporting them as "skipped". I think that's actually more appropriate, the only downside is that today there's no indication in the xml results file that the test in question was skipped due to an issues.targets clause; I have filed CoreCLR test infra: improve "Skip" messages for tests blocked in issues.targets #91562 to fix that. In the above list, that pertains to the "Right-only tests" 305155, runningmono, enum, varargsupport_r and varargsupport.
  • For stackoverflow, stackoverflow3 or unhandled, I believe these need fixing in the merged wrapper generator. These projects are marked as BuildOnly and shouldn't be actually run, the xml result file pretends it has run the corresponding cmd / sh script but that is nonsense, there's no cmd / sh script getting generated for these. I'll follow up with Jeremy on fixing this, I'm not sure to what extent it's blocking for this particular PR. (In practice the projects actually getting run are stackoverflowtester and unhandledTester introduced as part of this PR.)
Thanks
Tomas
@trylek
trylek
commented
Sep 6, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 6, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 7, 2023

Closed

@markples
markples
commented
Sep 8, 2023

Copy link


Copy Markdown

Contributor
Thanks Tomas!
  • re: discontinuity in naming: I believe that we ended up in this position because we had a legacy naming system (path/project) which couldn't differentiate between multiple tests in a project, and the "new" way is closer to (or the same as?) standard xunit naming. The discontinuity was kept to minimize the number of tests that were renamed as part of adding merged test groups. However, with the eventual hope of merging the builds too, perhaps moving more towards xunit-style (always use it) would have less eventual churn that going back to incorporating paths? It is important for now that the assembly name is there (though it appears to be?) because we don't dedup class/test names.
  • agreed on your analysis and new work item
  • I believe that these are being caused by the RequiresProcessIsolation attribute, for which the processing doesn't check for BuildOnly, etc. Is this needed in the final iteration? If not, then instead of excluding BuildOnly, perhaps Dir.build.targets could consider this an error.

    • It seems odd that the wrapper is reporting it as run when it doesn't exist. It looks like it is this code, which appears to be necessary but could be a point where tests are discarded incorrectly.
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
@markples - I have rebased this change against the latest main and I believe I have addressed all your pending PR feedback, can you please take another look when you have a chance?
Thanks Tomas
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 13, 2023

Closed

@trylek
Fix visibility in CriticalFinalizer test
2fe3767
@trylek
Simplify RuntimeConfiguration/TestConfig
e0bacc0
@trylek
Clean up TieredCompilation tests
65d2a7a
@trylek
Convert istypeequivalent to use ConditionalFact clauses
6897201
@trylek
Fix visibility in RuntimeHelpersTests
c72d876
@trylek
Add CoreCLRTestLibrary as a dependency of istypeequivalent
573730a
@trylek
Fix merged behavior of test448035
c43b1d7
@trylek
Fix entrypoint in 305155
c2d27d4
@trylek
Modify TestConfig to use a separate TestConfigTester app
e41e8f2
@trylek
Additional fixes to TestConfig / TestConfigTester
cbf2417
@trylek
Mechanically merge all remaining tests under baseservices
d30f9c6
@trylek
Fix BasicTestWithMcj, address initial Mark's PR feedback
02ffa17
@trylek
Remove superfluous OutputType=Library annotations per Marks' PR feedback
c9854b2
@trylek
Fix the baseservices/exceptions/unhandled test
02e17cf
@trylek
Fix stackoverflow3 and unhandled exception tests
be2a499
@trylek
Remove unnecessary check from Directory.Build.targets
2d85c95
@trylek
Fix stackoverflowtester per Mark's PR feedback
38e77a4
@trylek
trylek
commented
Oct 29, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 29, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
merged commit 1a19c09
into
dotnet:mainOct 30, 2023
@trylek
trylek
deleted the

MergeBaseServices

branch
October 30, 2023 01:12
gbalykov added a commit
to gbalykov/runtime
that referenced
this pull request
Oct 31, 2023
@gbalykov
Tests build fix with BuildAsStandalone
1cdfc1b
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before dotnet#91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@gbalykovgbalykov
mentioned this pull request
Oct 31, 2023
 Merged

@jtschusterjtschuster
mentioned this pull request
Nov 1, 2023
 Closed

trylek pushed a commit
that referenced
this pull request
Nov 1, 2023
@gbalykov
Tests build fix with BuildAsStandalone (#94208)
194fc5f
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before #91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@BruceForstallBruceForstall
mentioned this pull request
Nov 6, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 29, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ivdiazsaivdiazsaAwaiting requested review from ivdiazsa

1 more reviewer

@markplesmarkplesmarkples approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@trylektrylek

Labels

area-Infrastructure-coreclrOnly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@trylek@markples@steveharter

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Convert all tests under baseservices to the merged test infrastructure - #91560
Merged
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Oct 30, 2023
Merged
Convert all tests under baseservices to the merged test infrastructure#91560
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Conversation
@trylek
@trylektrylek
commented
Sep 4, 2023

Copy link


Copy Markdown

Member
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
@ghostghost
assigned trylekSep 4, 2023
@ghost
ghost
commented
Sep 4, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata
Milestone:-
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
How have you checked whether all tests have been preserved across the conversion? It seems like the changes to how the internal test harness work could impact the number of reported tests and make this difficult. (If so, do you have recommendations on how to review this with that in mind?)
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
For the tests that define a Main, I believe that BuildAsStandalone will be broken since it will try to generate an entry point. I think setting ReferenceXUnitWrapperGenerator to false will fix this.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Well, in the local testing I have so far just monitored the number of tests; in fact even that has changed slightly as I split one or two tests to multiple [Fact]s (I tried to clearly separate those individual changes from the bulk conversions as individual commits on the PR). I'm still working on fixing the BasicTestWithMcj test, once I'm done with that, I think I'll compare the output xml files, I think the last time I wrote a one-off managed app for the purpose, I need to take a look if I still have it around but it's about half an hour's work to write.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the suggestion regarding ReferenceXUnitWrapperGenerator, that should make it nicely visible which tests are problematic for some future cleanup wave.
markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/Directory.Build.targets
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfig.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfigTester.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/ilasm_ildasm/regression/vswhidbey305155/305155.cs
Outdated

@stevehartersteveharter
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Sep 6, 2023
@ghost
ghost
commented
Sep 6, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @hoyosjs

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata, area-Infrastructure-coreclr
Milestone:-
@trylek
trylek
commented
Sep 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@markples - I have stood up the xml differ and I have managed to convince myself that the results are fine:
Left-only tests (5 total):
--------------------------
baseservices\exceptions\exceptioninterop\ExceptionInterop_ro\ExceptionInterop_ro
baseservices\exceptions\exceptioninterop\ExceptionInterop\ExceptionInterop
baseservices\exceptions\simple\ParallelCrashMainThread\ParallelCrashMainThread
baseservices\exceptions\simple\ParallelCrashWorkerThreads\ParallelCrashWorkerThreads
baseservices\TieredCompilation\BasicTestWithMcj\RunBasicTestWithMcj
Right-only tests (24 total):
----------------------------
_ExceptionInterop_ro::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ExceptionInterop::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThread()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThreadAndWorkerThreads()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashWorkerThreads()
baseservices\exceptions\stackoverflow\stackoverflow\stackoverflow
baseservices\exceptions\stackoverflow\stackoverflow3\stackoverflow3
baseservices\exceptions\StackTracePreserve\StackTracePreserveTests\StackTracePreserveTests
baseservices\exceptions\unhandled\unhandledTester\unhandledTester
baseservices\finalization\CriticalFinalizer\CriticalFinalizer
baseservices\ilasm_ildasm\regression\vswhidbey305155\305155\305155
baseservices\mono\runningmono\runningmono
baseservices\multidimmarray\enum\enum
baseservices\RuntimeConfiguration\TestConfigTester\TestConfigTester
baseservices\varargs\varargsupport_r\varargsupport_r
baseservices\varargs\varargsupport\varargsupport

There are several interesting observations based on this diff.
  • We have an ugly discontinuity regarding test naming based on whether a given test project contains one or multiple [Fact] clauses - when there are multiple Fact clauses, we drop the test path information and just name the test based on the method. I think we should probably concatenate these two or concatenate the test dll / script path with the method name or something, the completely different representation of tests with multiple test cases is confusing and makes it harder for developers to correlate their test failures to the source / binary code.
  • We have a subtle difference with regard to reporting tests blocked in issues.targets. The legacy infrastructure filtered them out at the msbuild level so that they never made it into the XUnit wrappers and simply aren't reported in any manner in the results. With the merged infrastructure we filter them out in a different manner and in the report we end up reporting them as "skipped". I think that's actually more appropriate, the only downside is that today there's no indication in the xml results file that the test in question was skipped due to an issues.targets clause; I have filed CoreCLR test infra: improve "Skip" messages for tests blocked in issues.targets #91562 to fix that. In the above list, that pertains to the "Right-only tests" 305155, runningmono, enum, varargsupport_r and varargsupport.
  • For stackoverflow, stackoverflow3 or unhandled, I believe these need fixing in the merged wrapper generator. These projects are marked as BuildOnly and shouldn't be actually run, the xml result file pretends it has run the corresponding cmd / sh script but that is nonsense, there's no cmd / sh script getting generated for these. I'll follow up with Jeremy on fixing this, I'm not sure to what extent it's blocking for this particular PR. (In practice the projects actually getting run are stackoverflowtester and unhandledTester introduced as part of this PR.)
Thanks
Tomas
@trylek
trylek
commented
Sep 6, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 6, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 7, 2023

Closed

@markples
markples
commented
Sep 8, 2023

Copy link


Copy Markdown

Contributor
Thanks Tomas!
  • re: discontinuity in naming: I believe that we ended up in this position because we had a legacy naming system (path/project) which couldn't differentiate between multiple tests in a project, and the "new" way is closer to (or the same as?) standard xunit naming. The discontinuity was kept to minimize the number of tests that were renamed as part of adding merged test groups. However, with the eventual hope of merging the builds too, perhaps moving more towards xunit-style (always use it) would have less eventual churn that going back to incorporating paths? It is important for now that the assembly name is there (though it appears to be?) because we don't dedup class/test names.
  • agreed on your analysis and new work item
  • I believe that these are being caused by the RequiresProcessIsolation attribute, for which the processing doesn't check for BuildOnly, etc. Is this needed in the final iteration? If not, then instead of excluding BuildOnly, perhaps Dir.build.targets could consider this an error.

    • It seems odd that the wrapper is reporting it as run when it doesn't exist. It looks like it is this code, which appears to be necessary but could be a point where tests are discarded incorrectly.
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
@markples - I have rebased this change against the latest main and I believe I have addressed all your pending PR feedback, can you please take another look when you have a chance?
Thanks Tomas
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 13, 2023

Closed

@trylek
Fix visibility in CriticalFinalizer test
2fe3767
@trylek
Simplify RuntimeConfiguration/TestConfig
e0bacc0
@trylek
Clean up TieredCompilation tests
65d2a7a
@trylek
Convert istypeequivalent to use ConditionalFact clauses
6897201
@trylek
Fix visibility in RuntimeHelpersTests
c72d876
@trylek
Add CoreCLRTestLibrary as a dependency of istypeequivalent
573730a
@trylek
Fix merged behavior of test448035
c43b1d7
@trylek
Fix entrypoint in 305155
c2d27d4
@trylek
Modify TestConfig to use a separate TestConfigTester app
e41e8f2
@trylek
Additional fixes to TestConfig / TestConfigTester
cbf2417
@trylek
Mechanically merge all remaining tests under baseservices
d30f9c6
@trylek
Fix BasicTestWithMcj, address initial Mark's PR feedback
02ffa17
@trylek
Remove superfluous OutputType=Library annotations per Marks' PR feedback
c9854b2
@trylek
Fix the baseservices/exceptions/unhandled test
02e17cf
@trylek
Fix stackoverflow3 and unhandled exception tests
be2a499
@trylek
Remove unnecessary check from Directory.Build.targets
2d85c95
@trylek
Fix stackoverflowtester per Mark's PR feedback
38e77a4
@trylek
trylek
commented
Oct 29, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 29, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
merged commit 1a19c09
into
dotnet:mainOct 30, 2023
@trylek
trylek
deleted the

MergeBaseServices

branch
October 30, 2023 01:12
gbalykov added a commit
to gbalykov/runtime
that referenced
this pull request
Oct 31, 2023
@gbalykov
Tests build fix with BuildAsStandalone
1cdfc1b
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before dotnet#91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@gbalykovgbalykov
mentioned this pull request
Oct 31, 2023
 Merged

@jtschusterjtschuster
mentioned this pull request
Nov 1, 2023
 Closed

trylek pushed a commit
that referenced
this pull request
Nov 1, 2023
@gbalykov
Tests build fix with BuildAsStandalone (#94208)
194fc5f
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before #91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@BruceForstallBruceForstall
mentioned this pull request
Nov 6, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 29, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ivdiazsaivdiazsaAwaiting requested review from ivdiazsa

1 more reviewer

@markplesmarkplesmarkples approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@trylektrylek

Labels

area-Infrastructure-coreclrOnly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@trylek@markples@steveharter

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Convert all tests under baseservices to the merged test infrastructure - #91560
Merged
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Oct 30, 2023
Merged
Convert all tests under baseservices to the merged test infrastructure#91560
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Conversation
@trylek
@trylektrylek
commented
Sep 4, 2023

Copy link


Copy Markdown

Member
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
@ghostghost
assigned trylekSep 4, 2023
@ghost
ghost
commented
Sep 4, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata
Milestone:-
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
How have you checked whether all tests have been preserved across the conversion? It seems like the changes to how the internal test harness work could impact the number of reported tests and make this difficult. (If so, do you have recommendations on how to review this with that in mind?)
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
For the tests that define a Main, I believe that BuildAsStandalone will be broken since it will try to generate an entry point. I think setting ReferenceXUnitWrapperGenerator to false will fix this.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Well, in the local testing I have so far just monitored the number of tests; in fact even that has changed slightly as I split one or two tests to multiple [Fact]s (I tried to clearly separate those individual changes from the bulk conversions as individual commits on the PR). I'm still working on fixing the BasicTestWithMcj test, once I'm done with that, I think I'll compare the output xml files, I think the last time I wrote a one-off managed app for the purpose, I need to take a look if I still have it around but it's about half an hour's work to write.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the suggestion regarding ReferenceXUnitWrapperGenerator, that should make it nicely visible which tests are problematic for some future cleanup wave.
markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/Directory.Build.targets
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfig.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfigTester.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/ilasm_ildasm/regression/vswhidbey305155/305155.cs
Outdated

@stevehartersteveharter
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Sep 6, 2023
@ghost
ghost
commented
Sep 6, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @hoyosjs

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata, area-Infrastructure-coreclr
Milestone:-
@trylek
trylek
commented
Sep 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@markples - I have stood up the xml differ and I have managed to convince myself that the results are fine:
Left-only tests (5 total):
--------------------------
baseservices\exceptions\exceptioninterop\ExceptionInterop_ro\ExceptionInterop_ro
baseservices\exceptions\exceptioninterop\ExceptionInterop\ExceptionInterop
baseservices\exceptions\simple\ParallelCrashMainThread\ParallelCrashMainThread
baseservices\exceptions\simple\ParallelCrashWorkerThreads\ParallelCrashWorkerThreads
baseservices\TieredCompilation\BasicTestWithMcj\RunBasicTestWithMcj
Right-only tests (24 total):
----------------------------
_ExceptionInterop_ro::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ExceptionInterop::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThread()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThreadAndWorkerThreads()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashWorkerThreads()
baseservices\exceptions\stackoverflow\stackoverflow\stackoverflow
baseservices\exceptions\stackoverflow\stackoverflow3\stackoverflow3
baseservices\exceptions\StackTracePreserve\StackTracePreserveTests\StackTracePreserveTests
baseservices\exceptions\unhandled\unhandledTester\unhandledTester
baseservices\finalization\CriticalFinalizer\CriticalFinalizer
baseservices\ilasm_ildasm\regression\vswhidbey305155\305155\305155
baseservices\mono\runningmono\runningmono
baseservices\multidimmarray\enum\enum
baseservices\RuntimeConfiguration\TestConfigTester\TestConfigTester
baseservices\varargs\varargsupport_r\varargsupport_r
baseservices\varargs\varargsupport\varargsupport

There are several interesting observations based on this diff.
  • We have an ugly discontinuity regarding test naming based on whether a given test project contains one or multiple [Fact] clauses - when there are multiple Fact clauses, we drop the test path information and just name the test based on the method. I think we should probably concatenate these two or concatenate the test dll / script path with the method name or something, the completely different representation of tests with multiple test cases is confusing and makes it harder for developers to correlate their test failures to the source / binary code.
  • We have a subtle difference with regard to reporting tests blocked in issues.targets. The legacy infrastructure filtered them out at the msbuild level so that they never made it into the XUnit wrappers and simply aren't reported in any manner in the results. With the merged infrastructure we filter them out in a different manner and in the report we end up reporting them as "skipped". I think that's actually more appropriate, the only downside is that today there's no indication in the xml results file that the test in question was skipped due to an issues.targets clause; I have filed CoreCLR test infra: improve "Skip" messages for tests blocked in issues.targets #91562 to fix that. In the above list, that pertains to the "Right-only tests" 305155, runningmono, enum, varargsupport_r and varargsupport.
  • For stackoverflow, stackoverflow3 or unhandled, I believe these need fixing in the merged wrapper generator. These projects are marked as BuildOnly and shouldn't be actually run, the xml result file pretends it has run the corresponding cmd / sh script but that is nonsense, there's no cmd / sh script getting generated for these. I'll follow up with Jeremy on fixing this, I'm not sure to what extent it's blocking for this particular PR. (In practice the projects actually getting run are stackoverflowtester and unhandledTester introduced as part of this PR.)
Thanks
Tomas
@trylek
trylek
commented
Sep 6, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 6, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 7, 2023

Closed

@markples
markples
commented
Sep 8, 2023

Copy link


Copy Markdown

Contributor
Thanks Tomas!
  • re: discontinuity in naming: I believe that we ended up in this position because we had a legacy naming system (path/project) which couldn't differentiate between multiple tests in a project, and the "new" way is closer to (or the same as?) standard xunit naming. The discontinuity was kept to minimize the number of tests that were renamed as part of adding merged test groups. However, with the eventual hope of merging the builds too, perhaps moving more towards xunit-style (always use it) would have less eventual churn that going back to incorporating paths? It is important for now that the assembly name is there (though it appears to be?) because we don't dedup class/test names.
  • agreed on your analysis and new work item
  • I believe that these are being caused by the RequiresProcessIsolation attribute, for which the processing doesn't check for BuildOnly, etc. Is this needed in the final iteration? If not, then instead of excluding BuildOnly, perhaps Dir.build.targets could consider this an error.

    • It seems odd that the wrapper is reporting it as run when it doesn't exist. It looks like it is this code, which appears to be necessary but could be a point where tests are discarded incorrectly.
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
@markples - I have rebased this change against the latest main and I believe I have addressed all your pending PR feedback, can you please take another look when you have a chance?
Thanks Tomas
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 13, 2023

Closed

@trylek
Fix visibility in CriticalFinalizer test
2fe3767
@trylek
Simplify RuntimeConfiguration/TestConfig
e0bacc0
@trylek
Clean up TieredCompilation tests
65d2a7a
@trylek
Convert istypeequivalent to use ConditionalFact clauses
6897201
@trylek
Fix visibility in RuntimeHelpersTests
c72d876
@trylek
Add CoreCLRTestLibrary as a dependency of istypeequivalent
573730a
@trylek
Fix merged behavior of test448035
c43b1d7
@trylek
Fix entrypoint in 305155
c2d27d4
@trylek
Modify TestConfig to use a separate TestConfigTester app
e41e8f2
@trylek
Additional fixes to TestConfig / TestConfigTester
cbf2417
@trylek
Mechanically merge all remaining tests under baseservices
d30f9c6
@trylek
Fix BasicTestWithMcj, address initial Mark's PR feedback
02ffa17
@trylek
Remove superfluous OutputType=Library annotations per Marks' PR feedback
c9854b2
@trylek
Fix the baseservices/exceptions/unhandled test
02e17cf
@trylek
Fix stackoverflow3 and unhandled exception tests
be2a499
@trylek
Remove unnecessary check from Directory.Build.targets
2d85c95
@trylek
Fix stackoverflowtester per Mark's PR feedback
38e77a4
@trylek
trylek
commented
Oct 29, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 29, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
merged commit 1a19c09
into
dotnet:mainOct 30, 2023
@trylek
trylek
deleted the

MergeBaseServices

branch
October 30, 2023 01:12
gbalykov added a commit
to gbalykov/runtime
that referenced
this pull request
Oct 31, 2023
@gbalykov
Tests build fix with BuildAsStandalone
1cdfc1b
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before dotnet#91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@gbalykovgbalykov
mentioned this pull request
Oct 31, 2023
 Merged

@jtschusterjtschuster
mentioned this pull request
Nov 1, 2023
 Closed

trylek pushed a commit
that referenced
this pull request
Nov 1, 2023
@gbalykov
Tests build fix with BuildAsStandalone (#94208)
194fc5f
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before #91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@BruceForstallBruceForstall
mentioned this pull request
Nov 6, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 29, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ivdiazsaivdiazsaAwaiting requested review from ivdiazsa

1 more reviewer

@markplesmarkplesmarkples approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@trylektrylek

Labels

area-Infrastructure-coreclrOnly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@trylek@markples@steveharter

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

Skip to content
Convert all tests under baseservices to the merged test infrastructure - #91560
Merged
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Oct 30, 2023
Merged
Convert all tests under baseservices to the merged test infrastructure#91560
trylek merged 31 commits into
dotnet:mainfrom 
trylek:MergeBaseServices
Conversation
@trylek
@trylektrylek
commented
Sep 4, 2023

Copy link


Copy Markdown

Member
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
@ghostghost
assigned trylekSep 4, 2023
@ghost
ghost
commented
Sep 4, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata
Milestone:-
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
How have you checked whether all tests have been preserved across the conversion? It seems like the changes to how the internal test harness work could impact the number of reported tests and make this difficult. (If so, do you have recommendations on how to review this with that in mind?)
@markples
markples
commented
Sep 5, 2023

Copy link


Copy Markdown

Contributor
For the tests that define a Main, I believe that BuildAsStandalone will be broken since it will try to generate an entry point. I think setting ReferenceXUnitWrapperGenerator to false will fix this.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Well, in the local testing I have so far just monitored the number of tests; in fact even that has changed slightly as I split one or two tests to multiple [Fact]s (I tried to clearly separate those individual changes from the bulk conversions as individual commits on the PR). I'm still working on fixing the BasicTestWithMcj test, once I'm done with that, I think I'll compare the output xml files, I think the last time I wrote a one-off managed app for the purpose, I need to take a look if I still have it around but it's about half an hour's work to write.
@trylek
trylek
commented
Sep 5, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the suggestion regarding ReferenceXUnitWrapperGenerator, that should make it nicely visible which tests are problematic for some future cleanup wave.
markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/Directory.Build.targets
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfig.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/RuntimeConfiguration/TestConfigTester.csproj
Outdated

markples
markples
reviewed
Sep 5, 2023
View reviewed changes
Comment threadsrc/tests/baseservices/ilasm_ildasm/regression/vswhidbey305155/305155.cs
Outdated

@stevehartersteveharter
added
the 
area-Infrastructure-coreclr
Only use for closed issues
label
Sep 6, 2023
@ghost
ghost
commented
Sep 6, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @hoyosjs

See info in area-owners.md if you want to be subscribed.
Issue Details
This change converts all remaining tests under baseservices to use the merged test model.

Apart from a few tiny tweaks I found a somewhat funny case in the test
baseservices/RuntimeConfiguration/TestConfig.csproj
This test had two problematic characteristics w.r.t. test merging - it was launching itself as

a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally

didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"

app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were

actually superfluous because the test uses two additional attributes (ConfigProperty and

EnvVar) to mark the "interesting" methods.
Thanks
Tomas
/cc @dotnet/runtime-infrastructure
Author:trylek
Assignees:trylek
Labels:
area-System.Reflection.Metadata, area-Infrastructure-coreclr
Milestone:-
@trylek
trylek
commented
Sep 6, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
@markples - I have stood up the xml differ and I have managed to convince myself that the results are fine:
Left-only tests (5 total):
--------------------------
baseservices\exceptions\exceptioninterop\ExceptionInterop_ro\ExceptionInterop_ro
baseservices\exceptions\exceptioninterop\ExceptionInterop\ExceptionInterop
baseservices\exceptions\simple\ParallelCrashMainThread\ParallelCrashMainThread
baseservices\exceptions\simple\ParallelCrashWorkerThreads\ParallelCrashWorkerThreads
baseservices\TieredCompilation\BasicTestWithMcj\RunBasicTestWithMcj
Right-only tests (24 total):
----------------------------
_ExceptionInterop_ro::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ExceptionInterop::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThread()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThreadAndWorkerThreads()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashWorkerThreads()
baseservices\exceptions\stackoverflow\stackoverflow\stackoverflow
baseservices\exceptions\stackoverflow\stackoverflow3\stackoverflow3
baseservices\exceptions\StackTracePreserve\StackTracePreserveTests\StackTracePreserveTests
baseservices\exceptions\unhandled\unhandledTester\unhandledTester
baseservices\finalization\CriticalFinalizer\CriticalFinalizer
baseservices\ilasm_ildasm\regression\vswhidbey305155\305155\305155
baseservices\mono\runningmono\runningmono
baseservices\multidimmarray\enum\enum
baseservices\RuntimeConfiguration\TestConfigTester\TestConfigTester
baseservices\varargs\varargsupport_r\varargsupport_r
baseservices\varargs\varargsupport\varargsupport

There are several interesting observations based on this diff.
  • We have an ugly discontinuity regarding test naming based on whether a given test project contains one or multiple [Fact] clauses - when there are multiple Fact clauses, we drop the test path information and just name the test based on the method. I think we should probably concatenate these two or concatenate the test dll / script path with the method name or something, the completely different representation of tests with multiple test cases is confusing and makes it harder for developers to correlate their test failures to the source / binary code.
  • We have a subtle difference with regard to reporting tests blocked in issues.targets. The legacy infrastructure filtered them out at the msbuild level so that they never made it into the XUnit wrappers and simply aren't reported in any manner in the results. With the merged infrastructure we filter them out in a different manner and in the report we end up reporting them as "skipped". I think that's actually more appropriate, the only downside is that today there's no indication in the xml results file that the test in question was skipped due to an issues.targets clause; I have filed CoreCLR test infra: improve "Skip" messages for tests blocked in issues.targets #91562 to fix that. In the above list, that pertains to the "Right-only tests" 305155, runningmono, enum, varargsupport_r and varargsupport.
  • For stackoverflow, stackoverflow3 or unhandled, I believe these need fixing in the merged wrapper generator. These projects are marked as BuildOnly and shouldn't be actually run, the xml result file pretends it has run the corresponding cmd / sh script but that is nonsense, there's no cmd / sh script getting generated for these. I'll follow up with Jeremy on fixing this, I'm not sure to what extent it's blocking for this particular PR. (In practice the projects actually getting run are stackoverflowtester and unhandledTester introduced as part of this PR.)
Thanks
Tomas
@trylek
trylek
commented
Sep 6, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 6, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Sep 7, 2023

Closed

@markples
markples
commented
Sep 8, 2023

Copy link


Copy Markdown

Contributor
Thanks Tomas!
  • re: discontinuity in naming: I believe that we ended up in this position because we had a legacy naming system (path/project) which couldn't differentiate between multiple tests in a project, and the "new" way is closer to (or the same as?) standard xunit naming. The discontinuity was kept to minimize the number of tests that were renamed as part of adding merged test groups. However, with the eventual hope of merging the builds too, perhaps moving more towards xunit-style (always use it) would have less eventual churn that going back to incorporating paths? It is important for now that the assembly name is there (though it appears to be?) because we don't dedup class/test names.
  • agreed on your analysis and new work item
  • I believe that these are being caused by the RequiresProcessIsolation attribute, for which the processing doesn't check for BuildOnly, etc. Is this needed in the final iteration? If not, then instead of excluding BuildOnly, perhaps Dir.build.targets could consider this an error.

    • It seems odd that the wrapper is reporting it as run when it doesn't exist. It looks like it is this code, which appears to be necessary but could be a point where tests are discarded incorrectly.
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Oct 12, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
commented
Oct 12, 2023

Copy link


Copy Markdown

MemberAuthor
@markples - I have rebased this change against the latest main and I believe I have addressed all your pending PR feedback, can you please take another look when you have a chance?
Thanks Tomas
@build-analysisbuild-analysisBot
mentioned this pull request
Oct 13, 2023

Closed

@trylek
Fix visibility in CriticalFinalizer test
2fe3767
@trylek
Simplify RuntimeConfiguration/TestConfig
e0bacc0
@trylek
Clean up TieredCompilation tests
65d2a7a
@trylek
Convert istypeequivalent to use ConditionalFact clauses
6897201
@trylek
Fix visibility in RuntimeHelpersTests
c72d876
@trylek
Add CoreCLRTestLibrary as a dependency of istypeequivalent
573730a
@trylek
Fix merged behavior of test448035
c43b1d7
@trylek
Fix entrypoint in 305155
c2d27d4
@trylek
Modify TestConfig to use a separate TestConfigTester app
e41e8f2
@trylek
Additional fixes to TestConfig / TestConfigTester
cbf2417
@trylek
Mechanically merge all remaining tests under baseservices
d30f9c6
@trylek
Fix BasicTestWithMcj, address initial Mark's PR feedback
02ffa17
@trylek
Remove superfluous OutputType=Library annotations per Marks' PR feedback
c9854b2
@trylek
Fix the baseservices/exceptions/unhandled test
02e17cf
@trylek
Fix stackoverflow3 and unhandled exception tests
be2a499
@trylek
Remove unnecessary check from Directory.Build.targets
2d85c95
@trylek
Fix stackoverflowtester per Mark's PR feedback
38e77a4
@trylek
trylek
commented
Oct 29, 2023

Copy link


Copy Markdown

MemberAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Oct 29, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@trylek
trylek
merged commit 1a19c09
into
dotnet:mainOct 30, 2023
@trylek
trylek
deleted the

MergeBaseServices

branch
October 30, 2023 01:12
gbalykov added a commit
to gbalykov/runtime
that referenced
this pull request
Oct 31, 2023
@gbalykov
Tests build fix with BuildAsStandalone
1cdfc1b
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before dotnet#91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@gbalykovgbalykov
mentioned this pull request
Oct 31, 2023
 Merged

@jtschusterjtschuster
mentioned this pull request
Nov 1, 2023
 Closed

trylek pushed a commit
that referenced
this pull request
Nov 1, 2023
@gbalykov
Tests build fix with BuildAsStandalone (#94208)
194fc5f
Without this patch build of these tests fails with "No entry point declared for executable" with BuildAsStandalone.
Before #91560 they were built as OutputType=Library and these are actually not launched during testing, but are used as libs.
@BruceForstallBruceForstall
mentioned this pull request
Nov 6, 2023

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 29, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ivdiazsaivdiazsaAwaiting requested review from ivdiazsa

1 more reviewer

@markplesmarkplesmarkples approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@trylektrylek

Labels

area-Infrastructure-coreclrOnly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@trylek@markples@steveharter