nunit / nunit.analyzers

Roslyn analyzers for writing unit tests with NUnit
MIT License
88 stars 32 forks source link

DereferencePossiblyNullReferenceSuppressor for custom Is classes #692

Closed TheBigNeo closed 8 months ago

TheBigNeo commented 8 months ago

The DereferencePossiblyNullReferenceSuppressor class suppresses the Possible 'System.NullReferenceException' warning in case of:

Assert.That(exception,           Is.Not.Null);
Assert.That(exception.ParamName, Is.EqualTo("udpClient")); //<< No warning

Would it be possible for this to work for Assume as well?

Assume.That(exception, Is.Not.Null)
string name = exception.ParamName; //<< Possible 'System.NullReferenceException' warning

I have also noticed that if I create my own Is class, as described here, the warning is not suppressed.

I tried to find out why it doesn't work with my Is class, but I can't find anything in the code. In my opinion, it should work, because it always checks for string and not for the namespace of Is.

manfred-brands commented 8 months ago

@TheBigNeo Technically the rule could be augmented to do Assume. The reason it isn't, is simply because I never use Assume. I don't know what the benefit is of using Assume.That(exception, Is.Not.Null) vs Assert.That(exception, Is.Not.Null). The InconclusiveResult is more likely to be ignored than the FailureResult.

Can you supply me with a minimum example of your Is class? As you said the code only checks if the 2nd parameter is not Is.Null.

OsirisTerje commented 8 months ago

@manfred-brands The Assume is a central part of the Theory tests, used to skip irrelevant set of parameters. For that purpose it is essential as it makes you easily see in the result which parameters are valid and tested for, and which are not.

I also rarely use it for normal tests, but it is a part of the nunit framework toolbox, so I guess some people find it useful for expressing pre-conditions.

TheBigNeo commented 8 months ago

We use Assume, for example, to ensure that a property of a class is really false before my SetPropertyTrue() function is called and then checked with Assert. I came across it in the documentary.

Here my Is class:

using System.Net.Sockets;
using System.Numerics;
using BroadcastSuite.Utils.Data.Exceptions;
using BroadcastSuite.Utils.Helpers;
using JetBrains.Annotations;
using NUnit.Framework.Constraints;

namespace BroadcastSuite.Utils.Test.UnitTestHelpers.NUnitConstraints;

public abstract class Is : NUnit.Framework.Is
{
    [AssertionMethod]
    public static EqualConstraint OutOfRangeBetweenExceptionMessage<T>(T outOfRangeValue, string valueName, T from, T to)
        where T : INumber<T>
    {
        return new EqualConstraint($"Value {valueName.WithQuotes()} has to be between {from} and {to} but was {outOfRangeValue}.");
    }
}

/// <summary>
///     Test class for <see cref="Agent.Data.BroadcastSuiteDiscoverer" /> constructor.
/// </summary>
[TestFixture]
public class BroadcastSuiteDiscoverer_01_ConstructorTests : AbstractTest
{
    [TestCase(-1)]
    [TestCase(-10)]
    [TestCase(65536)]
    [TestCase(999999)]
    public void ParamUdpPortIsOutOfRange_ThrowsOutOfRangeException(int udpPort)
    {
        // Test & Assert
        OutOfRangeException exception = Assert.Throws<OutOfRangeException>(
            delegate
            {
                // ReSharper disable once ObjectCreationAsStatement
                new BroadcastSuiteDiscoverer(new UdpClient(), udpPort);
            });

        Assert.That(exception,         Is.Not.Null);
        Assert.That(exception.Message, Is.OutOfRangeBetweenExceptionMessage(udpPort, "udpPort", 0, 65535)); // << "Possible 'System.NullReferenceException'" Warning

        // The old way:
        ExceptionAssert.OutOfRangeBetweenExceptionMessage(udpPort, "udpPort", 0, 65535, exception);
    }
}

/// <summary>
///     Assert functions for exceptions.
/// </summary>
public static class ExceptionAssert
{
    /// <inheritdoc cref="OutOfRangeBetweenExceptionMessage{T}(T,string,T,T,string,string?)" />
    [AssertionMethod]
    public static void OutOfRangeBetweenExceptionMessage<T>(T outOfRangeValue, string valueName, T from, T to, OutOfRangeException exception, string? message = null)
        where T : INumber<T>
    {
        message ??= "Exception message";

        OutOfRangeBetweenExceptionMessage(outOfRangeValue, valueName, from, to, exception.Message, message);
    }

    /// <summary>
    ///     Assert that the <paramref name="actualExceptionMessage" /> is equal to the <see cref="OutOfRangeException.BetweenMessage{T}" />.
    /// </summary>
    /// <param name="outOfRangeValue">The value that is out of range</param>
    /// <param name="valueName">The name of the value that is out of range</param>
    /// <param name="from">The allowed minimum value</param>
    /// <param name="to">The allowed maximum value</param>
    /// <param name="actualExceptionMessage">The actual exception message</param>
    /// <param name="message">The message to display in case of failure</param>
    /// <typeparam name="T">The type of the value that is out of range</typeparam>
    /// <seealso cref="OutOfRangeException" />
    /// <seealso cref="OutOfRangeException.BetweenMessage{T}" />
    [AssertionMethod]
    public static void OutOfRangeBetweenExceptionMessage<T>(T outOfRangeValue, string valueName, T from, T to, string? actualExceptionMessage, string message)
        where T : INumber<T>
    {
        Assert.That(actualExceptionMessage, Is.EqualTo($"Value {valueName.WithQuotes()} has to be between {from} and {to} but was {outOfRangeValue}."), message);
    }
}
manfred-brands commented 8 months ago

@TheBigNeo I tried to reproduce your Is issue, but it seems to work for me. What is the exact error message you get.

image

image

TheBigNeo commented 8 months ago

image

At first, I thought it was a Rider problem. But I see the warning in VS image

If I use #nullable enable in this file, I get another warning. But always. image

// #nullable enable

/// <summary>
///     Test class for <see cref="Agent.Data.BroadcastSuiteDiscoverer" /> constructor.
/// </summary>
[TestFixture]
public class BroadcastSuiteDiscoverer_01_ConstructorTests : AbstractTest
{
    [TestCase(999999)]
    public void ParamUdpPortIsOutOfRange_ThrowsOutOfRangeException(int udpPort)
    {
        OutOfRangeException? exception1 = GetOutOfRangeException();
        OutOfRangeException? exception2 = GetOutOfRangeException();

        ArgumentOutOfRangeException? exception3 = GetArgumentOutOfRangeException();
        ArgumentOutOfRangeException? exception4 = GetArgumentOutOfRangeException();

        Assert.That(exception1,         Is.Not.Null);
        Assert.That(exception1.Message, Is.OutOfRangeBetweenExceptionMessage(udpPort, "udpPort", 0, 65535)); // << "Possible 'System.NullReferenceException'" Warning

        Assert.That(exception2,         NUnit.Framework.Is.Not.Null);
        Assert.That(exception2.Message, Is.OutOfRangeBetweenExceptionMessage(udpPort, "udpPort", 0, 65535));

        Assert.That(exception3,         Is.Not.Null);
        Assert.That(exception3.Message, Is.OutOfRangeBetweenExceptionMessage(udpPort, "udpPort", 0, 65535)); // << "Possible 'System.NullReferenceException'" Warning

        Assert.That(exception4,         NUnit.Framework.Is.Not.Null);
        Assert.That(exception4.Message, Is.OutOfRangeBetweenExceptionMessage(udpPort, "udpPort", 0, 65535));
    }

    private static OutOfRangeException? GetOutOfRangeException()
    {
        return new OutOfRangeException("");
    }

    private static ArgumentOutOfRangeException? GetArgumentOutOfRangeException()
    {
        return new ArgumentOutOfRangeException("");
    }
}
TheBigNeo commented 8 months ago

Ohh, it looks like a ReSharper problem. When I disable ReSharper in VS, I get the same warning as you and with Is.Not.Null no warning.

I have created a ReSharper Issue. https://youtrack.jetbrains.com/issue/RIDER-106099

However, there is still the question about the Assume :)

manfred-brands commented 8 months ago

My company dropped ReSharper a couple of years ago. VS with Analyzer is now better than ReSharper ever was. And way more flexible.

However, there is still the question about the Assume :)

Never combine two items in one ticket. One could get lost with the fluff of the other. Could you do me a favour and split the Assume in its own ticket and then we can close this ticket as not an nunit problem.

TheBigNeo commented 8 months ago

It is difficult to do without ReSharper when using Rider :)

Yep, I'll open a new ticket for Assume.

I apologize for the effort regarding the ReSharper problem.