Framework throws NullReferenceException if test parameter is marked with [Values(null)]

[LeonidVasilyev]

I'm using NUnit 3.9.0, NUnit Console Runner 3.7.0 and NUnit 3 Test Adapter 3.9.0 for Visual Studio Community 2017.

Exception is seems to be thrown by access to Array.Length property in ParamAttributeTypeConversions.GetData(). Values(null) is resolved to ValuesAttribute(params object[] args) call. Issue can be reproduced using following code snippet:

class Foo
{
    [Test]
    public void Bar([Values(null)] int? param)
    {
        Assert.Pass();
    }
}

In Visual Studio test is not discovered during test project build, i.e. it doesn't appear in Test Explorer list of tests. No message is shown in Output window. NUnit Console Runner shows following error message:

System.NullReferenceException: Object reference not set to an instance of an object. at NUnit.Framework.Internal.ParamAttributeTypeConversions.GetData(Object[] data, Type targetType) at NUnit.Framework.Internal.Builders.ParameterDataSourceProvider.GetDataFor(IParameterInfo parameter) at NUnit.Framework.CombiningStrategyAttribute.BuildFrom(IMethodInfo method, Test suite) at NUnit.Framework.Internal.Builders.DefaultTestCaseBuilder.BuildFrom(IMethodInfo method, Test parentSuite) at NUnit.Framework.Internal.Builders.NUnitTestFixtureBuilder.AddTestCasesToFixture(TestFixture fixture) at NUnit.Framework.Internal.Builders.NUnitTestFixtureBuilder.BuildFrom(ITypeInfo typeInfo) at NUnit.Framework.Internal.Builders.DefaultSuiteBuilder.BuildFrom(ITypeInfo typeInfo)

You can check original use case in How to pass NULL value to ValuesAttribute of the test method? discussion on StackOverflow.

[jnm2]

You're right! Thanks for spotting that.

We should be doing the same thing we do for TestCaseAttribute:

nunit/src/NUnitFramework/framework/Attributes/TestCaseAttribute.cs

Lines 52 to 55 in e914415

	if (arguments == null)
	Arguments = new object[] { null };
	else
	Arguments = arguments;

And we should be doing the same test:

nunit/src/NUnitFramework/tests/Attributes/TestCaseAttributeTests.cs

Lines 509 to 513 in e914415

	[TestCase(null)]
	public void NullableSimpleFormalParametersWithNullArgument(int? a)
	{
	Assert.IsNull(a);
	}

This is pretty straightforward. @LeonidVasilyev, are you interested in contributing the fix?

[CharliePoole]

@jnm2 There's a little more to it, I think, although not much. Introduction of ParamAttributeTypeConversions seems to have started it. The GetData method needs a Guard statement and the call should be conditional. I'm a bit surprised that the params argument is the one selected but there's no way we can change that.

[jnm2]

Thanks, yes. The implementer should add a Guard.ArgumentNotNull here to prevent the NRE:

nunit/src/NUnitFramework/framework/Internal/ParamAttributeTypeConversions.cs

Lines 53 to 55 in 04c6b7b

	private static IEnumerable GetData(object[] data, Type targetType)
	{
	for (int i = 0; i < data.Length; i++)

@CharliePoole Which call should be conditional?

[jnm2]

Ah, this one, I think:

nunit/src/NUnitFramework/framework/Attributes/ValuesAttribute.cs

Lines 101 to 104 in 04c6b7b

	public IEnumerable GetData(IParameterInfo parameter)
	{
	return ParamAttributeTypeConversions.ConvertData(data, parameter.ParameterType);
	}

If data is null, what should it return or throw?

[CharliePoole]

I think that call points out the problem. The field data should never be null, it's the args values being provided so it has to be an array. If it's seen to be null at that point, it is meant to be an array of length 1 with a null as it's single member, and there is nothing to be converted.

I'm doubting "good first issue"on this one. Maybe @mikkelbu wants to come back to it. 😸

[jnm2]

Do we even need a check? The only possible cause is if ValueAttribute has a badly implemented constructor, and we'll get the ArgumentNullException then due to the guard we're adding.

Yep, agree.

[CharliePoole]

If we fix the constructor, I agree we can avoid the check.

[jnm2]

Okay. To recap:

We should be doing the same thing in the ValuesAttribute params constructor that we do for TestCaseAttribute:

nunit/src/NUnitFramework/framework/Attributes/TestCaseAttribute.cs

Lines 52 to 55 in e914415

	if (arguments == null)
	Arguments = new object[] { null };
	else
	Arguments = arguments;

And we should be doing the same test:

nunit/src/NUnitFramework/tests/Attributes/TestCaseAttributeTests.cs

Lines 509 to 513 in e914415

	[TestCase(null)]
	public void NullableSimpleFormalParametersWithNullArgument(int? a)
	{
	Assert.IsNull(a);
	}

We should add a Guard.ArgumentNotNull here to prevent the NRE:

nunit/src/NUnitFramework/framework/Internal/ParamAttributeTypeConversions.cs

Lines 53 to 55 in 04c6b7b

	private static IEnumerable GetData(object[] data, Type targetType)
	{
	for (int i = 0; i < data.Length; i++)

And add a test to ParamAttributeTypeConversionTests to ensure this behavior.

@mikkelbu (or @LeonidVasilyev), are you interested in working on this one?

[mikkelbu]

@jnm2 Not particularly 😄, but if I have time then I could take a look at this.
Ps. I'm a bit unsure why my name came up. AFAICR I have not touched any of the files mentioned above.

[jnm2]

Oh, I think I mixed up the two issues. I'll do it if I have time.

[LeonidVasilyev]

@jnm2, thank you for asking. Unfortunately, I'm currently in lack of time needed to setup all environment and study the developer documentation. Maybe I will comeback to this issue if nobody will do this earlier.

[LeonidVasilyev]

I'm a bit surprised that the params argument is the one selected but there's no way we can change that.

@CharliePoole, It appears to be that C# compiler treats constructor with params array as more specific than constructor with single object parameter because every object[] is object but not every object is object[].

[jnm2]

As part of fixing this bug, I copied many tests from TestCaseAttributeTests to ValuesAttributeTests and to my surprise there are at least two new kinds of ValuesAttribute failure to deal with. I'm going to submit PRs granularly.

[ChrisMaddock]

I imagine this overlaps with @StanEgo's ongoing work on #2104. Maybe one to discuss, to avoid duplicating work. 🙂

[jnm2]

Good, thanks for pointing that out! I'll have to rewrite the one I am editing. 😊