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

TheoryAttribute does not work with generic methods #19

Open
CharliePoole opened this issue Oct 19, 2013 · 16 comments
Open

TheoryAttribute does not work with generic methods #19

CharliePoole opened this issue Oct 19, 2013 · 16 comments

Comments

@CharliePoole
Copy link
Contributor

Setting: NUnit 2.5.3, Console runner, .NET 3.5

TheoryAttribute does not work with generics:

[Datapoint]
public double[,] Array2X2 = new double[,] { { 1, 0 }, { 0, 1 } };

[Theory]
public void TestForArbitraryArray(T[,] array)
{
// ...
}

NUnit gives a warning saying "No arguments were provided".

See discussion at https://bugs.launchpad.net/nunit-3.0/+bug/537914

@CharliePoole CharliePoole added this to the 3.0 milestone Mar 2, 2014
CharliePoole added a commit that referenced this issue Sep 1, 2014
Create NUnitLiteFrameworkDriver and modify code to select correct driver

I'm merging this now but the framework PR one will still need another commit so all builds build correctly.
@CharliePoole CharliePoole removed the 3.0 label Mar 26, 2015
@CharliePoole CharliePoole modified the milestones: 3.0Beta2, 3.0 Mar 26, 2015
@CharliePoole CharliePoole self-assigned this Mar 26, 2015
@constructor-igor
Copy link
Contributor

Generic allows to use one test method for different types (see sample below).
Sample: Use one method TestGenericForArbitraryArray for double and int arrays, instead of 2 methods TestDoubleForArbitraryArray and TestIntForArbitraryArray for each type.

    [TestFixture]
    public class TheorySampleTests
    {
        [Datapoint]
        public double[,] Array2X2Double = new double[,] { { 1, 0 }, { 0, 1 } };
        [Datapoint]
        public int[,] Array2X2Int = new int[,] { { 1, 0 }, { 0, 1 } };

        [Theory]
        public void TestDoubleForArbitraryArray(double[,] array)
        {
            Console.WriteLine("TestDoubleForArbitraryArray()");
            // ...
        }
        [Theory]
        public void TestIntForArbitraryArray(int[,] array)
        {
            Console.WriteLine("TestIntForArbitraryArray()");
            // ...
        } 

        [Theory]
        public void TestGenericForArbitraryArray<T>(T[,] array)
        {
            Console.WriteLine("TestGenericForArbitraryArray()");
            // ...
        } 
    }

@CharliePoole
Copy link
Contributor Author

@constructor-igor This seems like a logical workaround. The OP wanted the generic method to pick up all data that could possibly satisfy the generic method. I'm not sure that's really feasible here. I think it would probably work in a generic fixture, where T would actually mean a specific type.

@constructor-igor
Copy link
Contributor

@CharliePoole, please, could you help me to understand how can I help in the issue (status:confirm)?

@CharliePoole
Copy link
Contributor Author

The first step would be to verify that the failure actually occurs. As I commented a while back, it makes sense that NUnit can't find an open generic type. Next would be to create some tests to show exactly what does and doesn't work. You got a good start at that. I think your first and second methods work, but the third does not find any data. Right?

A further step would be to see what happens in a generic fixture, where the method itself uses T but is no longer generic. I am guessing that it would work and would find the specific int or double data.

If we can lay it all out, then we know it's truly a bug. If you want to work on it, we will have to decide how much to fix and how much to simply document.

Is that enought to help you get started?

@constructor-igor
Copy link
Contributor

original test can be reproduced in nunit v3 Beta1

    [TestFixture]
    public class TheorySampleTests
    {
        [Datapoint]
        public double[] ArrayDouble = { 0, 1, 2, 3 };
        [Datapoint]
        public int[] ArrayInt = { 0, 1, 2, 3 };

        [Theory]
        public void TestDoubleForArbitraryArray(double[] array)
        {
            Console.WriteLine("TestDoubleForArbitraryArray()");
            TestGenericForArbitraryArray(array);
        }
        [Theory]
        public void TestIntForArbitraryArray(int[] array)
        {
            Console.WriteLine("TestIntForArbitraryArray()");
            TestGenericForArbitraryArray(array);
        }

        [Theory]
        public void TestGenericForArbitraryArray<T>(T[] array)
        {
            Console.WriteLine("TestGenericForArbitraryArray()");
            // ...
        }
    }
Test Name:  TestGenericForArbitraryArray
Test FullName:  NUnit_v3_samples.TheorySampleTests.TestGenericForArbitraryArray
Test Source:    \NUnit_v3_samples\TheorySampleTests.cs : line 58
Test Outcome:   Failed
Test Duration:  0:00:00.001

Result Message: No arguments were provided

image

@constructor-igor
Copy link
Contributor

Found next workaround: base class with generic test method and number (per type) of test classes

    public class TheorySampleTestsBasic<T>
    {
        [Theory]
        public void TestGenericForArbitraryArray(T[] array)
        {
            Console.WriteLine("TestGenericForArbitraryArray()");
        }
    }
    [TestFixture]
    public class TheorySampleTestsChildDouble : TheorySampleTestsBasic<double>
    {
        [Datapoint]
        public double[] ArrayDouble1 = { 0, 1, 2, 3 };
        [Datapoint]
        public double[] ArrayDouble2 = { 4, 5, 6, 7 };
    }
    [TestFixture]
    public class TheorySampleTestsChildInt : TheorySampleTestsBasic<int>
    {
        [Datapoint]
        public int[] ArrayDouble1 = { 0, 1, 2, 3 };
        [Datapoint]
        public int[] ArrayDouble2 = { 4, 5, 6, 7 };
    }

@CharliePoole
Copy link
Contributor Author

What if you make the generic class a generic fixture, eliminating the derived classes and moving all the datapoints into the one class? The generic fixture could have [TestFixture(typeof(int))] and [TestFixture(typeof(double))] attributes.

@constructor-igor
Copy link
Contributor

ok.
Next way checked: seems works correctly.

    [TestFixture(typeof(int))]
    [TestFixture(typeof(double))]
    public class TheorySampleTestsGeneric<T>
    {
        [Datapoint]
        public double[] ArrayDouble1 = { 0, 1, 2, 3 };
        [Datapoint]
        public double[] ArrayDouble2 = { 4, 5, 6, 7 };
        [Datapoint]
        public int[] ArrayInt = { 0, 1, 2, 3 };
        [Theory]
        public void TestGenericForArbitraryArray(T[] array)
        {
            Console.WriteLine("TestGenericForArbitraryArray()");
        }
    }

image

by the way, Resharper v8 doesn't work correct for the use case:

image

@CharliePoole CharliePoole changed the title TheoryAttribute does not work with generics TheoryAttribute does not work with generic methods Apr 17, 2015
@CharliePoole
Copy link
Contributor Author

Yes, so the bug is really that it doesn't work for Generic methods. When the class is generic, then that method is no longer a generic because T has already been resolved to int or double. I changed the title accordingly.

@CharliePoole
Copy link
Contributor Author

So do you think we should "fix" this or just document it?

For the original example to work, NUnit would have to find all Datapoints of any type and try to use them for the method. That seems a bit extreme.

Another option would be to give Theory an optional type argument, so that the theory was instantiated multiple times, just as we do for a TestFixture. IOW, we would have [Theory(typeof(int))] and [Theory(typeof(double))] in this case. Is that desirable?

Finally, as I say, we could just document what works and what doesn't on the TheoryAttribute page.

@constructor-igor
Copy link
Contributor

I guess, the issue should be supported ("fixed").
In my opinion, if compiler can understand and call generic method (see sample below), then developer expects same behavior in "Theory" attribute.

    [TestFixture]
    public class TheorySampleTests
    {
        [Datapoint]
        public double[] ArrayDouble = { 0, 1, 2, 3 };
        [Datapoint]
        public int[] ArrayInt = { 0, 1, 2, 3 };

        [Theory]
        public void TestDoubleForArbitraryArray(double[] array)
        {
            Console.WriteLine("TestDoubleForArbitraryArray()");
            TestGenericForArbitraryArray(array);
        }
        [Theory]
        public void TestIntForArbitraryArray(int[] array)
        {
            Console.WriteLine("TestIntForArbitraryArray()");
            TestGenericForArbitraryArray(array);
        }

        private void TestGenericForArbitraryArray<T>(T[] array)
        {
            Console.WriteLine("TestGenericForArbitraryArray()");
        }
    }

@CharliePoole
Copy link
Contributor Author

Thanks for checking this out. I think we need to figure out just what to support here, so I'm flagging it as status:design. We use that label when we don't want anyone to just start coding, without first having an agreed-upon specification. In this case, I think we have choices ranging from most complete implementation down to no implementation at all:

  1. NUnit could search for data points of every type that can be used to instantiate the theory. It would have to honor any constraints on the type parameters, which would be new code for us. This approach would be challenging but definitely possible. The main drawback is that it might make generic Theories less useful. The user could not define datapoints in the class of any type that should not be used for this Theory. Requires some thought and some sample code.
  2. We could give TheoryAttribute a type parameter and have the user specify exactly what instances of the Theory should be tested. In our example, we would use [Theory(typeof(int))] and [Theory(typeof(double))] to explicitly cause the theory to be used twice. This would probably be a somewhat easier implementation and we would be treating TheoryAttribute in the same way we currently handle TestFixtureAttribute.
  3. We could simply document the limitation. Rather than generic Theories, users should use a generic class to contain the Theory.

These issues are not mutually exclusive. We also could:

  1. Do both 1 and 2.
  2. Do 3 first, then follow up with 1, 2 or 4.

This issue interacts with #150 and with the fix already made for #377. We should review it in light of those issues as well.

We can discuss which way to go here. Once we set a direction, we should post it here and remove the design status.

@CharliePoole
Copy link
Contributor Author

The documentation has been updated to indicate that generic methods may not be used and explaining the workaround using a generic fixture: https://github.com/nunit/nunit/wiki/Theory-Attribute

I'm moving this issue out of the beta 2 milestone so we can decide which way to go in the next release.

@CharliePoole CharliePoole modified the milestones: 3.0, 3.0Beta2 Apr 24, 2015
@CharliePoole
Copy link
Contributor Author

Looking at the options, I lean towards giving TheoryAttribute a Type argument. Finding all possible types that can be used seems like a step too far in making decisions for the user. Just as the programmer writes [TestFixture(typeof(int))] to determine how a generic fixture will be instantiated, they can write [Theory(typeof(int))] for a generic theory. The above example would then be handled like this:

    [TestFixture]
    public class TheorySampleTests
    {
        [Datapoint]
        public double[] ArrayDouble1 = { 0, 1, 2, 3 };
        [Datapoint]
        public double[] ArrayDouble2 = { 4, 5, 6, 7 };
        [Datapoint]
        public int[] ArrayInt = { 0, 1, 2, 3 };

        [Theory(typeof(int))]
        [Theory(typeof(double))]
        public void TestGenericForArbitraryArray(T[] array)
        {
            Console.WriteLine("TestGenericForArbitraryArray()");
        }
    }

@nunit/core-team What do you think of this approach versus the other? Should this stay in the 3.0 release as a feature? What priority?

@rprouse
Copy link
Member

rprouse commented Apr 24, 2015

I like this alternative. I would be fine with leaving it in 3.0, but I think that it should be low priority for the release. I think we should be concentrating on core-vision type issues for the 3.0 release so we can get it out there and widely used, and then start adding features.

@CharliePoole
Copy link
Contributor Author

OK, I'm making it low and leaving it in the 3.0 milestone. It could just as well end up pushed to 'future'.

@CharliePoole CharliePoole modified the milestones: 3.2, 3.0 Aug 23, 2015
@CharliePoole CharliePoole modified the milestones: 3.2, Backlog Dec 5, 2015
@CharliePoole CharliePoole removed their assignment Jun 26, 2016
@CharliePoole CharliePoole removed this from the Backlog milestone Jul 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants