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

TestFixtureSource arguments usage in the TestCaseSource method #3593

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

skomialek
Copy link

Hi,

I have found a need to use some information specific to TestFixture I'm in during TestCaseSource generation code.
It might be something that is being discussed in #2950

The solution presented here allows to reference arguments passed to TestFixtureData in the TestCaseSource attribute.

Please see sample usage in TestCaseSourceWithTestFixtureArgumentsTests.

The fixtures are created with data as follows

        public static IEnumerable<TestFixtureData> TestFixturesSource
        {
            get
            {
                for (int maxArgs = 0; maxArgs <= NUMBER_OF_ARGS; maxArgs++)
                {
                    string[] args = FixtureArgumentsSource.Take(maxArgs).ToArray();

                    yield return new TestFixtureData(maxArgs, args).SetArgDisplayNames($"{maxArgs}_arguments_test");

                }
            }
        }

So that each fixture contains different number of arguments. In order to make test case source aware one can ask for the maxArgs value by referencing TestFixtureArgumentRef.Arg0.

        [Test]
        [TestCaseSource(nameof(TestCaseDataWithFixtureContext), new object[]{ TestFixtureArgumentRef.Arg0 })]
        public void FixtureArgumentTest(int argumentIndex, int numberOfArguments)
        {
            Assert.AreEqual(FixtureNumberOfArgs, numberOfArguments);
            Assert.AreEqual(FixtureArgs[argumentIndex], FixtureArgumentsSource[argumentIndex]);
        }

        public static IEnumerable<TestCaseData> TestCaseDataWithFixtureContext(int numberOfArgsFixtureWasCreatedWith)
        {
            for (int i = 0; i < numberOfArgsFixtureWasCreatedWith; i++)
            {
                yield return new TestCaseData(i, numberOfArgsFixtureWasCreatedWith)
                    .SetArgDisplayNames($"Number of args : {i + 1} of {numberOfArgsFixtureWasCreatedWith} ");
            }
        }

In this case the TextFixtureArgumentRef.Arg0 gets replaced with respective test fixture argument before the TestCaseDataWithFixtureContext method is called, which allows to generate test cases specific for given fixture.

The TextFixtureArgumentRef enumeration allows addressing up to 10 parameters (0-9). IF there is a need for more one can cast any integer value like (TextFixtureArgumentRef) 22 which will simply get replaced by value of fixture argument at index 22.

@dnfadmin
Copy link

dnfadmin commented Jul 12, 2020

CLA assistant check
All CLA requirements met.

@skomialek
Copy link
Author

Hi @rprouse / @jnm2 / @mikkelbu ,

I'm not sure If I'm missing something in the process on how to contribute to NUnit, but this PR has been sitting here for a while.

Any input here would be appreciated.

@rprouse
Copy link
Member

rprouse commented Sep 10, 2020

Sorry @skomialek this is totally on us. Everyone on the team has been focused on our day jobs and haven't been as involved as we need to be. We'll do our best to give this the review it deserves as soon as we can. Please bear with us and thank you for your contribution.

@skomialek
Copy link
Author

Thank you @rprouse for the update. I completely understand. I just wanted to make sure that I have not forgotten to press any button or follow some process before/after submitting the PR. As long as everything is in place, there is no rush - the PR is not going anywhere.

PS. I'm not sure I like the sound of "give this the review it deserves" :D

@rprouse rprouse added this to In progress in 3.13 Release via automation Dec 11, 2020
@rprouse rprouse added this to the 3.13 milestone Dec 11, 2020
@rprouse rprouse self-assigned this Dec 11, 2020
@rprouse rprouse removed this from the 3.13 milestone Dec 28, 2020
@rprouse rprouse removed this from In progress in 3.13 Release Dec 28, 2020
@TrxIsVerCtrl
Copy link

TrxIsVerCtrl commented Mar 13, 2021

Any idea when this feature will be available in nunit3? I have similar requirement and this would help a lot.

@skomialek
Copy link
Author

@rprouse - is there anything I can/should do in order to get this feature in the release? Is there anything missing?

Thank you.

@rprouse
Copy link
Member

rprouse commented Apr 3, 2021

@skomialek I admit that I've been avoiding this PR. The syntax of this doesn't feel right to me but I'm having trouble putting why it makes me feel that way into words and I also don't have alternatives. We need to be very careful when accepting new features because we need to continue to support them in the future. We have several features in the framework that I regret accepting. They confuse users and we end up having to spend too much time answering questions or correcting misconceptions.

I can say that an arbitrary limit of 10 parameters called Arg0 to Arg9 seems very unintuitive to me 😄

Can you give me a more concrete example of the problem this solves for you? I can see it in the abstract, but I'd like more of a real world example. I'd also like to see an idea of how you would propose we document this feature so that users understand it and it is clear what problem it will solve for them.

@nunit/framework-team does anyone else have opinions on, or better syntax ideas for this feature?

@skomialek
Copy link
Author

Thank you for the feedback. I completely understand feature management problem.

Let me start with the problem explanation which in my case is simple test organization.
I generate tests dynamically for the chain of builds that verify dependency for each of them.

So I do have multi-layer structure where for each build I need to validate bunch of registered dependencies.

foreach(var buildId in buildIds)
{
    foreach(var dependencyId in GetDependencies(buildId)
    {
        Assert.IsTrue(IsValid(dependencyId));
    } 
}

I do have multiple options here:

  • one test iterating all the cases - this is the least favorable option as the first run will not report all failures making the turnaround time for fixing problems very long.
  • using test case source - this is much better - I get one test per dependency, but because of the volume, it is becoming a very long, flat list... it is also hard to search / filter.
  • using fixture of every build and then generate tests for every dependency using test data source. This is the scenario that this code enables me to do - generate tests in context of argument passed to the fixture generation. I'm getting nicely organized results grouped by build (one fixture per build) and then have specific tests generated for every dependency to be verified.

This way I get nice filtering, reporting and overview of the tests :)

The use can be generalized to any two (or more) layer hierarchy (enumerating folders and files, databases and tables etc.).
Some sample scenario descriptions from others:

#2950 (comment)
#2707 (comment)

You can also refer to the sample from this comment : #2950 (comment)

About the implementation of Arg0 ... Arg9 - it actually does support any number of arguments by casting int to the enum type, but I agree it might feel awkward.

I wast trying to get a very explicit declaration of intent here and not to work with 'magic index' approach like negative numbers etc. Unfortunately, because we are in the context of attribute the nicer approach of having dedicated class :

public class FixtureArgument
{
    public int Index { get; }

    public FixtureArgument(int index)
    {
    }

    public static FixtureArgument At(int index)
    {
        return new FixtureArgument(index);
    }
}

Will not work because the only things we can pass to the attribute constructor is constant expressions which can be evaluated at compile time.

image

So the solution is driven by language limitation, but if there is anything that could potentially improve this, I'm all in. Enum based solution is where my C# superpowers ended :)

@Dreamescaper
Copy link
Member

Dreamescaper commented Apr 8, 2021

Just an idea. We could add some attribute like TestFixtureSourceArgumentsAttribute, which could be applied to method's object[] parameter. We could detect this attribute, and provide all fixture arguments as as argument.

So it would look something like that:

        [TestCaseSource(nameof(TestCaseDataWithFixtureContext))]
        public void FixtureArgumentTest(int argumentIndex, int numberOfArguments)
        {
        }

        public static IEnumerable<TestCaseData> TestCaseDataWithFixtureContext([TestFixtureSourceArguments] object[] fixtureArguments)
        {
            int numberOfArgsFixtureWasCreatedWith = (int)fixtureArguments[0];

            for (int i = 0; i < numberOfArgsFixtureWasCreatedWith; i++)
            {
                yield return new TestCaseData(i, numberOfArgsFixtureWasCreatedWith)
                    .SetArgDisplayNames($"Number of args : {i + 1} of {numberOfArgsFixtureWasCreatedWith} ");
            }
        }

Wdyt?

@skomialek
Copy link
Author

skomialek commented May 14, 2021

@Dreamescaper , this is definitely an option that opens more door and I must say I like the approach, One could even take it to the level of using attributes to point to specific argument

        [TestCaseSource(nameof(TestCaseDataWithFixtureContext))]
        public void FixtureArgumentTest(int argumentIndex, int numberOfArguments)
        {
        }

        public static IEnumerable<TestCaseData> TestCaseDataWithFixtureContext([TestFixtureSourceArgument("argumentIndex")] int argumentIndex, [TestFixtureSourceArgument] numberOfArguments)
        {
            int numberOfArgsFixtureWasCreatedWith = numberOfArguments;

            for (int i = 0; i < numberOfArgsFixtureWasCreatedWith; i++)
            {
                yield return new TestCaseData(i, numberOfArgsFixtureWasCreatedWith)
                    .SetArgDisplayNames($"Number of args : {i + 1} of {numberOfArgsFixtureWasCreatedWith} ");
            }
        }

Where the default constructor can just match arguments by name (if named the same). That would eliminate the need for user addressing parameters by their index (which is also the downside in my suggestion that I'm not really happy with).

The reason I selected the path of indexed access is already exiting feature of test case source with arguments, which passes parameters array in the attribute constructor.

(from official NUnit Doc)

public class MyTestClass
{
    [TestCaseSource(nameof(TestStrings), new object[] { true })]
    public void LongNameWithEvenNumberOfCharacters(string name)
    {
        Assert.That(name.Length, Is.GreaterThan(5));
        bool hasEvenNumOfCharacters = (name.Length / 2) == 0;
    }

    [TestCaseSource(nameof(TestStrings), new object[] { false })]
    public void ShortName(string name)
    {
        Assert.That(name.Length, Is.LessThan(15));
    }

    static IEnumerable<string> TestStrings(bool generateLongTestCase)
    {
        if (generateLongTestCase)
            yield return "ThisIsAVeryLongNameThisIsAVeryLongName";
        yield return "SomeName";
        yield return "YetAnotherName";
    }
}

I have seen advantage of being able to 'inherit' some parameter yet still being able to use test case sources with specific parameters. This way it becomes a part of already established method rather than another way of doing similar thing. Since NUnit is a framework, I believe flexibility of mixing the two approaches might also solve some potential scenarios and I don't really see a reason to force user to choose either you use feature A or B. Obviously, this is my personal opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants