Better error message for generic test where type parameter cannot be determined

[mikkelbu]

This is a corner case that I only ran into in due to work on the analyzers in nunit/nunit.analyzers#54, and my guess is that very few will run into this problem. Nevertheless, I think we could improve the error message, and fix seems simple and safe.

Given a generic test method with no parameters we get a failure that is difficult to interpret for the users (see last example below for more detail). E.g. the test

        [Test]
        public void TestWithGenericTypeThatFailsWithUnusableMessage<T>()
        {
        }

fails with System.InvalidOperationException : Late bound operations cannot be performed on types or methods for which ContainsGenericParameters is true.

I propose that we change the code at

nunit/src/NUnitFramework/framework/Internal/Builders/NUnitTestCaseBuilder.cs

Lines 221 to 229 in db5bee0

	if (testMethod.Method.IsGenericMethodDefinition && arglist != null)
	{
	Type[] typeArguments;
	if (!new GenericMethodHelper(testMethod.Method.MethodInfo).TryGetTypeArguments(arglist, out typeArguments))
	return MarkAsNotRunnable(testMethod, "Unable to determine type arguments for method");
	testMethod.Method = testMethod.Method.MakeGenericMethod(typeArguments);
	parameters = testMethod.Method.GetParameters();
	}

so that we also return MarkAsNotRunnable(testMethod, "Unable to determine type arguments for method") if arglist == null instead of failing hard later on.

Context
The following test is executed without problem, as NUnit determines that int can be used as T.

        [TestCase(1, ExpectedResult = 1)]
        public T TestWithGenericTypeThatWorks<T>(T arg1)
        {
            return arg1;
        }

But if we cannot determine the type of T using the parameters of the method

        [TestCase(1, ExpectedResult = 1)]
        public T TestWithGenericTypeThatFailsWithUsableMessage<T>(int arg1)
        {
            return default(T);
        }

then we mark the test as not runnable with the message Unable to determine type arguments for method.

However, if the method does not have any parameters

        [Test]
        public void TestWithGenericTypeThatFailsWithUnusableMessage<T>()
        {
        }

then we fail with the message System.InvalidOperationException : Late bound operations cannot be performed on types or methods for which ContainsGenericParameters is true. and a stacktrace

at System.Reflection.RuntimeMethodInfo.ThrowNoInvokeException()
   at System.Reflection.RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at NUnit.Framework.Internal.Reflect.InvokeMethod(MethodInfo method, Object fixture, Object[] args) in C:\src\nunit\nunit\src\NUnitFramework\framework\Internal\Reflect.cs:line 266

[Augure34]

I'd like to contribute and I'm starting it.
Edit: I'm assuming I need some kind of approval to publish the branch before the PR since I get a 403?

[mikkelbu]

Hi @Augure34 Thanks for taking this issue. I did not see the edit, as we only get notifications about new messages, but not for edits.

Regarding your question then I'm guessing that you have just cloned this repo directly which is the reason why you cannot push the branch (only members of this project can create branches in this repo). So you'll need to:

• create a fork of this repo,
• clone your fork to your computer,
• create the changes on a new branch,
• push this branch to github,
• create the PR
  This is described in more detail in e.g. https://thenewstack.io/getting-legit-with-git-and-github-your-first-pull-request/

[Augure34]

Hey @mikkelbu, thanks you for your help (and the link to the article).
I've done this small modification and created a PR, however I wanted to extend the code coverage to this case as well as making sure I didn't break anything, but some tests are already failing without any changes. Is it normal? Or should I look for something on my side of things?

Thanks again

[mikkelbu]

Hi @Augure34 I can take a look at it tonight (in 4 hours time), but if it is the failing tests on the build server then I don't think it is your fault (only Linux is failing), but I've not looked at your changes yet. If you have local failing tests then I'm more worried 😄.

Ps you can run the tests by invoking build.ps1 -t Test if you are on windows and similarly using build.sh for Linux, see e.g. https://github.com/nunit/nunit/blob/master/BUILDING.md.

[Augure34]

Thank you I'll have a look with my local changes, don't waste your time on this it's probably my fault :) I was using VS test explorer, I may get a different result with your script

[Augure34]

I have 6 errors in unit tests using the powershell script. They are due to my computer culture being set in french, failing string checks in exception messages that's all.

VS tests explorer launch tests that are meant to fail that's why I was so worried :)

The build output from DevOps isn't very helpful though (no failed tests name) I guess I need you to check the Tests runs report ;).

[mikkelbu]

Regarding the 6 errors, please create an issue for them with the failing tests, as this should be corrected, but not as part of this PR. We have had the problem before (in #2895 and #2228) and we also have an issue for a more robust solution - #2900.

Regarding executing all tests in VS test explorer, then you are correct, as far as I can remember we have that "issue" both in the Console - nunit/nunit-console#347 - and in the framework - #2297.

Ps. I've forced a rebuild on the build-server and also linked the PR with this issue (by editing the first comment in the PR).

[seanpoulter]

I've been looking for a Good First Issue. Can I help in any way?