Disable tests on CoreCLR using new attribute #118
Conversation
safern
added
NO-MERGE
| @@ -16,6 +16,7 @@ | |||
| <add key="dotnet5-transport" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet5-transport/nuget/v3/index.json" /> | |||
| <add key="dotnet-core" value="https://dotnetfeed.blob.core.windows.net/dotnet-core/index.json" /> | |||
| <add key="dotnet-coreclr" value="https://dotnetfeed.blob.core.windows.net/dotnet-coreclr/index.json" /> | |||
| <add key="safern-test" value="https://www.myget.org/F/safern-test/api/v3/index.json" /> | |||
There was a problem hiding this comment.
I presume this will get removed before you merge?
There was a problem hiding this comment.
Yes. I’m just waiting for arcade to propagate.
src/libraries/System.Net.Http/tests/FunctionalTests/AssemblyInfo.cs
Outdated
Show resolved
Hide resolved
...em/ComponentModel/Composition/Registration/RegistrationBuilderAttributedOverrideUnitTests.cs
Show resolved
Hide resolved
src/libraries/System.Net.HttpListener/tests/HttpListenerAuthenticationTests.cs
Show resolved
Hide resolved
src/libraries/System.Net.HttpListener/tests/HttpListenerResponseTests.Cookies.cs
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/tests/FunctionalTests/AssemblyInfo.cs
Outdated
Show resolved
Hide resolved
|
Thanks for working on this. I'm happy to see the mechanisms consolidated. |
src/libraries/System.Utf8String.Experimental/tests/System.Utf8String.Experimental.Tests.csproj
Outdated
Show resolved
Hide resolved
|
@safern, this is still marked NO MERGE. Is it ready to go, or still waiting on something? Thanks! |
|
|
||
| using Xunit; | ||
|
|
||
| [assembly: SkipOnCoreClr("System.Net.Tests are flaky and/or long running: https://github.com/dotnet/runtime/issues/131")] |
There was a problem hiding this comment.
If the tests are flaky why do you skip them on CoreCLR only?
There was a problem hiding this comment.
If I remember correctly, they're flaky only on Checked CoreCLR because they have checks based on timeouts. The performance hits of running on a Checked CoreCLR leads to some of the timeout tests failing.
There was a problem hiding this comment.
It's not clear from the naming, but this is only skipping these tests in coreclr's stress test modes (including checked builds). We should a) fix the attribute to require that to be expressed such that no parameters means to always skip on coreclr, and b) we should fix the tests to be runnable in these cases, as well.
There was a problem hiding this comment.
SkipOnCoreClrStressTest sounds like a better naming?
There was a problem hiding this comment.
SkipOnCoreClrStressTest sounds like a better naming?
I think there are two options:
- Make the attribute specific to stress testing, in which case a name like you suggest would be fine.
- Actually make the attribute do what it's name says, which is to skip on coreclr. Then if you only wanted to skip on coreclr in a specific situation, you would pass the argument to say so, e.g.
[SkipOnCoreClr]would always skip the test on coreclr, whereas[SkipOnCoreClr(CoreClrTestMode.Stress)]would skip it in stress scenarios,[SkipOnCoreClr(CoreClrTestMode.Checked | CoreClrTestMode.GCStress)]would skip only if using a checked build or running GC stress, etc.
There was a problem hiding this comment.
Ok, makes sense. I'll work later on that.
safern
removed
the
NO-MERGE
|
Replaced by: #261 since I had to re-create my fork. |
ghost
locked as resolved and limited conversation to collaborators
This disables corefx tests on coreclr runs using the new
SkipOnCoreClrAttributeadded on: dotnet/arcade#4231I followed the rsp file list to add the attributes and also the labels on the issues referenced to identify the stress mode or the platform.
This still needs an arcade update to consume that PR.
cc: @BruceForstall @stephentoub @jkotas @jkoritzinsky @jaredpar