Execution.CheckFilter implementation when 2000 test cases are exceeded

[jjachimowicz]

NUnit 3.12.0
NUnit3TestAdapter 4.5.0
.NET 6.0 (Windows command line with dotnet test)

It has already been discussed in the past (#998 and others) that exceeding 2000 test cases with FullyQualifiedName filter will cause the execution of all tests regardless of the filter. As I understand this is supposed to address issues when running tests from Visual Studio when the test filter gets too huge like described in #660, but why is that limitation also applied when running tests from console with just a single FullyQualifiedName filter?

Couldn't this be applied only when running tests from Visual Studio?

The Execution.CheckFilter() method already supports returning a raw filter when TestCategory filter is included. In that case AssemblySelectLimit doesn't matter and individual test cases from filterBuilder.FilterByList() are not returned. Why isn't this behaviour used when amount of loaded test cases (without TestCategory filter) exceeds the limit and we are not executing from Visual Studio? The calling code already verifies if it's an IDE execution or not and behaves differently.

Today we have a workaround of adding TestCategory filter with a fictive category to achieve such behavior (passing raw filter). Alternative with increasing AssemblySelectLimit probably should be avoided since this will create a very large test filter, with thousands of entries instead of our short filter for one category.

P.S. Please also note that even though all tests are executed, the result from dotnet test shows the filtered tests only, not all test cases (same problem as point 3 in #998). This is very misleading when trying to identify the problem.

[OsirisTerje]

Sure, this could be improved

Today we have a workaround of adding TestCategory filter with a fictive category to achieve such behavior (passing raw filter). Alternative with increasing AssemblySelectLimit probably should be avoided since this will create a very large test filter, with thousands of entries instead of our short filter for one category.

Not sure I understand the issue here, nor the workaround.

Isn't the most easy way just set the AssemblySelectLimit to a very high number ? That eliminates the logic.

[OsirisTerje]

@jjachimowicz Any comments?

[jjachimowicz]

@OsirisTerje I'll get back to you soon.

[jjachimowicz]

This took a bit longer than anticipated but @cyanite and I gave this a look and we think we have an idea how this can be fixed. We will create a pull request based on our findings and assumptions.

[OsirisTerje]

Before you raise that PR, please explain what you're thinking, also respond to my comment above.
And, please add a suitable repro for this, add it to the nunit3vstestadapter.issues repo.

And, what do you mean by "raw filter" ?
And, have you looked at the Where filter?
And last, I hope: The assembly select limit is not there just for the TE, it also is meant to apply to dotnet test too. The issue happens between the adapter and the engine/framework.

[cyanite]

Hi @OsirisTerje,

(FYI, me and @jjachimowicz are colleagues; we debugged this together.)

Here is how the adapter behaves currently (on master), when invoked from dotnet test.

We start test with e.g. dotnet test SomeAssembly.dll "--filter:(FullyQualifiedName~Some.Namespace)"
Let's say this namespace contains 3000 tests.

The adapter then does the following. Method names in parenthesis, * means overridden for the non-IDE case.

1. (RunTests(sources...)) Converts this dotnet-test filter to an NUnit filter (in some cases, including this).
2. If no filter was given, uses Settings.Where instead.
3. (RunAssembly) Does discovery with this filter (finding 3000 tests, in this case).
4. (Run*, CheckVsTestFilter) Recreate the filter (like step 1), but in more situations.
5. (Run*) If filter is the special "no tests found", skip. This doesn't happen even with 0 tests found.
6. (Run, CheckFilterInCurrentMode*, CheckFilter) If the filter doesn't contain a category and discovery found too many cases (>2000), clear the filter.
   Even though the filter is small and simple.
7. Otherwise, if the filter contains any category, handle explicit tests and return.
8. Otherwise, convert the entire filter into a list of cases. Even though the filter is small and simple before this. Here we again check if the list is too long, even though that can't happen due to step 6.

So the end result is that all filters get converted into, usually much longer, filters with a list of cases except
if it contains a category of any kind, in which case it will be left completely alone.

I don't understand why this would be desired. As I understand it, the test length check is to avoid passing in too
long filters. But this is not applicable when calling from dotnet test, since the only instance that creates long
filters, is the adapter itself, in step 8.

We instead propose this flow (referencing the steps above):

1. Unchanged.
2. Unchanged.
3. Unchanged.
4. Unchanged.
5. Unchanged (although this seems to not work as intended).
6. If the filter contains any category, handle explicit tests (as before).
7. Removed.
8. Removed.

Note that all this only covers the dotnet-test flow (non-IDE mode). For the IDE-mode, nothing will change (although there is some redundant work there, which we also address in the PR).

[OsirisTerje]

Appreciate your efforts here!

I believe that what you're looking at is the code here:

Your 6 => A in the image, 7 => B and 8 => C.

I don't remember exactly the reason for the code in C. So it might be worth a try to change this to return the original filter instead.

B is the handling of explicits, which you suggest to remove, but that would remove the explicit handling, so that's a no-go.

The filter in your case from discovery should be something like this: <filter><test re='1'>somenamespace</test></filter>, which is created by the Tokenizer.
The Tokenizer is a recent addition, so it might be that section C should have been redone at that time.

Also, the check at A and B should then be something else than IsCategory, also covering for other short filters. I need to look more at the tokenizer to see what each of the VSTest filters is being translated to. Note that we have had situations with extremely long VSTest filters too, so it is not a case of just looking at the existence of such a filter.

This is also a part of the code that have been (earlier) very hard to test. So as part of any changes here, there need to be a better test suite to cover all changes.

There are tests in the Acceptance tests that could be extended to cover all situations, that is one solution to the test issue at hand.

This code has evolved over a decade, and there are multiple different situations that needs to be considered. Any change should be as small as possible, and thoroughly tested.

We have also discussed whether we really need the discovery phase at all, but the work involved to ensure the adapter works without it has been too much.

[cyanite]

B is the handling of explicits, which you suggest to remove, but that would remove the explicit handling, so that's a no-go.

No no, I don't. My step 6 retains that logic unchanged. As for your "A", that's my step 5. This step doesn't trigger if you, for instance, provide a filter that doesn't enumerate any tests (but this is a separate point, and I don't propose to fix or change that; also retained.)

Also, the check at A and B should then be something else than IsCategory, also covering for other short filters.

In this code path, all filters will come from the command line, so how aren't they all short? If they are long, it's because someone explicitly provided a long filter on the command line. Filters that come from Visual Studio doesn't contain categories and similar, only test cases.

Note that we have had situations with extremely long VSTest filters too, so it is not a case of just looking at the existence of such a filter.

Right, but since people explicitly chose to provide a long filter, I don't see how it's NUnit's job to "shorten" it (by removing it). If it performs poorly, people should provide shorter filters. Also, the current code actually makes many filters much longer due to step 8. Also, the check implemented currently in step 6 isn't about the filter at all: It's about the number of tests the filter discovers. It can be the smallest filter in the world, but since it'll discover many tests, the adapter decides to clear it.

Edit: Here is the commit (proposed PR), for reference: edlund-as@4cf4b5a please ignore the .gitattributes part :)

[OsirisTerje]

Right, but since people explicitly chose to provide a long filter, I don't see how it's NUnit's job to "shorten" it (by removing it). If it performs poorly, people should provide shorter filters.

Actually, there are tools that do this, e.g. Jetbrain's TeamCity have a feature which does this (we have a separate issue around that). So, no, it might not be the user itself, we simply don't know. All we see is a long list.

Looking at the code, I am not sure why you do what you do. I might misunderstand what the pieces are intended to do though.

• The Current Mode and Legacy mode difference gone?
• No limit on IDE either ?

Can you add some tests to check where this is actually ending up?

I understand it might solve your immediate problem, but wouldn't it be better to just allow more filters than a category filter to be excluded from the assemblyselectlimit? One could add a property to the testfilter that sets what kind of filter has been applied, and use that to check whether it can be forwarded or not.

[cyanite]

Actually, there are tools that do this, e.g. Jetbrain's TeamCity have a feature which does this (we have a separate issue around that). So, no, it might not be the user itself, we simply don't know. All we see is a long list.

Right, ok. But as I tried to explain, the current filter limiter doesn't look at the filter at all, but only on the number of tests it results in. It doesn't seem to make much sense, since short filters can, and often do, result in many cases. So if you really want to avoid long filters in that situation, I suppose look at the textual or structural length. Because when you say "All we see is a long list", no we don't. We see a filter. The long list comes from our own discovery, and there is no reason for us to turn that into a list at all, since we have the original filter.

The Current Mode and Legacy mode difference gone?

Not in any way that matters. After the simplifications, those two code paths become the same in some cases, so there is no need to have a method with if (CurrentMode) return x; else return x;. If you are refering to the method CheckFilterInCurrentMode, what I did is remove the filter checked when we are in IsDiscoveryMethodCurrent, since that is destroying the perfectly good filters you get passed in for no good reason.

No limit on IDE either ?

Same thing. There are no material changes in the IDE path, if you look closely. I'll add a new comment below which explains the flow before and after. I'll also look at the test suite. However, I have the problem currently, that I can't build using build.ps1, because it tries to target .NET standard with C# constructions that are too new:

C:\src\nunit3-vs-adapter\src\NUnitTestAdapter\IAdapterSettings.cs(8,41): error CS1514: { expected [C:\src\nunit3-vs-adapter
\src\NUnitTestAdapter\NUnit.TestAdapter.csproj]
C:\src\nunit3-vs-adapter\src\NUnitTestAdapter\IAdapterSettings.cs(119,2): error CS1513: } expected [C:\src\nunit3-vs-adapte
r\src\NUnitTestAdapter\NUnit.TestAdapter.csproj]
C:\src\nunit3-vs-adapter\src\NUnitTestAdapter\IAdapterSettings.cs(8,41): error CS1514: { expected [C:\src\nunit3-vs-adapter
\src\NUnitTestAdapter\NUnit.TestAdapter.csproj]
C:\src\nunit3-vs-adapter\src\NUnitTestAdapter\IAdapterSettings.cs(119,2): error CS1513: } expected [C:\src\nunit3-vs-adapte
r\src\NUnitTestAdapter\NUnit.TestAdapter.csproj]

These refer to bracket-less namespace declarations, which are not valid for older C#.

[cyanite]

Existing flow when entering through a list of assemblies and filter, so, from dotnet test (not via the IDE). This list itself is repeating from an earlier comment.

1. (RunTests(sources...)) Converts this dotnet-test filter to an NUnit filter (in some cases, including this).
2. If no filter was given, uses Settings.Where instead.
3. (RunAssembly) Does discovery with this filter (finding 3000 tests, in this case).
4. (Run*, CheckVsTestFilter) Recreate the filter (like step 1), but in more situations.
5. (Run*) If filter is the special "no tests found", skip. This doesn't happen even with 0 tests found.
6. (Run, CheckFilterInCurrentMode*, CheckFilter) If the filter doesn't contain a category and discovery found too many cases (>2000), clear the filter.
   Even though the filter is small and simple.
7. Otherwise, if the filter contains any category, handle explicit tests and return.
8. Otherwise, convert the entire filter into a list of cases. Even though the filter is small and simple before this. Here we again check if the list is too long, even though that can't happen due to step 6.

The first issue comes in step 6, where we clear the filter based on how many tests it enumerates, even though this has no connection at all with the size of the filter. In fact, from dotnet test, most filters will be short, but can still enumerate many tests.

The next issue is in step 8, where we insist on converting the filter to a list of tests, which will almost always make the filter much longer than it was. There is no reason I can see, why this is done in non-IDE mode. In IDE mode, it will already be a list at this point, making it also superfluous.

Note that if we are randomly lucky enough to have a category in the filter, we are saved from this conversion by step 7, which simply returns out of the method early (for no reason I can tell).

So, my proposed PR removes the conversion to list and the clearing of filters than enumerate too many tests. That's it for non-IDE mode. I'll add another comment for how IDE mode is affected (which it almost isn't).

[cyanite]

The IDE-mode run is much simpler:

1. (RunTests(tests...)) Converts the list of cases into a list-of-cases filter (so "case=c1|case=c2...") unless there are too many cases, in which case we clear it.
2. (RunAssembly) Does discovery with this filter.
3. (Run, CheckFilterInCurrentMode*, CheckFilter) If we are in current discovery method, if the filter doesn't contain a category (and it never does, in IDE mode) and discovery found too many cases (>2000), clear the filter. Even though
   the filter will already be cleared in step 1 in that case, so this is a waste of time.

Here step 1 is the same as non-IDE mode, and step 2 here is the same as step 3 above, and step 3 here is a variant of step 6 above (the CheckFilter method is shared).

My PR proposal will simply remove step 3, since it does nothing we didn't already do.