Optimization for NUnit3

[frblondin]

The pull request providing the ability to generate fixed name for NUnit3 has just been merged (thanks again @zvirja).

I'm working on a project containing 8,000+ tests. The discovery phase is longer than a minute. In Visual Studio there is a separate process for discovering the tests: during this phase there is no point to create the fixture nor generate the parameter values since they won't be used (we just generate a test name TestName(auto<string>).

I successfully tested the following:

1. Have the ability to use a constructor for AutoData that takes a Func<IFixture> parameter instead of IFixture. See this line. For backward compatibility the existing constructor does still exist.
2. The IFixture factory is only invoked when argument values are required, and argument values are generated only if we are not in a discovery process. See this util type.

Please let me know if this is something that could be merged in this project, in which case I can submit a pull request. Wanted to ask you first before spending time on the pull request.

The outcome I observed is important since the discovery went from more than a minute down to a few seconds.

Let me know

[zvirja]

I've reviewed your suggestion thoroughly and here is my reply.

Have the ability to use a constructor for AutoData that takes a Func parameter instead of IFixture.

We already have the exactly same issue to make Fixture creation lazy and win some time: #843. It's queued to the v4 scope and will be implemented soon - I'm already working on that. As a part of that implementation I'll ensure that test arguments are created only if that is required by the ITestMethodBuilder (currently the lazy IEnumerable<> is passed).

However, that will not fix the issue you describe completely. Even if you pass lazy enumerable, the FixedNameTestMethodBuilder will still enumerate it as the TestCaseParameters constructor requires the argument values.

To solve the issue you suggest the following hack:

internal static class TestAdapterHelper
{
    private const string Discovery = "discovery";

    internal static bool IsDiscovery { get; private set; }

    static TestAdapterHelper()
    {
        try
        {
            var processName = Process.GetCurrentProcess().ProcessName;
            IsDiscovery = CultureInfo.InvariantCulture.CompareInfo.IndexOf(processName, Discovery) != -1;
        }
        catch
        {
        }
    }
}


private TestCaseParameters GetParametersForMethod(IMethodInfo method)
{
    ...
    var parameterValues = !TestAdapterHelper.IsDiscovery ? GetParameterValues(parameters) : new object[0];
    ...
}

I don't like that way at all. It's VERY tricky as you provide NUnit with invalid arguments and expect that they will not be used. However, that is never guaranteed by NUnit. This behavior might be also changed in future version of NUnit and our integration will become broken because of that. Also it could happen that some third-party runner will have discovery in the process name - in this case you will break test execution. The approach is very weak and could be easily broken.

I vote to have safe optimizations only, while this one is absolutely unsafe, even if it provides with very good results.

Rather, I'd suggest to resolve it in the following way:

1. AutoFixture implements laziness, so IFixture is created (and arguments are resolved) only if needed. That might already help to win some time as attributes might be created more than once.
2. You will create your own ITestMethodBuilder strategy that doesn't resolve values if current process is discovery, returning the empty enumerable instead (the hack you suggested). Later you will use this custom strategy in your tests as described here.

Everybody will win from this approach - you will be provided with a way to perform a tricky and unsafe optimization, while we, as maintainers, will not worry about compatibility. If later NUnit will become broken due to this hack, you could quickly fix that in your project.

This is how I see this issue. If you wish, we could invite other @AutoFixture/core members and wheter they have different vision on this.

[frblondin]

We already have the exactly same issue to make Fixture creation lazy and win some time: #843. It's queued to the v4 scope and will be implemented soon - I'm already working on that. As a part of that implementation I'll ensure that test arguments are created only if that is required by the ITestMethodBuilder (currently the lazy IEnumerable<> is passed).

🥇

However, that is never guaranteed by NUnit. This behavior might be also changed in future version of NUnit and our integration will become broken because of that. Also it could happen that some third-party runner will have discovery in the process name - in this case you will break test execution. The approach is very weak and could be easily broken.

In reality the process name is the one from the runner - in my case the one from Visual Studio. If the process name changes the worst that would happen is that we won't properly detect that we are in the discovery phase (existing behavior). It seems very unlikely that a process named discovery will also be used as a runner.

As you said now that you are implementing the lazy behavior I'll be able to do the implementation I want by creating my own ITestMethodBuilder implementation.

Don't know if my additional details above clarify anything, I guess you can confirm whether you don't want me to create a pull request about that... and close this issue anyways.

[zvirja]

In reality the process name is the one from the runner - in my case the one from Visual Studio. If the process name changes the worst that would happen is that we won't properly detect that we are in the discovery phase (existing behavior). It seems very unlikely that a process named discovery will also be used as a runner.

You rely on an assumption that NUnit doesn't use real argument values during the test discovery. While this might be so in the current versions, it could be changed in future (e.g. to hash values for some checksums, etc). In this case the optimization might totally break NUnit in unpredictable way.

Another point is that process name is a very weak assumption as there too many different runners and environments - might don't know about them all (and it's cool that we shouldn't). You could affect not discovery only, but the execution as well.

From the supportability perspective the suggested optimization will create a dept, so I'm reluctant to accept it..

Given that you'll have a way to achieve the desired behavior via custom ITestMethodBuilder, I'd persist on avoiding this feature in the core. That is a good tradeoff, as for me.

@frblondin Hope you understand and agree with my concern. Personally, I love optimizations very much and feel cool when we could boost some particular area. But we should be able to analyze situation from different perspectives to ensure that we'll not reach potential implications in the future. 😕

[frblondin]

Understood 👍

[frblondin]

FYI here is my implementation - just tested succeffully :-)

    public class SkipFixtureWhenDiscoveryMethodBuilder : ITestMethodBuilder
    {
        private const string Discovery = "discovery";

        internal static bool IsDiscovery { get; private set; }

        static SkipFixtureWhenDiscoveryMethodBuilder()
        {
            try
            {
                var processName = Process.GetCurrentProcess().ProcessName;
                IsDiscovery = CultureInfo.InvariantCulture.CompareInfo.IndexOf(processName, Discovery) != -1;
            }
            catch
            {
            }
        }

        private readonly FixedNameTestMethodBuilder _fixedNameBuilder = new FixedNameTestMethodBuilder();

        public virtual TestMethod Build(IMethodInfo method, Test suite, IEnumerable<object> parameterValues, int autoDataStartIndex)
        {
            if (method == null) throw new ArgumentNullException(nameof(method));
            if (parameterValues == null) throw new ArgumentNullException(nameof(parameterValues));

            parameterValues = SkipAutoFixtureParametersIfInDiscovery(method, parameterValues, autoDataStartIndex);
            return _fixedNameBuilder.Build(method, suite, parameterValues, autoDataStartIndex);
        }

        private static IEnumerable<object> SkipAutoFixtureParametersIfInDiscovery(IMethodInfo method, IEnumerable<object> parameterValues, int autoDataStartIndex)
        {
            if (IsDiscovery)
            {
                return parameterValues.Take(autoDataStartIndex) // Only take parameter values that are not managed by AutoFixture
                   .Concat(method.GetParameters().Skip(autoDataStartIndex).Select(_ => default(object)));
            }

            return parameterValues;
        }
    }

[zvirja]

@frblondin Thanks for sharing your implementation - looks pretty clean. It's awesome that new AutoFixture API allows you to easily perform such kind of optimizations saving the discovery time! 😃