Reapply VB Operator fixes#32459
Conversation
|
Yes @tannergooding changed the behavior related to negative zero to be IEE754 compliant. How are you dealing with this difference between Core and Desktop? Is that this 187ad6c ? |
| { | ||
| if (expected is double expectedDouble && actual is double actualDouble) | ||
| { | ||
| Assert.Equal(expected.ToString(), actual.ToString()); |
There was a problem hiding this comment.
Why are you comparing equality using strings? That will not do what you expect for inputs that only differ in the 16th or 17th digit?
This should probably be doing:
if (double.IsNaN(expectedDouble))
{
Assert.True(double.IsNaN(actualDouble));
}
else
{
Assert.Equal(BitConverter.DoubleToInt64Bits(expectedDouble), BitConverter.DoubleToInt64Bits(actualDouble));
}There was a problem hiding this comment.
This required a bunch of updating the expected test results but I actually just ended up with a simple assert.equal for float/double because if your condition works, then the doubles are necessarily equal, and the results are easier to view due to xunit printing the expected/actual values on failure
| End If | ||
| Else | ||
| tc1 = conv1.GetTypeCode() | ||
| End If |
There was a problem hiding this comment.
When does this give a different result than the current implementation?
Never mind, it looks like this change reverts the implementation of this method so it matches the reference source.
| } | ||
| else | ||
| { | ||
| Assert.Equal(expected.ToString(), actual.ToString()); |
There was a problem hiding this comment.
As from before, this shouldn't be doing a string comparison, as that will end up truncating bits and doing a relative comparison.
There was a problem hiding this comment.
Comment on an outdated commit, just look at the second commit now
|
Which part of this is acommodating the the printing |
Commit 1 reapplies the reverted PR |
| else if (expected is float expectedFloat && actual is float actualFloat) | ||
| { | ||
| Assert.Equal(expected.ToString(), actual.ToString()); | ||
| Assert.Equal(expectedFloat, actualFloat, 10); |
There was a problem hiding this comment.
I don't think xUnit has an Assert.Equal overload that takes a float, so this will be converting to double and comparing those.
10 digits is also more than needed. System.Single has approx. 6-9 digits of precision, and you need at most 9 to uniquely represent every value.
| if (expected is double expectedDouble && actual is double actualDouble) | ||
| { | ||
| Assert.Equal(expected.ToString(), actual.ToString()); | ||
| Assert.Equal(expectedDouble, actualDouble, 10); |
There was a problem hiding this comment.
This doesn't seem right, on all platforms the results for these simple operations should be deterministic.
We were previously correct for 15 digits, so I would expect the same here (at a minimum). Ideally we would be comparing to the full 17 digits, however (or just doing a bit comparison).
There was a problem hiding this comment.
@hughbe, can we simply use Assert.Equal(expected, actual) in all cases?
There was a problem hiding this comment.
@hughbe, can we use Assert.Equal()? Thanks.
* Add VB operator tests * Delete dead code from VB operators * Add comparison operator tests * Add more tests * Fix VB handling of DBNull.Value * Do all number parsing in the invariant culture * Workaround https://github.com/dotnet/coreclr/issues/8648
|
@dotnet-bot Test Linux x64 Release Build (System.Net.Http.Functional.Tests failed) |
|
@dotnet-bot Test Linux x64 Release Build |
|
Good to merge? :) |
* Add more VB operator tests (dotnet#31320) * Add VB operator tests * Delete dead code from VB operators * Add comparison operator tests * Add more tests * Fix VB handling of DBNull.Value * Do all number parsing in the invariant culture * Workaround https://github.com/dotnet/coreclr/issues/8648 * Accomodate for -0.0 change and fix test comparisons
* Add more VB operator tests (dotnet/corefx#31320) * Add VB operator tests * Delete dead code from VB operators * Add comparison operator tests * Add more tests * Fix VB handling of DBNull.Value * Do all number parsing in the invariant culture * Workaround https://github.com/dotnet/coreclr/issues/8648 * Accomodate for -0.0 change and fix test comparisons Commit migrated from dotnet/corefx@1c34aea
There seems to have been a regression with the printing of 0/-0 (maybe this was a deliberate change??)
Anyway, fixed