Skip to content

Fix high precision decimal calculations in v3.13 (#3898) #3929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

stevenaw
Copy link
Member

@stevenaw stevenaw commented Sep 11, 2021

Fixes #3898
Fixes high-precision decimal calculations by internally using decimal when appropriate.

I tried a few approaches here, and the one I found worked best while maintaining full backwards compatibility is to compare expected and actual as decimals when at least one of the values is decimal and BOTH are within the valid values for decimals (between decimal.MinValue and decimal.MaxValue).

I also tried to treat all floating point operations as decimal internally when expected and actual are within valid range, however this caused some a change in failure messages when comparing two double or float values (or a double to an int, etc) with a tolerance since the number of decimal places in the "+/-" portion of the error message changed. This approach broke some existing tests in nunit, and likely would've broken others as well.

NOTE: After merging, this should also be ported to the main development branch too

@stevenaw stevenaw marked this pull request as draft September 12, 2021 14:58
@stevenaw stevenaw force-pushed the stevenaw/3898-decimal-precision-in-numerics branch from 2800ab8 to 4dc416f Compare September 14, 2021 08:45
@stevenaw stevenaw marked this pull request as ready for review September 14, 2021 12:08
@rprouse rprouse added this to the 3.13.3 milestone Sep 14, 2021
@rprouse rprouse added the requires:backport Requires backportint to the v3 branch label Sep 14, 2021
@stevenaw stevenaw merged commit 1c8f5a8 into nunit:v3.13-dev Sep 15, 2021
@realfaisalk
Copy link

realfaisalk commented Oct 22, 2021

EDIT - It could be that my tolerance level is too high so the code tries to "compensate" by adding more decimal places?

Hi,
Will this change fix the issue where doubles are being compared and the assertion seems to "change" a value by 19 or 20 decimal places?

using System;
using NUnit.Framework;

public class Program
{
	public static void Main()
	{ 
		var myCheck = EqualExample.EqualStringIgnoreCase();
	}
	
	public static class EqualExample
	{
		[Test]
		public static bool EqualStringIgnoreCase()
		{
			bool myCheck = false;
			double expected = -0.09404701272000002;
			double actual = -0.09;

			Console.WriteLine("BeforeExpected = " + expected + " BeforeActual =  " + actual);

			Assert.That(actual, Is.EqualTo(expected).Within(0.001));
			//Assert.That(actual, Is.EqualTo(expected));
			
			myCheck = true;
			Console.WriteLine("AfterExpected = " + expected + " AfterActual =  " + actual);
			
			return myCheck;
		}
	}
}

Result:

_BeforeExpected = -0.09404701272000002 BeforeActual = -0.09
Unhandled exception. NUnit.Framework.AssertionException: Expected: -0.094047012720000017d +/- 0.001d
But was: -0.089999999999999997d
Off by: -0.0040470127200000205d

at NUnit.Framework.Assert.ReportFailure(String message)
at NUnit.Framework.Assert.ReportFailure(ConstraintResult result, String message, Object[] args)
at NUnit.Framework.Assert.That[TActual](TActual actual, IResolveConstraint expression, String message, Object[] args)
at NUnit.Framework.Assert.That[TActual](TActual actual, IResolveConstraint expression)
at Program.EqualExample.EqualStringIgnoreCase()
at Program.Main()_

@stevenaw
Copy link
Member Author

Hi @realfaisalk
Thanks for asking. I see in your edit that you suspect the tolerance in your example is the reason for your result, and I think you're right. Those numbers differ by 0.004 which is greater than your tolerance of 0.001. You may need a higher tolerance, such as 0.01, for your example test to pass.

@realfaisalk
Copy link

@stevenaw - Yup, exactly right. The tolerance was asked for by the customer but changed to 0.01 due the the 2dp numbers.
The "But was: -0.89999...." message threw me off. Don't know if it should be "But was: -0.09000000...." or "Your actual number does not have enough dps for the tolerance you set" or similar?

@stevenaw
Copy link
Member Author

Oh that's a good point @realfaisalk thanks for pointing that out! That could probably be made clearer. Would you be willing to file a GitHub issue about it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires:backport Requires backportint to the v3 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants