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
Optimization for NUnit3 #858
Comments
I've reviewed your suggestion thoroughly and here is my reply.
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 However, that will not fix the issue you describe completely. Even if you pass lazy enumerable, the 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 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:
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. |
🥇
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 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. |
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 @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. 😕 |
Understood 👍 |
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;
}
} |
@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! 😃 |
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:
Func<IFixture>
parameter instead ofIFixture
. See this line. For backward compatibility the existing constructor does still exist.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
The text was updated successfully, but these errors were encountered: