Tests with custom `[Test]` attibute gives NUnit1027 when tests have arguments

[lucasteles]

NUnit have an analyser package to help on common mistakes

One of them is when you expect arguments in an [Test], it gives the NUnit1027 error. It predicts that only [TestCase] can have parameters.

So I'm using FsCheck which has our PropertyAttribute which inherits from TestAttribute. This attribute accuses error on the analyzer because it passes arguments to test (https://github.com/fscheck/FsCheck/issues/623)

Workarounds:

• Shutdown the NUnit1027 check

It would be good to have a way to make the analyzer ignore some kinds of custom test attributes

[stevenaw]

Thanks for raising this @lucasteles , as well as for already getting the fscheck perspective by asking there too.

While this issue tracker is a good place for framework-related or general issues, in this case it sounds like we suspect any nunit-side fix could be in the analyzer and so the best place for a discussion and any potential fix would be here: https://github.com/nunit/nunit.analyzers/issues

I'm unable to move it over myself; @mikkelbu do you have the right permissions?

[manfred-brands]

The nunit.analyzer rule for NUnit1027 detects certain extensions: Parameter marked with IParametersDataSource such as the Values and Range attributes. Attributes implementing ITestBuilder

Looking at FsCheckProperty it does implement ISimpleTestBuilder. The NUnit.Analyzer rule does check for this, but it then expects the parameters to have attributes supplying values.

This is before my time, so I don't know if FsCheckProperty should implement ITestBuilder or the rule should be updated.

[mikkelbu]

@lucasteles I've transferred the issue to the nunit.analyzers repository.

Can you give an example of this problem - just for more context as I don't know much about FsCheck? Preferably a small project that I can experiment with.

I've only skimmed the FsCheckProperty code, but if it implements ISimpleTestBuilder, then it sounds odd that it also has parameters as ISimpleTestBuilder is "...used by attributes that know how to build a single, non-parameterized test from a MethodInfo..." to quote the documentation that @manfred-brands linked to above (emphasis mine). Also looking at the FsCheckPropertyAttribute source code it seems like it implements its own testrunner that don't follow the usual conventions in NUnit.

Perhaps if PropertyAttribute derived from ITestBuilder instead of ISimpleTestBuilder then I think the problem would go away (but then they should also derive from another attribute - although I'm not sure they need to do it). I need to experiment some with FsCheckProperty as I have never used it, but my initial feeling is that it would be odd to special case this in the analyzers given that they don't follow the conventions as far as I can tell - the also state

You can now attribute tests with PropertyAttribute (a subclass of NUnit's TestAttribute). Unlike NUnit tests, these methods can take arguments and should return a property.

But perhaps my experiments will make me change my mind.

EDIT: Also how to you execute the tests? I would think that NUnits own testrunners would be confused about the ISimpleTestBuilder vs method having parameters.

[kurtschelfthout]

Sorry to be late to the game here.

A property is somewhat subtly different from a parametrized test, although it does look very similar of course (i.e. as a function with a bunch of parameters.)

I believe the reason FsCheckProperty implements ISimpleTestBuilder and not ITestBuilder is that the number of test cases for a single property is open-ended. If a randomly generated test case fails, for example, then FsCheck starts a process knowns as "shrinking": the failing values are used as a seed and incrementally made smaller (i.e. collections are made shorter, integers smaller etc) until the test stops failing. This produces a minimal failing test case that is typically easier to debug.

In other words, from the ITestBuilder docs:

A custom attribute implementing this interface should examine the IMethodInfo and return as many TestMethod instances as it is able to construct, using the parameters available to it.

FsCheck cannot do this before actually running the test.

All that said, feel free to point us in the direction of a more appropriate NUnit extension point than ISimpleTestBuilder.

[kurtschelfthout]

Also how to you execute the tests? I would think that NUnits own testrunners would be confused about the ISimpleTestBuilder vs method having parameters.

Using the normal NUnit runner. It does not appear to cause any confusion.

[kurtschelfthout]

Finally, for your consideration: the xunit analyzer had the same problem with FsCheck's property, and fixed it by not triggering on attributes that are subtypes of the in-box xunit FactAttribute & friends:

https://github.com/xunit/xunit.analyzers/pull/158

Can I suggest a similar approach here?

[mikkelbu]

Sorry about overlooking the last comments - I guess I lost track of them in a busy period. I've taken a closer look at FsCheck, the NUnit ITestBuilder vs ISimpleTestBuilder, and also the xunit issue.

As I see there are at least two paths going forward:

• Restrict NUnit1027 to only work on Attributes from NUnit. This will solve this issue, but also remove the Analyzers from other derivations from the NUnit interfaces (I don't think that there is that many in open, but this is just a guess)
• Make NUnit1027 ignore the derived attributes that comes from FsCheck, but work on other derivations from the NUnit interfaces. This will again fix this issue, but then we might have other issues for other frameworks that derive from NUnit.

I can live with both approaches - @manfred-brands Do you have any opinions on this?

[OsirisTerje]

By default I would go for pt.1 above, only trigger on our own attributes.
We are only responsible for our own stuff.

If possible allow an option (using a rule for this perhaps) for also triggering on subtypes.

[manfred-brands]

@mikkelbu Even though FsCheck seems to use the interfaces contrary the original intensions, they have a working system.

As the system is more flexible, we can only test what we know and that is our Test attribute.

There is a small risk we will no longer pick up tests with other custom attributes, but we don't know if these add an implicit parameter to tests or not.

So I vote also for option 1, but also add a configuration option: dotnet_diagnostic.NUnit1027.additional_no_parameter_attributes

Should we then also adjust the checks for the TestCaseAttribute from ITestBuilder to only check the TestCase and TestCaseSource attributes? NUnit has 7 attributes deriving from ITestBuilder some I never heard of regarding a CombiningStrategy. As these deal with parameters supplied by Value, we don't need to check those for number of paramaters. Did I interpret this correct or did I miss something here?