Values attribute support for nullable bool and enum types

[AGBrown]

#27 was the original work for ValuesAttribute enum (and bool) support. It would be nice (read: less typing every time I have to do it) to extend this to nullable bool and enums.

Environment: NUnit 3.8, c#, .net 4.5

[CharliePoole]

Should a null be generated as one of the values for an enum?

[jnm2]

I like this. I'd expect null to be generated.

Here's where the change would be made. If we do this, I think we should split the value generation out of this method:

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

Lines 60 to 67 in e94c6d9

	if (targetType.GetTypeInfo().IsEnum && data.Length == 0)
	{
	return Enum.GetValues(targetType);
	}
	if (targetType == typeof(bool) && data.Length == 0)
	{
	return new object[] { true, false };
	}

[CharliePoole]

Whether we do it or not, I don't think the automatic generation of data should be in a method named ConvertData. Automatic generation is a policy decision and needs to be made at a higher level of the software....

if (some condition)
    GenerateDataAutomatically()

Keeping policy decisions separate and at a higher level makes them visible and clarifies the code.

[jnm2]

@CharliePoole I forget. Safe to put this up-for-grabs, or shall we wait for another team member to comment?

[CharliePoole]

You're asking me? 😺 That's crazy!

How does the framework team make decisions about what features it will add? Refer to project lead? Majority? Consensus? I do think that "any two people can agree to add anything" would be a pretty dangerous rule.

If you want my opinion on the feature request, I'd say it's reasonable for explicit if driven by a setting of some kind, but I'd rather see a setting that says which reports to generate rather than one tailored "explicitly" for explicit.

BTW, the old order of reports I referred to was to put the Summary first. That worked better IMO for printing out the reports but the current one is better if you're just watching the output scroll by. Some way to tailor order might be nice.

[jnm2]

@CharliePoole Based on my memory, we've lived dangerously then. I see the governance doc says we should wait at least 72 hours and then assume implicit support if no one objects. 👍

Btw this is the issue about nullable values with [Values].

[CharliePoole]

@jnm2 Sorry, my mind was on a different issue.

Yes, at least the doc says we will try to allow that long. I take "try" to mean we will do it unless there is some need to get it done rather quickly.

For this issue, I see little to argue about. I'd probably tell people not to create nullable enums, but if they exist, they need to be tested. I might lean to leaving out null, which can always be added separately to the parameter, but it's not that important.

[jnm2]

The reason I'd leave null in by default is that if you didn't want it, you would have written [Values] bool myTestParam instead of [Values] bool? myTestParam (same for enum).

[CharliePoole]

That makes sense. BTW, I think nullable bool makes sense. Nullable enum doesn't make sense because you can always define your enum such that the default (zero) value is something like Unspecified, thereby avoiding boxing entirely.

[jnm2]

What if it's a third-party enum, or null indicates "do not call the setter" or "do not add to the property bag" for your test?

[CharliePoole]

I'm focusing on tests, where you are in control. That is, you don't have to use the same enums that are used by the SUT if you don't want to. I'll grant, however, that it could be more convenient to use them. WRT "do not add property" NUnit itself has enums where that's the interpretation we give the default value. It's just a check for a value where otherwise you check for null.

But this is a side discussion, I think the feature is fine. :-)

[mikkelbu]

I think that is a good suggestion (and would also prefer that the null value generated).

[AGBrown]

Should a null be generated as one of the values for an enum?

Late reply, apologies. Yes, my original (badly communicated) intention was that a null value would be generated. So I was seeing this as:

[Test] public void TestNullableBool([Values] bool? testInput)
{ /* runs with null, true, false in no particular order */ }

public enum BananaSize { Big, Small }
[Test] public void TestNullableEnum([Values] BananaSize? bananaForScale)
{ /* runs with null, Big, Small in no particular order */ }

[AGBrown]

Nullable enum doesn't make sense because you can always define your enum such that the default (zero) value is something like Unspecified, thereby avoiding boxing entirely.

I see your point here. In an ideal world I would not allow the use of nullable enums. As c# allows the use of MyEnum? as a type, code in large team projects gets written that way in the systems under test, and we do therefore have to test with all possible values in the input argument set, including null. (You can also have nullable database fields that are represented by a nullable enum in the codebase. So, there are a lot of circumstances where generating a test input of null is useful necessary).

[jnm2]

@AGBrown, could I interest you in doing a PR for this? Charlie and I think we should move the current logic.

I'm partial to putting the logic into a new private static method here, used before we ever call ConvertData:

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

Lines 102 to 105 in 98cc4df

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

@mikkelbu, @CharliePoole What do you think?

[CharliePoole]

I agree that the generating of args and the conversion of args should be separate, but why would you be calling both methods? We only want to generate the data if none is provided, in which case there is nothing to convert.

[jnm2]

@CharliePoole You would never call both. You'd call one if data.Length == 0, and the other if it is not.

[CharliePoole]

OK. I misunderstood "before."

[nisha-kaushik]

Hello,

I want to pick this up for contribution.

To Clarify, right now following test fails with Message: No arguments were provided.

[Test] public void TestNullableBool([Values] bool? testInput)
{ /* runs with null, true, false in no particular order */ }

Expected- It should run with all possible values the input can take in case of Enum and bool.

Please Let me know if there is any misunderstanding at my end.

Thanks,
Nisha

[ChrisMaddock]

Thanks again @nisha-kaushik!

You're correct on the expected behaviour, yes. I'm afraid I don't know the 'current behaviour', from memory!

[mikkelbu]

@nisha-kaushik
Actually, I think the "No arguments were provided" is the behaviour from Resharper or Visual Studio. If you run the test using the nunit-console then you will get one testcase for true and one for false

...
        <test-suite type="ParameterizedMethod" id="0-1004" name="TestNullableBool" fullname="ConsoleApp7.Tests.TestNullableBool" classname="ConsoleApp7.Tests" runstate="Runnable" testcasecount="2" result="Passed" start-time="2018-08-16 08:29:01Z" end-time="2018-08-16 08:29:01Z" duration="0.001875" total="2" passed="2" failed="0" warnings="0" inconclusive="0" skipped="0" asserts="0">
          <test-case id="0-1002" name="TestNullableBool(True)" fullname="ConsoleApp7.Tests.TestNullableBool(True)" methodname="TestNullableBool" classname="ConsoleApp7.Tests" runstate="Runnable" seed="952673480" result="Passed" start-time="2018-08-16 08:29:01Z" end-time="2018-08-16 08:29:01Z" duration="0.000168" asserts="0" />
          <test-case id="0-1003" name="TestNullableBool(False)" fullname="ConsoleApp7.Tests.TestNullableBool(False)" methodname="TestNullableBool" classname="ConsoleApp7.Tests" runstate="Runnable" seed="236076019" result="Passed" start-time="2018-08-16 08:29:01Z" end-time="2018-08-16 08:29:01Z" duration="0.000056" asserts="0" />
        </test-suite>
...

Edit: I've assigned this issue to myself to keep an eye on - let us know if you have any further questions, and we'll help where we can! 😄

[nisha-kaushik]

Hi,
I moved Generate Data logic to a private method in ValuesAttribute. Can you please suggest if we still need conversion in ParamAttributeTypeConversions.cs .

if (targetType.GetTypeInfo().IsEnum && data.Length == 0)
{
return Enum.GetValues(targetType);
}
if (targetType == typeof(bool) && data.Length == 0)
{
return new object[] { true, false };
}

[jnm2]

@nisha-kaushik In the above discussion, we don't think the value-generation logic should be inside a class named ParamAttributeTypeConversions. To answer your question about whether the value generation should be private to ValuesAttribute, can you tell us how many other classes had been referencing ParamAttributeTypeConversions.ConvertData other than ValuesAttribute?

[nisha-kaushik]

I removed initially, But it was used in some test cases that's why wanted to just confirm. I will remove it from ParamAttributeTypeConversions. Thanks.