Use framework prefilter for massive perf improvement when running fraction of tests

[jnm2]

Following up from nunit/nunit#2867: I had cut inner-loop test lag in continuous testing of a single no-op test down to nine seconds for a work project, but I could go no further without getting NUnit to skip invoking test case sources that are outside the filter.

@CharliePoole has accomplished this by introducing discovery-time filtering in the framework and integrating with it in the NUnitLite runner. This pattern should be easy to follow in all the other runners: nunit/nunit#2878

For @oznetmaster, this has resulted in roughly a 12x performance boost: nunit/nunit#2878 (comment). At work, I'm expecting something similar. I'd really love to help get this in the next releases of the VS adapter and the console runner.

@OsirisTerje Is this something you're interested in, or would you accept a PR?

[jnm2]

Here's a thought I just had: would this have to be done in the engine? (Same issue: nunit/nunit-console#438)

[CharliePoole]

@JMM2 Typing that as we speak. 😄

[CharliePoole]

For the nunitlite+framework implementation, we (mainly @jnm2 and I) decided that the framework would accept both pre-filters and filters. It would not generate any pre-filters automatically. NUnitLite actually does that. The user may specify a filter or a pre-filter on the command-line. If a filter is specified without a pre-filter, then we generate a pre-filter that eliminates, where possible, any tests that would be eliminated by the filter, before they are ever created. That's where the performance gain comes.

Before adding this to the adapter, I think we have to make a general decision about the role of the engine. Options seem to be...

1. Engine does nothing. Any runner that wants automatic prefilters must create them. The engine will pass them through to the framework as it does with any argument.

2. Engine provides an option to generate pre-filters. Any runner that wants one generated from the filter will have to ask for it. This could be done by providing a new service, accessible to runners or with less change to the API by adding a package setting that causes it to happen.

3. Engine automatically generates prefilters in the same situation where nunitlite does it, using equivalent code.

4. Same as 3 with an option to suppress the filter generation.

It should be noted that the pre-filtering will only work for NUnit 3 tests built against the framework release or greater where we actually support it.

If runners want to support manual filters, they have to provide a way for the user to specify them at the command-line or otherwise.

[jnm2]

I like option 4, maybe with the word ‘override’ rather than ‘suppress.’ If it's possible for the engine to generate safe defaults that no one could argue with, there would be an escape hatch to help with cross-project synchronization in case of bugs or optimization ideas.

[CharliePoole]

I used the word "suppress" because there is already an "override" implicit in the ability of the runner to actually provide a manual-prefilter. But it's no big deal. That's the option I prefer as well.

It seems like this point of design has to be decided first. If the engine will do it, then the engine part has to be completed before the adapter can use it.

[jnm2]

@rprouse, @OsirisTerje As project leads, what do you think?

[ChrisMaddock]

I agree this is the engine responsibility. That will also give the advantage to third-party runners.

[OsirisTerje]

@jnm2 @CharliePoole I'm positive, just go on.

But, with source based discovery in VSTest/VS, how much benefit will it give then?

[jnm2]

@OsirisTerje The performance benefit that I'm seeing comes mainly from skipping the invocation and enumeration of TestCaseSources. Source-based discovery does not invoke TestCaseSources, correct?

[OsirisTerje]

Correct, but they are not being run either on discovery, only on execution.

[jnm2]

@OsirisTerje Execution is where the performance matters, so I'm not sure how to answer the question about source-based discovery. Is the discovery method going to have an effect on the ability to prefilter at execution time?

[CharliePoole]

Forgetting temporarily about source-based discovery, let's consider how this works for any GUI. Most GUIs need to display all the tests and therefore need to discover all of them. So no filter is used at discovery time in order to display the tests. Then, at execution time, a simple test filter is used, if requested by the user. So there is no pre-filter (discovery filter) for a GUI.

However, the VS adapter works differently. Due to the peculiarities of the Test Explorer, NUnit has to re-discover the tests at execution time, leading it to do twice as much work as a normal GUI. We might want to use a pre-filter for that second discovery for the sake of efficiency. However, if we did, the test ids would not match those generated n the first discovery that used no filter.

My conclusion is that this feature is probably not very useful, maybe not useful at all, for the adapter.

[OsirisTerje]

If this only applies to TestCaseSource then it doesnt matter what IDs it will get, because source based discovery doesnt try to understand TestCaseSources. It only see the node itself, and will not expand it into its individual test cases

[jnm2]

My conclusion is that this feature is probably not very useful, maybe not useful at all, for the adapter.

This would be terrible news for me, since the adapter is the only runner involved in my original request to skip test case sources. Given a VS filter or test list, it seems like it should be just as possible to skip invoking sources as it is with NUnitLite.

[CharliePoole]

@OsirisTerje As I tried to explain, the problem is more fundamental than source-based discovery or expansion of TestCaseSource. Prefiltering means ignoring some Types and Methods entirely, which is not what you normally do in a gui. I don't know how source-based discovery interacts with nunit's own discovery, so I'll leave that extension to you.

@jnm2 Source-based discovery aside, I can see how a manual pre-filter could help you, perhaps specified in the runsettings file. I don't quite see how it could be applied automatically. I could be wrong. Someone who works on the adapter should think this through at a detailed level.

[jnm2]

I was imagining that the adapter would do the equivalent of this:

            else if (options.TestList.Count > 0)
            {
                var prefilters = new List<string>();


                foreach (var testName in options.TestList)
                {
                    int end = testName.IndexOfAny(new char[] { '(', '<' });
                    if (end > 0)
                        prefilters.Add(testName.Substring(0, end));
                    else
                        prefilters.Add(testName);
                }


                runSettings[FrameworkPackageSettings.LOAD] = prefilters;

https://github.com/nunit/nunit/blob/dcdc37bf030c744fb6cdccde8ea148b8a092aadf/src/NUnitFramework/nunitlite/TextRunner.cs#L334-L347

We're given a test list when running tests from Test Explorer. We'd have to be more clever when running with a command-line VSTest filter, but that could be done separately.

[CharliePoole]

@jnm2 Warning: I'm writing this from memory of the code. I haven't been on the VS extension team for almost a year, so things may have changed. Somebody who is thoroughly familiar with it (i.e. @OsirisTerje ) has to look at this and figure it out.

The pre-filter is a discovery filter. At the point in time that the adapter is called with a set of tests to execute, discovery has already taken place. We could, as I wrote earlier, try to shortcut the second discovery, which the adapter performs as part of execution.

However, IIRC, the adapter uses test ids to indicate to NUnit what tests should be executed. The ids are assigned sequentially during the first discovery. If the second discovery used a pre-filter, then the ids would change. The two stage process that is forced on us by the Test Explorer only works by virtue of our guaranteeing that the second discovery matches the first discovery.

The only way around this that I can see would be to stop using ids and use test names instead. We would then have to live with the ambiguity that is possible when using names.

But again... let's give @OsirisTerje time to analyze this since he's the only one who completely understands the current operation of the adapter!

[ChrisMaddock]

I have even less knowledge of the adapter - but thought it was worth mentioning that there is an open framework issue to provide persistent test IDs, which sounds like it may help. 🙂

[CharliePoole]

Yes, that would definitely help with the first versus second discovery issue.

Of course, what would help most would be for Microsoft to redesign Test Explorer so we don't have to run discovery twice. This has been discussed with MS in the past, but I don't imagine it will happen. IMNSHO I think the only good answer is a native Visual Studio extension for NUnit that doesn't depend on Test Explorer.

[rprouse]

I'm jumping in fairly late here, but I favor doing it in the engine (option 4) too.

[jnm2]

Just throwing this out there: instead of putting the overridable default prefilter logic in the engine, what if we put it in the framework itself?

[CharliePoole]

That makes me a little nervous. In general, where we have a policy option (i.e. more than one way to do something) the framework just does what it's told. The decision is made at a higher level. This, BTW, is different from V2, where the framework read the settings file. Now it just follows package settings.

In fact, in light of the discussion of the adapter, I'm wondering if the engine should do it automatically or perhaps require a positive switch from a runner. If, as it seems, automatic generation of a pre-filter would break the adapter, then it seems like we shouldn't be doing it. Of course, the adapter could use a setting to turn off the breaking behavior, but that sounds backwards to me.

[jnm2]

If, as it seems, automatic generation of a pre-filter would break the adapter, then it seems like we shouldn't be doing it.

I am skeptical that the adapter could break, since the adapter explicitly passes no filter or test list to the engine during the VSTest discovery phase. During the VSTest execution phase when a filter is passed to the engine, how could the engine break the adapter with a safe prefilter any more than NUnitLite is broken when it procedurally generates a prefilter?

[CharliePoole]

I could be wrong, but this is what I envision:

1. In the discovery phase, no filter is used (unless we allow specifying it in the .runsettings file, which is a different use case).

2. All the test information is stored in the VS TestCases that are created out of the NUnit test cases.

3. In the execution phase, we get a list of VS TestCases. Normally, we would re-do discovery and then run the tests using an id filter, BUT using a prefilter...

4 We redo discovery using a prefilter that is composed of the test names from all the tests selected. The returned tests have different ids from those that were originally discovered, since we are bypassing non-selected classes and methods. Ids are assigned sequentially.

5. When we run using an id filter we either run different tests or don't find any tests of a specific id at all.

Note that I'm not saying it can't be made to work by changing how we select tests, just that it won't work the way we now do things.

[jnm2]

I don't recall the adapter using NUnit test IDs. At any rate, it appears to run using an FQN filter, not an id filter:

nunit3-vs-adapter/src/NUnitTestAdapter/NUnitTestFilterBuilder.cs

Lines 32 to 43 in 28b9b85

	public TestFilter MakeTestFilter(IEnumerable<TestCase> testCases)
	{
	if (testCases.Count() == 0)
	return NoTestsFound;
	ITestFilterBuilder filterBuilder = _filterService.GetTestFilterBuilder();
	foreach (TestCase testCase in testCases)
	filterBuilder.AddTest(testCase.FullyQualifiedName);
	return filterBuilder.GetFilter();
	}

[CharliePoole]

If that's the case, then it may not present the problem I thought it would. Depends on what the FullyQualifiedName looks like. At one time, it was provided by the adapter and stored by Test Explorer, but I seem to remember that changed. If it's a class name or class plus method name, then it can be used in a pre-filter.

That said, I'd still rather see the decision about whether to have a filter generated automatically kept in the hands of the runner. Just an opinion. I'm neither a team member nor a user any longer.

[kristofferkarlsson]

Tried this using NUnit3TestAdapter 3.15.0-dev-01121 and it works, with notable speed improvements for my test projects.

[OsirisTerje]

Thanks @kristofferkarlsson, for checking this out !

[OsirisTerje]

@MatthewBeardmore Release 3.15 is out now :-)

[OsirisTerje]

@MatthewBeardmore The prefiltering fails with SetupFixture, see https://github.com/nunit/nunit3-vs-adapter/issues/647

@jnm2 Do you have any knowledge of this change and what's been done in the NUnit Console - which should be equal ?

[OsirisTerje]

This issue was included in 3.15.0, and had some sideeffects, see #651
Version 3.15.1 has been released now, and the prefiltering is then by default turned off. To turn it on, add a runsetttings file. See https://github.com/nunit/docs/wiki/Tips-And-Tricks#PreFilter for details.

[CharliePoole]

@rprouse @jnm2 @OsirisTerje This issue is still hanging around in the nunit-console project. I'd appreciate your thoughts on whether it still needs to be active and what, if anything, ought to be done for 4.0.

[OsirisTerje]

@CharliePoole I wonder if we really need the discovery phase at all? Earlier it was needed as it was a part of the test explorer, and a way to fill the explorer tree before running tests, also in order to save time. Now, there is source code based discovery, and most test projects are actually being slower doing both discovery and execution. If discovery is gone, then we don't need prefilters either. Alternatively, discovery could be optional. This is the same I have talked about above, but with more time now with source based discovery and the latest changes in the adapter, the extra discovery phase there is just a nuisance.

[CharliePoole]

@OsirisTerje Am I wrong in thinking that Source=Based discovery is only available in paid versions of Visual Studio?

[OsirisTerje]

Source based discovery should be in all versions. Live unit testing though is only in Enterprise versions, but we don't need that one. It is only source based discovery that is needed.

[CharliePoole]

OK... in that case, can Source-based discovery discover the arguments for each test method? They would have to know a lot about NUnit internals to do that, so it seems unlikely. So even if you were able to avoid calling NUnit in the discovery phase, this issue would still apply to you in the execution phase when nunit loads all the tests before running a subset of them.