Category as filter not working in single agent flow in vstest task

[abhishkk]

Issue

While running NUnit tests from vstest task, we can run tests in:

1. Single agent i.e. non-distributed flow
2. Multiple agents i.e. distributed flow
3. Test run with TIA (test impact analysis) as true

For 2nd and 3rd scenario, there comes a requirement in which tests needs to be filtered out at discovery time. As discovery time filtering was not supported for adapters till now (its supported now), TestPlatform filters tests at discovery on its own for these scenarios.

While doing filtering at discovery time, TestPlatform has exact mapping of Filters to traits. i.e. For Category trait, Category filters is understood.

Thus behavior change in TestPlatform side filtering(understand Category filter for Category trait) and adapter side filtering(understand TestCategory filter for Category trait) forces users to use

1. TestCategory as filter for running test in single agent (as discovery time filtering is not done by TestPlatform here. Filtering is done by NUnit adapter at execution time)
2. Category as filter for running tests in multiple agents or when TIA is enabled. (as discovery time filtering is done by TestPlatform here)

Sample Issues

Few of the issues related to this are:

1. Run Functional Tests in TFS on Premise using NUnit3 Adapter microsoft/azure-pipelines-tasks#3206
2. Visual Studio Test Task fails on TFS 2017 RTM during build microsoft/azure-pipelines-tasks#3800
3. Visual Studio Test Task ignores Test Filter Criteria with Nunit microsoft/azure-pipelines-tasks#4251

[jnm2]

@abhishkk

During filtering at vstest/TestPlatform level, we map Traits and filters with exact names. So we understand Category filter rather than TestCategory filter (as trait name is Category in NUnit).

I would prefer that VSTest mapped the category attribute to TestCategory traits at the vstest/TestPlatform level. NUnit doesn't have traits; it has properties, and they are not translated 1:1 into traits for different reasons.

[abhishkk]

@jnm2 Sorry I have used traits instead of attribute. Will refactor that in all the comments.
In vstest side, we are now mapping both Category and TestCategory filter to Category attribute. So when filtering is done by vstest, everything works fine.
But when the filtering is done by NUnit adapter, it maps only TestCategory filter to Category attribute. PR for this issue contains changes for mapping Category filter also to Category attribute.

[jnm2]

@abhishkk Just so I know what we're talking about, where are you getting the string "Category"? Is this from your static analysis of the C# syntax, [Category("foo")] (also legal C#: [CategoryAttribute("foo")])?

If so, why not make a rule in VSTest that interprets CategoryAttribute as "TestCategory" and stay consistent with the past?

Down the road, I am hoping NUnit can take responsibility for the static analysis, used for example in realtime test discovery. (nunit/nunit#2733) NUnit has a lot of subtleties and is still changing. If we went this direction, we'd provide you an assembly where you pass in something like Roslyn syntax nodes and we spit out something like TestCase instances, set up properly with knowledge from both the NUnit Framework and NUnit VS adapter repositories.

[abhishkk]

@jnm2

Just so I know what we're talking about, where are you getting the string "Category"? Is this from your static analysis of the C# syntax, [Category("foo")] (also legal C#: [CategoryAttribute("foo")])?

Correct.

If so, why not make a rule in VSTest that interprets CategoryAttribute as "TestCategory" and stay consistent with the past?

We have already done this. Please check this comment for more details.
Problem remains is that, ideally name of filter should match with the attribute name to avoid confusion.

In in-built adapters of VS, TestCategory is the filter name because attribute name there is TestCategory. NUnit adapter today understands TestCategory as filter just because VS in-built adapters do so. This should not happen. NUnit filter name should be driven through their respective attribute name.

In vstest, any user expect filter to be passed as per attribute name. As vstest is used for various adapters, it is better to use Category as filter to avoid confusion.

[jnm2]

NUnit adapter today understands TestCategory as filter just because VS in-built adapters do so. This should not happen. NUnit filter name should be driven through their respective attribute name.

Thank you, that's a great explanation! This makes sense; rather than being about TIA per se, VSTest officially wants all adapters to support only their own names as traits under all circumstances.

If @OsirisTerje and @rprouse support this VSTest guidance, going ahead sounds awesome to me so long as we document that everyone should use Category and stop using TestCategory. (Both options would continue to work.)

[OsirisTerje]

@abhishkk Whether one want to use a common name or a specific name depends on your situation. Given that a system is having a mix of two frameworks, for whatever reason (legacy, moving from one to another), it makes sense to have a common naming too, so that the devops people don't need to worry about whether the underlying code uses one or the other framework. For situations where the opposite is true, a single framework is being used, it makes more sense to use that frameworks "native" names.

[jnm2]

Very good point. We should support TestCategory in case multiple frameworks are involved in the run, but we can still prefer Category in the docs while calling out the multi-framework fallback.

[abhishkk]

@jnm2 @OsirisTerje

Whether one want to use a common name or a specific name depends on your situation. Given that a system is having a mix of two frameworks, for whatever reason (legacy, moving from one to another), it makes sense to have a common naming too, so that the devops people don't need to worry about whether the underlying code uses one or the other framework.

Filters are adapter specific and not test platform specific. In case multiple adapters are used in a run, we expect user will pass filters for both. Example: /TestCaseFilter:Category=SampleValue|TestCategory=SampleValue

But as vstest and VS TestPlatform, we allow both:

1. Common naming. Example: TestCategory is working fine today for both NUnit and in-built adapters.
2. Not having same filter name as of attribute name. Again TestCategory is the example here. Ideally this should not be preferred to avoid confusion.

The reason changes are done from our side in NUnit is because we were doing filtering at our end in few scenarios like TIA (which we should avoid as filtering should be done by adapters and not by TestPlatform). Filtering was done at our side as we needed discovery time filtering which was not supported for adapters at that time.

Recently, I have added support for disocvery time filtering in TestPlatform as well. This will make filtering totally on adapters for all flows. We will send guidelines and raise issue in near future regarding the same.