Skip to content
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

Within throws ArgumentException if null value is involved #4400

Closed
BrightLight opened this issue Jun 10, 2023 · 3 comments · Fixed by #4482
Closed

Within throws ArgumentException if null value is involved #4400

BrightLight opened this issue Jun 10, 2023 · 3 comments · Fixed by #4482

Comments

@BrightLight
Copy link
Contributor

We tried to compare Nullable<DateTime> with a tolerance using Within. If the actual value is null, we get an ArgumentException.

Example:

[TestCase("2023-01-01 13:00:10", "2023-01-01 13:00:00", 10)] // succeeds
[TestCase(null, "2023-01-01 13:00:00", 10)] // throws
[TestCase(null, null, 10)] // succeeds
public void DatesShouldBeTheSame(DateTime? value, DateTime? expectedDateTime, int toleranceInSeconds)
{
  Assert.That(value, Is.EqualTo(expectedDateTime).Within(toleranceInSeconds).Seconds);
}

Callstack:

System.ArgumentException : Both arguments must be DateTime, DateTimeOffset or TimeSpan
   at NUnit.Framework.Constraints.DateTimes.Difference(Object x, Object y)
   at NUnit.Framework.Internal.TextMessageWriter.WriteDifferenceLine(Object expected, Object actual, Tolerance tolerance)
   at NUnit.Framework.Internal.TextMessageWriter.DisplayDifferences(Object expected, Object actual, Tolerance tolerance)
   at NUnit.Framework.Constraints.EqualConstraintResult.DisplayDifferences(MessageWriter writer, Object expected, Object actual, Int32 depth)
   at NUnit.Framework.Constraints.EqualConstraintResult.WriteMessageTo(MessageWriter writer)
   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 MyUnitTests.DatesShouldBeTheSame(Nullable`1 value, Nullable`1 expectedDateTime, Int32 toleranceInSeconds) in ...

The exception happens when the "Off by:" is calculated. It seems DateTimes.Difference does not consider/expect that one of the values might be null.

internal static object Difference(object? x, object? y)
{
    if (x is DateTime xDateTime && y is DateTime yDateTime)
        return (xDateTime - yDateTime).Duration();

    if (x is TimeSpan xTimeSpan && y is TimeSpan yTimeSpan)
        return (xTimeSpan - yTimeSpan).Duration();

    if (x is DateTimeOffset xDateTimeOffset && y is DateTimeOffset yDateTimeOffset)
        return (xDateTimeOffset - yDateTimeOffset).Duration();

    throw new ArgumentException("Both arguments must be DateTime, DateTimeOffset or TimeSpan");
}

Is this the intended behavior? It would be nice if Within could support DateTime? as well. The message could simply omit the "Off by:" part and just read

Expected: 2023-01-01 13:00:00 +/- 00:00:10
  But was:  null
@OsirisTerje OsirisTerje added Investigate We will look into this is:bug and removed Investigate We will look into this labels Jun 12, 2023
@OsirisTerje
Copy link
Member

OsirisTerje commented Jun 14, 2023

Repro in NUnit.Issues.
It fails this way only with Nullable Datetime

@BrightLight Since you have found the part of the code that fails, would you consider raising a PR for the fix ?

@BrightLight
Copy link
Contributor Author

Sure. I'm still learning GitHub, but I will give it a try.

@ashishdawale20
Copy link
Contributor

Hello @OsirisTerje & @BrightLight ,
I have seen no activity since long time hence created the PR #4482

@OsirisTerje OsirisTerje added this to the 4.0 milestone Oct 7, 2023
OsirisTerje pushed a commit that referenced this issue Oct 7, 2023
* Changes to fix issue 4400

* Fixing the testcase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants