Add tests for dotnet/coreclr#20815 - new Math and MathF rounding modes #33297
Conversation
| [InlineData(-11.4, -11, MidpointRounding.ToZero)] | ||
| [InlineData(-11.5, -11, MidpointRounding.ToZero)] | ||
| [InlineData(-11.6, -11, MidpointRounding.ToZero)] | ||
| public static void Round_Double_Modes(double x, double expected, MidpointRounding mode) |
There was a problem hiding this comment.
Would some tests with more decimal places make sense? Could add them to some of the existing cases
There was a problem hiding this comment.
added tests for digits with new modes
| @@ -1441,6 +1441,145 @@ public static void Round_Double_Digits() | |||
| Assert.Equal(double.NegativeInfinity, Math.Round(double.NegativeInfinity, 3, MidpointRounding.AwayFromZero)); | |||
| } | |||
|
|
|||
| [Theory] | |||
There was a problem hiding this comment.
You should be able to avoid repeating the inline data using MemberData...see elsewhere in repo for examples
There was a problem hiding this comment.
using MemberData now
|
Hey @danmosemsft, sorry for the delay in updating my PR. Is there any outstanding feedback to get this merged ? |
|
Thanks @hamish-rose for updating. @tannergooding should sign off before merging, as the area expert here. |
|
@tannergooding, what's the status of this? |
|
@dotnet-bot test this please |
|
This is pending the corresponding CoreCLR side changes, which should be getting merged within the next day or two. |
|
@dotnet-bot test this please |
|
I see it still hasn't been merged. Soon? |
|
The coreclr changes have been consumed into corefx. To do so I had to update the ref in corefx to expose the new enum values and fix a negative test based on them. Can this PR be rebased to deal with that? Thanks. |
OK, will rebase PR onto master. |
|
Some checks are failing - i'm away until l next week, I can take a look then |
|
test NETFX x86 Release Build |
|
I built CoreCLR master, then built CoreFX hamish-rose:MidpointRoundingmodes branch against that private build in release configuration with no errors. I notice in Windows NETFX_x86_Release check that's failing is building against CoreCLR from https://dotnetcli.azureedge.net/dotnet/Sdk/2.2.103/dotnet-sdk-2.2.103-win-x64.zip - i'm not sure what source this SDK version is built from. |
Those netfx legs use the .NET Framework, which does not have the APIs in question. You'll need to add the tests that refer to the new APIs to files that aren't compiled in for the netfx test builds, e.g. into the MathTests.netcoreapp.cs file rather than Math.cs. |
|
Hmmm, this is now almost 4 months old PR. Do we have plan / ETA for closure? |
new tests added for Math.cs double Round methods
convert to theory
tests now reflect correct rounding behavior
- refactor tests to use member data - move MathF tests to MathF.netcoreapp.cs - add tests for new modes with digits and special cases
netcoreapp specific tests moved
|
I think this is ready to merge now |
|
@hamish-rose, thanks for getting this implemented; and sorry for the delay in getting this merged. |
…rounding modes (dotnet/corefx#33297) * add tests for new midpointrounding modes new tests added for Math.cs double Round methods * add tests for float, decimal convert to theory * Update tests tests now reflect correct rounding behavior * add tests for existing modes * code review feedback - refactor tests to use member data - move MathF tests to MathF.netcoreapp.cs - add tests for new modes with digits and special cases * move math tests to mathtests.netcoreapp.cs netcoreapp specific tests moved Commit migrated from dotnet/corefx@cc42bb9
adds tests for PR dotnet/coreclr#20815
resolves dotnet/corefx#31902