BigInteger based random testing of Decimal (part 2) #26338
Conversation
| int actual = decimal.ToInt32(d1); | ||
| throw new Xunit.Sdk.AssertActualExpectedException(typeof(OverflowException), actual, d1 + " ToInt32"); | ||
| } | ||
| catch (OverflowException) { } |
There was a problem hiding this comment.
Can this use Assert.Throws instead?
There was a problem hiding this comment.
There was a problem hiding this comment.
Assert.Throws loses the context (the input values).
| [Fact] | ||
| public static void BigInteger_ToInt32() | ||
| { | ||
| int overflowBudget = 100; |
There was a problem hiding this comment.
The existing tests all use an overflowBudget of 1000, but these 2 new ones have 100. Why the difference?
There was a problem hiding this comment.
I'm not sure I understand what overflowBudget is trying to achieve. Can you explain?
In reply to: 162363865 [](ancestors = 162363865)
There was a problem hiding this comment.
The existing tests do N*N invocations (where N is about 1000), but these new tests run only N invocations so I reduced the max time spent on exception cases also somewhat. Should I remove the overflowBudget entirely from these new tests?
There was a problem hiding this comment.
So overflowBudget is just about reducing test execution time? Do these tests take more than a few seconds to run?
My opinion would be if this will only add a couple hundred milliseconds to the testing time, I don't think the added complexity is worth it.
| @@ -1455,6 +1455,159 @@ public static void Test() | |||
| } | |||
| } | |||
|
|
|||
| [Fact] | |||
| public static void BigInteger_Floor() | |||
There was a problem hiding this comment.
If these were [Theory]s that took pairs of values then the message produced would be clearer as to the failing input if a test failed, making fixing it easier.
There was a problem hiding this comment.
This code generates the same AssertActualExpectedException that would be generated when using a [Theory], but it runs a lot faster. GetRandomData produces about N=1000 decimals that are then tested in N * N, N * 29 or N invocations depending on the test.
| } | ||
|
|
||
| [Fact] | ||
| public static void BigInteger_Ceiling() |
There was a problem hiding this comment.
Any reason these tests are called BigInteger? I guess I would have thought they would be called BigDecimal.
There was a problem hiding this comment.
I used the same name for previous BigInteger based tests. I guess they could be called BigDecimal based also, but the BigDecimal type is a private type inside this test class only.
|
Can this be merged or should I rename the tests (including the previously added Cmp/Add/Mul/Div tests)? |
…et#26338) User may have extra expectations for the privatly owned array pools. For example there could be an expectation that array never contains negative numbers (since user never puts them there). Uninitialized allocations can break such expectations. Signed-off-by: dotnet-bot <[email protected]>
User may have extra expectations for the privatly owned array pools. For example there could be an expectation that array never contains negative numbers (since user never puts them there). Uninitialized allocations can break such expectations. Signed-off-by: dotnet-bot <[email protected]>
…et#26338) (dotnet#26366) User may have extra expectations for the privatly owned array pools. For example there could be an expectation that array never contains negative numbers (since user never puts them there). Uninitialized allocations can break such expectations. Signed-off-by: dotnet-bot <[email protected]>
…) (#26366) User may have extra expectations for the privatly owned array pools. For example there could be an expectation that array never contains negative numbers (since user never puts them there). Uninitialized allocations can break such expectations. Signed-off-by: dotnet-bot <[email protected]>
) Commit migrated from dotnet/corefx@64634c7
This is a continuation of #24053 and contains new tests for dotnet/corert#4997