Adding generic-math tests for decimal and double/single/half to parity the integer tests #68043
Conversation
…y the integer tests
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
ghost
assigned
|
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsThese apparently didn't get checked in alongside the integer tests like they should've. I've extracted them from #67939 since that is otherwise blocked (hopefully due to something specific to the changes in that PR) There are no changes from #67939 which has already been reviewed/approved, so you shouldn't feel the need to re-review the files.
|
dakersnar
left a comment
dakersnar
left a comment
There was a problem hiding this comment.
Already approved the previous PR, so I'm assuming this is all good.
jeffhandley
left a comment
jeffhandley
left a comment
There was a problem hiding this comment.
I strongly prefer the use of [Theory] with [InlineData] or [MemberData] over [Fact] with many asserts to capture the scenarios. With a [Theory], regressions surface N times more failures and the impact is much more obvious than a [Fact] that fails on the first regressed scenario. Are all scenarios broken, a subset, or just one?
With the sheer number of asserts that exist herein though, would we incur overhead from [Theory] with measurable difference in build time? Or is there another reason why you're favoring multi-assert [Fact] tests?
Approved as this recommendation isn't a blocker, but I'd like it to be considered
| AssertBitwiseEqual(1.0, NumberHelper<double>.CreateSaturating<nuint>((nuint)0x00000001)); | ||
| AssertBitwiseEqual(2147483647.0, NumberHelper<double>.CreateSaturating<nuint>((nuint)0x7FFFFFFF)); | ||
|
|
||
| // https://github.com/dotnet/roslyn/issues/60714 |
There was a problem hiding this comment.
I suggest breaking these commented-out asserts (in both branches) into a new test, and annotating with [ActiveIssue] instead of commenting them out. The new tests would then have names that denote the trait that makes them fail currently.
And this comment is true for each place you have tests commented out.
I'd generally agree. However these are all very basic tests covering the primitives where we are really doing the bare minimum validation that the one line implementation isn't falling over at runtime. Given the 14 different types and that there needs to be tests covering conversion between the 14 different types to each other, this effectively becomes a combinatorics problem where its much simpler to just copy past the literal code with a simple find/replace for each type. It then also handles the issues around types like -- If we were using nunit, where a lot of the basic support for these kinds of problems are built-in, I likely would've gone a different route. |
4 participants
These apparently didn't get checked in alongside the integer tests like they should've. I've extracted them from #67939 since that is otherwise blocked (hopefully due to something specific to the changes in that PR)
There are no changes from #67939 which has already been reviewed/approved, so you shouldn't feel the need to re-review the files.