Consider an analyzer for when non-NUnit category attributes are applies to tests

[czdietrich]

I just would like to add the suggestion that we should consider renaming NUnit.Framework.CategoryAttribute to NUnit.Framework.TestCategoryAttribute.

The main reason is that there is already System.ComponentModel.CategoryAttribute with a similar constructor and it can easily happen to accidentally use the wrong attribute there, which may lead to tests being ignored, since the expected test category is not applied. (This happened for us in the past)

I also think that it would better align with other common attributes like [Test], [TestCase], [TestFixture], etc.

To make the transition easier, we could consider making CategoryAttribute to inherit from TestCategoryAttribute and so both attributes would just do the same and so there would be no immediate breaking change and at one point we could just obsolete the original CategoryAttribute.

This is just an idea that could help to avoid running into unexpected issues by reducing ambiguity.

[OsirisTerje]

To make the transition easier, we could consider making CategoryAttribute to inherit from TestCategoryAttribute

Doing this is as you say non-breaking for the framework. It will however cause a change in the adapter, which is currently converting TestCategory to Category coming from the dotnet test/vstest. It also handles Categories using the current name, which then have to extended to also handle a new name.

The adapter then have to be changed before the framework, and it will be "breaking" in the sense that older adapters will not be able to run this updated framework.

[czdietrich]

I guess if we actually would rename Category to TestCategory at some point a breaking change is most probably unavoidable long term if we decide to not support both forever?

Regarding the breaking change in the adapter. To use the proposed new TestCategory you would need to update the NUnit framework version to access the new attribute. But in the same time we do not expect a consumer to also update the adapter version? If I understand this correctly this is the only time where it could 'break' for someone?

What would happen if we switch the inheritance around, so TestCategory would inherit from Category.
Is the adapter then able to also recognize TestCategory as Category without the need of a newer version?
Or do I have a misunderstanding of how the relationship between the adapter and the framework works?

[jnm2]

NUnit or NUnit Analyzers could also look for System.ComponentModel.CategoryAttribute, and provide a warning if it is found applied to a test.

[stevenaw]

I like @jnm2 's analyzer idea. It seems simplest and would allow us to handle a design-time issue at design time.

[jnm2]

It could even be expanded to warn on any CategoryAttribute that is applied that is in a different namespace than NUnit's, and maybe even be widened beyond CategoryAttribute alone.

[czdietrich]

And the analyzer would look for any CategoryAttribute on methods that have also a NUnit.Framework.TestAttribute or classes that contain at least one method that has a NUnit.Framework.TestAttribute?

Sounds like a good idea to me.

[jnm2]

Yes! Or however https://github.com/nunit/nunit.analyzers already determines that a method is a test method or that a class is a test fixture.

[stevenaw]

I've moved this to the analyzers repo since it sounds like an analyzer-based solution may be preferable

[CharliePoole]

@czdietrich There's nothing to prevent defining your own class NUnit.Framewok.TestCategory : NUnit.Framework.Category. You'll get an error due to ambiguity in any test class using System.ComponentModel, which you can then fix at compile time.

[czdietrich]

Sure, it is possible to inherit from NUnit.Framework.Category in a local project to avoid this issue, but this doesn't seem very preferable here.

The main motivation for this request is mainly to make NUnit more beginner friendly, simply by reducing the chance of doing avoidable mistakes I would say.
In my opinion this could be achieved either by adding a TestCategory attribute which is just more precise or by having an analyzer as suggested by others.

This being said, if you feel that both options are not really worth the effort, it would be completely fine for me, if we do not implement any of those.

[CharliePoole]

@czdietrich Sure, it was just a thought and not just aimed at you but at anyone reading this, since it has been moved to the analyzer repo.