Add difference to numeric assertion failure messages

[iliashkolyar]

…ctual and expected

IMPORTANT

• The code complies with the Coding Guidelines for C#.
• The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
• If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
• If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
• If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.

Fixes #1412.

[coveralls]

Pull Request Test Coverage Report for Build 2009772673

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 48 of 48 (100.0%) changed or added relevant lines in 9 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.1%) to 95.861%

---

Totals
Change from base Build 2004200011:	-0.1%
Covered Lines:	11775
Relevant Lines:	12100

---

💛 - Coveralls

[dennisdoomen]

Nice work. I think we should try to fine-tune the output a little bit more.

And please check the code coverage. We should at least remain at the same level.

[dennisdoomen]

❓ Would it be possible to keep the number of digits in the fraction similar to the numbers being compared?

[iliashkolyar]

The difference is not consistent in different runtime environments (for example in .NET4.7 the difference is 0.1 and in .NET6 the difference is the one you see above).
Something similar to what exists in the FormatterSpecs file.
I changed the test to use 0.1*, but this is just a workaround.

Regarding your suggestion, how do you see it happening?
What if the subject has X numbers in the fraction and the expected has Y digits?
Feels to me that this is not properly defined and could lead to misunderstandings (without even addressing the runtime issue mentioned above).

[dennisdoomen]

Maybe by rendering the run-time value and determining the number of fractional digits and taking the highest number? It's just a thought.

[dennisdoomen]

One more thought. Apparently there's a subtle difference between differs and difference.

[dennisdoomen]

🤔 I think you've misunderstood me. What I meant is that if the subject and expectation are both under 10, you don't need FA to show you the difference.
🤔 I wonder why you decided to introduce so much complexity if the CalculateDifferenceForFailureMessage can determine itself whether it's worth to return a difference for small numbers or return null.

[iliashkolyar]

I wanted a single location to decide whether or not to generate the "difference message", which logically should be in the GenerateDifferenceMessage method.
Unfortunately, I could not simply run the logic on the result that is returned from CalculateDifferenceForFailureMessage, as the result is a T on which I cannot perform < and > operations.
Therefore I went with the approach of comparing the T difference with concrete values returned from the Minimal/Maximal methods.

Anyway, your suggestion is a lot simpler.
I'll add logic to specific CalculateDifferenceForFailureMessage methods to return null if we subject and expectation are under 10 (int, uint, short, ushort, long, ulong).
And for the rest (decimal, float, double, byte, sbyte) we will always return the message.
WDYT?

[dennisdoomen]

Yeah, sounds like a good idea.

[iliashkolyar]

What about messages like the following:
Expected customer.Age to be less than 21, but found 22 (difference of 1)
Don't you feel that for small differences (say less than 10) the "difference part" is a bit redundant?

[dennisdoomen]

Yes, I agree. But I can imagine that when you have large numbers like 12345784, a difference of 3 might be less obvious to the eye.

[dennisdoomen]

⛏️ I'm nitpicking, but I think you don't need the work comparison.

[jnyrup]

I didn't get time to look deeper into this, but we probably need to handle overflow in the difference calculation.

int.MinValue.Should().Be(int.MaxValue);

Expected int.MinValue to be 2147483647, but found -2147483648 (difference of 1).

[jnyrup]

For all integral types I think (only fiddled for a couple of minutes) we could upcast the values to decimal.

private protected override string CalculateDifferenceForFailureMessage(byte expected)
{
    decimal difference = ((decimal)Subject) - expected;
    return difference != 0 ? difference.Tostring(CultureInfo.InvariantCulture) : null;
}

Not all floats and doubles are representable by decimal

private protected override string CalculateDifferenceForFailureMessage(float expected)
{
    try
    {
        decimal difference = ((decimal)Subject) - expected;
        return difference != 0 ? difference.Tostring(CultureInfo.InvariantCulture) : null;
    }
    catch (OverflowException)
    {
        return null;
    }
}

This should allow us to print differences that are not presentable with the type itself.
E.g. byte.MinValue - byte.MaxValue is -255, but -255 is not representable as a byte.

[iliashkolyar]

For all integral types I think (only fiddled for a couple of minutes) we could upcast the values to decimal.

private protected override string CalculateDifferenceForFailureMessage(byte expected)
{
    decimal difference = ((decimal)Subject) - expected;
    return difference != 0 ? difference.Tostring(CultureInfo.InvariantCulture) : null;
}

Not all floats and doubles are representable by decimal

private protected override string CalculateDifferenceForFailureMessage(float expected)
{
    try
    {
        decimal difference = ((decimal)Subject) - expected;
        return difference != 0 ? difference.Tostring(CultureInfo.InvariantCulture) : null;
    }
    catch (OverflowException)
    {
        return null;
    }
}

This should allow us to print differences that are not presentable with the type itself. E.g. byte.MinValue - byte.MaxValue is -255, but -255 is not representable as a byte.

Would it be possible to handle these types of tweaks in a future PR?
This PR already holds a lot of content and is becoming a bit hard to maintain (conflicts-wise).

[jnyrup]

Would it be possible to handle these types of tweaks in a future PR?
This PR already holds a lot of content and is becoming a bit hard to maintain (conflicts-wise).

Sure and thanks for pointing out that my comments begin to scope creep 👍
If you remove the try/catch+checked from this PR, then handling overflow and related tests can be postponed to another PR.

To get this PR ready:

• As the PR title is used when we generate release notes for GitHub releases, it should be a complete sentence, probably quite close to the line you added to releases.md
• The code coverage should not decrease.

[iliashkolyar]

Would it be possible to handle these types of tweaks in a future PR?
This PR already holds a lot of content and is becoming a bit hard to maintain (conflicts-wise).

Sure and thanks for pointing out that my comments begin to scope creep 👍 If you remove the try/catch+checked from this PR, then handling overflow and related tests can be postponed to another PR.

To get this PR ready:

• As the PR title is used when we generate release notes for GitHub releases, it should be a complete sentence, probably quite close to the line you added to releases.md
• The code coverage should not decrease.

1. I've updated the PR title. Tell me if that is OK.
2. I'm trying to see how my PR decreased the code coverage.
   Looking at the report I see files that were not modified in this PR, so i'm probably missing something.
   Could you please point me to the uncovered code that is related to my PR?
3. I was hoping that we could merge this PR in its current state and deal with the non-integral edge cases later on.
   The current state includes handling Overflows and IMO merging it like this will provide better functionality then removing the Overflow handling all-together.
   I believe that handling Overflows (without the edge cases in presentable differences) is better than not handling them at all.
   WDYT?

[jnyrup]

1: The new title is fine.

2: You'll have to login to Coveralls with your GitHub account to see the lines.
Here's a screenshot

3: I forgot that [u]short and [s]byte are automatically upcasted to int when performing a subtraction, so they are fine.
Is it intentional that you didn't add checked to unsigned integrals?

[jnyrup]

Not sure what goes on with Code Coverage.
It says that coverage decreased, but doesn't list any files with decreased coverage 😕
cc: @eNeRGy164

[dennisdoomen]

It says that coverage decreased, but doesn't list any files with decreased coverage

It does. The DoubleAssertions.CalculateDifferenceForFailureMessage isn't covered. And NullableDoubleAssertions isn't covered in .NET 6.0

[iliashkolyar]

It says that coverage decreased, but doesn't list any files with decreased coverage

It does. The DoubleAssertions.CalculateDifferenceForFailureMessage isn't covered. And NullableDoubleAssertions isn't covered in .NET 6.0

@dennisdoomen, I'm not sure where it is stated or how it is calculated, but I just verified again.

1. The following test covers DoubleAssertions.CalculateDifferenceForFailureMessage.
2. The following test covers NullableDoubleAssertions.CalculateDifferenceForFailureMessage (In .NET6.0 as well)

[dennisdoomen]

Weird.

[jnyrup]

Are you looking at the newest run Dennis?
I recall seeing the missing coverage you refer to in an older coverage report.

[dennisdoomen]

I've just requeued it and this is the result:

[iliashkolyar]

I've just requeued it and this is the result:

Since the actual code coverage is missing from the file, I don't know which lines it refers as not covered.
As I indicated in the comment above, the CalculateDifferenceForFailureMessage is covered.
The only other option is the IsNaN method, but it is also covered by the following test.
So bottom line, I don't really know what's missing.