Test Adapter runs explicit tests when TFS TestCaseFilter is used

[CharliePoole]

See the original issue filed against the 2.0 adapter: nunit/nunit-vs-adapter#71

[CharliePoole]

The 2.0 issue, nunit/nunit-vs-adapter#71, suggests two ways to deal with this problem:

1. Translate the TFS filter to an NUnit Filter.
2. Review the list of test names received from the filter after they are discovered by NUnit and remove any that are marked explicit.

Upon further review, (2) is not a solution. Tests need to be removed only if the filter does not explicitly select them. So, we have to parse the filter expression in either case... for (1) so we can generate the nunit filter and for (2) so we can retroactively correct the selection made by the filter in the first place. If we have to do so much work for (2) then I would lean toward (1) as the best approach.

That decided, however, another big choice looms!

In the V2 adapter, we implemented the use of MsTest-style filtering because it was the easiest approach. NUnit V2 didn't have it's own filtering language.

But in NUnit 3.0, we already have a filtering DSL, which is used by the console runner and is implemented as a service of the TestEngine. In fact, we could trivially transform an expression like "cat==Data || Priority == High" into a set of NUnit test filters.

We could also, with a bit more work, make a copy of our existing code to parse our DSL and change it to parse MsTest-style filters. As with our existing code, we would emit an nunit filter that honored the Explicit attribute. Of course, this would give us two DSLs to support and for users to remember.

Options:

1. Continue to use the MsTest filter language but parse it ourselves rather than leaving it to VS.
2. Replace it with our own DLS as used in the --where option of the console runner. This could be justified by the fact that the breaking change is being made prior to the 3.0 release.
3. Support both, which is obviously more work.
4. Support a hybrid of the two by allowing both MsTest-style expressions and NUnit expressions in the filter. This is probably the most work and might confuse users.

@OsirisTerje @rprouse What do you think?

[jnm2]

I can't get TestCategoryFilter to function at all in TFS 2015.1 build vNext with NUnit adapter 3.0.9.0. Is this related?

[CharliePoole]

@jnm2 No, this issue is a pretty well-known problem. I haven't tried vNext at all. If the problem persists, you may want to file another issue, showing details of what you are doing and what is happening (or not).

[CharliePoole]

Any rational way forward on this issue involves parsing the filter expression.

Parsing the filter expression requires deciding on the filtering DSL we want to support. The leading choices are our own existing DSL and the MsTest one.

Nobody is giving any feedback on the choice here, so I'm pulling the issue out of the milestone. Perhaps we can refer it to the general community for a future release.

[CharliePoole]

@OsirisTerje Nobody has ever responded to my request for comment on this, so I think it's up to us to decide what to do. Do you think we should continue to support the MsTest-style filter expressions here, require use of our own expressions as used in NUnit3-Console's --where option or support both?

Any option that supports MsTest-style filters requires us to parse them, of course.

I'd like to either decide to move ahead with this in a certain direction or simply to say we are not planning to fix it.

[adamralph]

I raised nunit/nunit#1664 and was directed here.

Can some please confirm whether this issue also represents the problem purely in the VS Test Explorer, regardless of TFS?

I'm not using TFS, but when I run "all tests" in the VS Test Explorer, tests marked Explicit are being run.

[rprouse]

@adamralph yes this is an issue with just VS Test Explorer also.

[adamralph]

Thanks @rprouse.

That's not at all obvious from the title and description. Would it be worth refining, to make it easier to find for anyone else having the same problem? This is the reason I couldn't find it when I searched, and why I raised nunit/nunit#1664 instead.

[dnperfors]

@CharliePoole shouldn't MsTest-style filters be supported in the vs-adapter anyways, so that it correctly works from within Visual Studio and TFS-Build?
I hope this issue can be solved soon, since we get quite a lot of problems on our buildserver (which is effectively using the VS Test Explorer)

[CharliePoole]

@rprouse Are you sure @adamralph 's problem is the same? In nunit/nunit#1664 he talks about using Run All from the IDE and doesn't mention any use of filters. Obviously it is an adapter issue, but I'm not so sure it's the same as this one.

@adamralph Can you comment on this? If we can't draw a connection with this one - i.e. if no filters are involved - then we will need to confirm it separately. It's not a problem I have ever seen. Specifically, the NUnit 3 Test Demo project contains tests marked explicit and they are handled as expected.

[ChrisMaddock]

I believe @adamralph's problem may also be with the v2 adapter rather than this one, I had a quick dig earlier.

@adamralph - is that correct, the issue was with the NServiceBus tests, running NUnit v2.6.4?

[CharliePoole]

@dnfperfors There's no requirement of supporting MsTest style filters in order to run under TFS and the IDE. The user supplies the filter as an expression and it's up to the adapter to interpret it.

The solution to this problem requires a significant amount of work - but rather interesting work if you like parsing. It can go in two completely different ways, so I asked for feedback three months ago. Yours is the first reply.

Our options are:

1. Support MsTest-format filters. If we do that, users will have to use one style of filter if working in the adapter and a different one when using the console or gui runners. We will have to write a parser in order to resolve the Explicit problem, probably basing it on our existing parser.

2. Support our own filters. If we do that, users will use one filter expression for all NUnit runners but a different one if they are working with MsTest. We will reuse our existing parser code. The Explicit problem will be resolved as a side effect. Existing users of MsTest-style filters will need to change to use NUnit Test Selection Language.

3. Support both kinds of filters. In this case there are sub-options.

   3.1 Leave the MsTest-style filters broken with respect to explicit. We would set up some way for the user to tell us which type of filter they were using, probably by requiring some special prefix to trigger our own.

   3.2 Fix the MsTest-style filters as well. This requires all the same work as both options 1 and 2. This is probably the "ideal" solution, but only if we know that folks want both options. That's why we ask for feedback!

Permit me to briefly rant...

I've been working on NUnit for about 14 years now. When asking for feedback, I have often been told "Just do what you think best - you're the one working on it." In many cases, I have taken this to heart because at least it gives a decision and we can move forward.

That approach doesn't work for everything, however. Sometimes I absolutely need user input, so I ask for it. This is such a case. Because we are talking about a feature that I personally never use, I can't have an intelligent opinion about it. All I can do is outline the options and the pros and cons that I see. Others have to respond to that if they want the thing to move ahead. If not enough people care strongly enough to give some feedback, then it doesn't move. There are other things in NUnit that I feel strongly about myself, so I focus on them.

End of rant...

Currently this issue is in the 3.5 milestone but with nobody assigned. That''s a bit of an anomaly. I put it there in the hopes of drawing attention to the fact that it is blocked. I should have marked it blocked, which I just did, for the reasons explained above.

[CharliePoole]

@ChrisMaddock Good on you for noticing that. We only have a one line problem description from @adamralph so let's wait for more information before replicating this anywhere else.

[adamralph]

I'll try and push a repro solution to GitHub some time tomorrow.

[rprouse]

@CharliePoole I had assumed that Run All was passing an MSTest style filter with the list of all tests to run and that is why the explicit tests are being run, but I haven't debugged it, so I might be mistaken. I am seeing the same behaviour in the .NET Core adapter.

[CharliePoole]

@rprouse If it's doing that, it's new behavior on VS's part and violates their documentation, which explicitly states that the kind of filter we use is up to us. I'll check in the debugger with VS 2015.

[dnperfors]

@CharliePoole, thanks for the explanation. I don't think we use the MsTest-style filters and even if we do, it would be no problem to change them to the NUnit-style filters. So I would opt for the quicker solution of at least start supporting the NUnit filters in 3.5 and, if necessary, include support for MsTest-style filters later.

[jnm2]

If you decide to unify it, and have explicit tests skipped in both scenarios, how would you like the user to opt in and run explicit tests?

When we get to the implementation phase, integration tests would be nice.

[OsirisTerje]

1. By default: All explicit tests are skipped, regardless how you run. This also makes it consistent with NUnit default behaviour. Also consistent with @CharliePoole's suggestion Test Adapter runs explicit tests when TFS TestCaseFilter is used nunit-vs-adapter#71 (comment) , see Fix 2.
2. Adding TestCategory = Explicit will run only Explicit tests.
3. Mixing in with other categories will allow them to run too.

Options:
4. TestCategory=Explicit can be shortened to just Explicit

Note that the only thing we don't do then is to allow an explicit test to be run because it is part of a test fixture marked with another selected category. I don't understand the logic of this though (my understanding of this might even be wrong).

5. We need to decide how to handle:
   a) [Category("Navigation"), Explicit]
   @CharliePoole says this is to be run, as per NUnit semantics. That both makes sense to me, but it doesn't when this is part of a hierarchy. However, I don't want to break NUnit semantics.
   And then we have the hierarchy parts of this, see further comments after the above one.
   I would also like to see the feasibility of this from @CharliePoole : "What I suggested we do is directly translate the VS-provided filter to an NUnit filter. That will automatically apply NUnit semantics." I don't see how we add the Explicit user choice, which has to be part of the filter.
   But we can do 1-4 first, and then 5 later.

[CharliePoole]

Looking back before moving ahead (in the next comment)...

@OsirisTerje You summarized my options as follows:

I might have misunderstood things here, but I try to rephrase it:
Option a) Replace with NUnit filtering syntax, and allow full NUnit filtering semantics
Option b) Stay with the VSTest filtering syntax, but extend it, and make it match the NUnit semantics as far as we can get it.

That's not quite it. In my comment back in April, 2016 🙄 I suggested four things...

1. Support the VS syntax by parsing it ourselves
2. Support our own syntax in the same way
3. Support both
4. Support a hybrid

By "support" I had no intention to suggest that vstest-console would accept our own syntax. I assumed we would have to provide it through the .runsettings file.

My own preference these days is 1 + 2. Command-line filters would have to use the VS format. Adding our own syntax as a runsetting would be an extra.

[CharliePoole]

@OsirisTerje Why do we need a command-line switch to run explicit tests? We don't have a way to do this in the console runner and the framework has no way to overrule explicit except by specifically selecting the test. Selection can be by name, by category, etc.

In any case, if you feel we need this new feature, it's orthogonal to the purpose of this issue, which is to stop running explicit tests when they are not selected.

[CharliePoole]

@OsirisTerje I'm not understanding the "extra" bug in the V2 adapter. You state that explicit tests are skipped when no filter is used. That's correct behavior. Is there some other bug?

[CharliePoole]

@OsirisTerje You wrote

Note that the only thing we don't do then is to allow an explicit test to be run because it is part of a test fixture marked with another selected category. I don't understand the logic of this though (my understanding of this might even be wrong).

I think you mean something like

[Category("X")]
public class Fixture()
{
   [Explicit, Test]
   public void TheTest() { }
}

When running category "X", TheTest will not be executed because it is not explicitly selected. Explicit selection means you have asked to run the test by name or by a category that appears on it. (Non-explicit selection means you have asked to run a test suite that contains the test.)

[jnm2]

I wish category selection was in the same genre as parent selection so that it would never explicitly select a test case. I can't imagine thinking of a category as a way to single out an explicit test. But that's determined by the framework.

[CharliePoole]

I think that one thing being missed in the discussion is the purpose of Explicit. It was designed for use on tests that one does not normally want to run as part of the CI build. In rare cases, one may want to override this behavior, which is possible through explicitly calling out the test to be run.

A situation where you want to run all your non-CI tests in the CI build seems pretty far-fetched. For example, I use [Explicit] myself...

1. When the test does UI and I am running headless in CI
2. When I want to actually see the result of a failure

I would never want such tests to run in CI.

[OsirisTerje]

@CharliePoole Thanks, the comments above really cleared it out for me. :-)

Then we have the default behavior cleared out, (stop running explicit tests) and we should get that fixed. (My pt.1. above). We should have this in with 3.10.

To allow people to run explicit tests, I think the parsing to NUnit filter and letting the engine do the rest, is the best way to go, since, if I understand the engine correctly, it already has that functionality. But, it takes some more time to implement that. I'll add a separate issue for implementing that post 3.10, for 3.11.
Until this is fixed, we keep the behavior in VS mode, that is that selected tests are run, regardless of how they are selected. Since that is a separate Runtests interface method, we know when this is being selected.

We can then just auto-add Explicit as a category, as was suggested above, which would also provide grouping in the UI as @rprouse points out, and it "unblocks us from blocking" when we implement the default behavior. And if someone really wants to run them in CI, they can. Even if I agree with @CharliePoole , can't really see why. But at least we don't block anyone. We do this in 3.10.

@CharliePoole The bug in NUnit2 adapter is that it runs explicit tests when any kind of test filter is added, even if it doesn't points to the explicit test. So based on our earlier chat about that, we should fix it there too.

[CharliePoole]

@OsirisTerje That clears things up for me. We should continue the chat we started offline as well. That will help me figure out where or if I can help you.

[CharliePoole]

@OsirisTerje Is there a schedule for 3.10?

[jnm2]

It's not impossible that people might use dotnet test --filter "X" (which gives /TestCaseFilter:"X" to VSTest.Console.exe) for non-CI purposes, so they might appreciate having a way to run explicit tests like Terje is planning.

[OsirisTerje]

@CharliePoole Yes, it is scheduled for release by the end of the month, that means, I hope to have it out by the end of next week.

[jnm2]

If I understood right, @OsirisTerje slated this for 3.10:

• Add an Explicit trait to test cases with [Explicit] (Adds Explicit as a trait #467)
  

  • This trait will be inherited from parent nodes the same way that traits created from categories do (in the translation to VS's leaf-only test cases)

• Skip all explicit tests when using a TestCaseFilter (Block explicit tests for 3.10 #471)
  

  • There is intended to be no way to run explicit tests while using TestCaseFilter in 3.10, to be addressed in the next release

And this for 3.next:

• Parsing TestCaseFilters to enable certain patterns to explicitly select test cases (Execute explicit tests using the TestCaseFilter argument on command line #473)

@OsirisTerje Any details missing for the 3.10 work?

[OsirisTerje]

Seems correct , thanks!

[jnm2]

Should we start a new issue for 3.11 to discuss ways to run explicit tests from the command line?

[OsirisTerje]

@jnm2 Please do

[jnm2]

Done! #473