Fix System.Runtime.Numerics Tests to run against desktop #15050
Conversation
|
@mellinoe could you please have a look? cc @danmosemsft |
There was a problem hiding this comment.
Most of the changes look good; we should disable the tests that are netcoreapp-specific (the ones that are using internal state). Some of the tests look pretty normal, though. I think we need to take a closer look to figure out why they are behaving differently. Are we able to mark those tests with ActiveIssue for .NET Framework instead?
| { | ||
| IComparable comparable = new BigInteger(); | ||
| Assert.Equal(1, comparable.CompareTo(null)); | ||
| Assert.Throws<ArgumentException>("obj", () => comparable.CompareTo(0)); // Obj is not a BigInteger | ||
| Assert.Throws<ArgumentException>(paramName, () => comparable.CompareTo(0)); // Obj is not a BigInteger |
There was a problem hiding this comment.
Do we want/need these exception messages to be the same?
There was a problem hiding this comment.
It doesn't have to be. it depends on the behavior we desire. if netcore behavior is correct or better one then we can have differences. if it doesn't matter to have this netcore behavior then we can change it to match the desktop. from what I am seeing it make sense to keep netcore behavior
There was a problem hiding this comment.
Another option is just to remove the name verification in this case. What you have is fine, though.
| @@ -13,6 +13,7 @@ public class IsEvenTest | |||
| private static int s_seed = 0; | |||
|
|
|||
| [Fact] | |||
| [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] | |||
There was a problem hiding this comment.
I think we actually need to figure out why the behavior is different here. This one doesn't seem to be doing anything with internal state.
| @@ -292,6 +292,7 @@ public static IEnumerable<object[]> ACos_Advanced_TestData() | |||
| } | |||
|
|
|||
| [Theory, MemberData("ACos_Advanced_TestData")] | |||
| [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] | |||
There was a problem hiding this comment.
I think all of the tests in this class should be checked as well; they don't seem to be testing anything platform-specific.
I am just trying to enable the test to run on the desktop. I have opened the issue #15051 to track the needed investigations so we don't have to block this PR and get the test runs on desktop. please let me know if you are ok with this. |
|
I'm okay with that. It sounds like we can't use ActiveIssue for a particular TFM, so this is fine for now. |
|
test Innerloop Ubuntu14.04 Debug Build and Test please |
|
test Innerloop OSX Debug Build and Test please |
Fix System.Runtime.Numerics Tests to run against desktop Commit migrated from dotnet/corefx@b434d42
Fixes #14993
Fixes #14992
Fixes #14991