BigInteger based random testing of System.Decimal #24053
Conversation
| [Fact] | ||
| public static void RandomBigInteger_Compare() | ||
| { | ||
| var values = GetRandomData(out var bigValues); |
There was a problem hiding this comment.
Style: don't use var or out var here.
There was a problem hiding this comment.
Also may help to name these decimalValues and bigIntValues.
| var values = GetRandomData(out var bigValues); | ||
| for (int i = 0, b = 0; i < values.Length; i++) | ||
| { | ||
| var d1 = values[i]; |
There was a problem hiding this comment.
I'd rather not see var throughout here. Everything has a similar name but lots of things have different types, which is important to highight.
| bigDecimals = Array.ConvertAll(data, d => new BigDecimal(d)); | ||
| return data; | ||
|
|
||
| int IncBitLimits(int i) |
There was a problem hiding this comment.
Can there be a description for these values? They seem arbitrary.
| } | ||
| } | ||
|
|
||
| struct BigDecimal |
There was a problem hiding this comment.
There's a very large amount of non-trivial code in this structure. Could you explain a bit more about where it came from / why it is correct? Is this logic copy-pasted from BigInteger? Without understanding how this stuff is valid / trustworthy, it's hard to put a lot of value in the tests that rely on it.
| public static void RandomBigInteger_Compare() | ||
| { | ||
| var values = GetRandomData(out var bigValues); | ||
| for (int i = 0, b = 0; i < values.Length; i++) |
There was a problem hiding this comment.
I don't think this b variable is actually used (it's only incremented).
| var expected = b1.Add(bigValues[j], out var expectedOF); | ||
| if (expectedOF) | ||
| { | ||
| if (--overflowBudget < 0) |
There was a problem hiding this comment.
This is for limiting the number of overflow cases verified - without the limit there would be about 100k additional exceptions thrown (4-5s additional test execution time).
| { | ||
| var overflowBudget = 1000; | ||
| var values = GetRandomData(out var bigValues); | ||
| for (int i = 0, b = 0; i < values.Length; i++) |
| { | ||
| var overflowBudget = 1000; | ||
| var values = GetRandomData(out var bigValues); | ||
| for (int i = 0, b = 0; i < values.Length; i++) |
|
The I added more comments for the tests and |
Well, it ensures that they have the same behavior, but not that they are correct. They could both have the same bug, and the tests would pass. I'm not saying that's what is happening, but by using the Could I see the source from which it is based on? |
|
I agree, the tests ensure that the results are the same as today in CoreRT/CoreCLR/Desktop, but not that they are correct. The source is in CoreRT or CoreCLR/oleaut32 (it's the same thing basically, in C# and C++), but I've tried to use as little as possible from these. I think I was a bit too hasty to conclude that |
|
In fact, I did find a bug in And another probable bug in |
|
If the tests were split across two classes, I believe xunit may parallelize, potentially cutting the time taken. @mellinoe please see responses above. |
|
I simplified |
|
Looks okay to me now. @joperezr Did you want to take a look? |
|
Merging now since the failing test in opensuse is not related (System.Drawing test) |
* BigInteger based random testing of Decimal * Comments for random testing of Decimal * Parallelize random testing of Decimal * Simplified BigDecimal.Div for Decimal tests Commit migrated from dotnet/corefx@d235612
New comprehensive tests are needed for a significantly improved managed decimal implementation: dotnet/corert#4452 (comment), at first in CoreRT, but hopefully later shared with CoreCLR.
These tests compare
System.Decimalcmp/add/mul/div results against aBigIntegerbased reference implementation using ~1000 different random decimals covering every scale and sign with ~20 different bit-patterns each (about 950k combinations). It adds about 1-2 seconds toSystem.Runtimetest execution time (from 10s to 12s) - is this acceptable?