nunit.analyzers icon indicating copy to clipboard operation
nunit.analyzers copied to clipboard

Detect treatment of CategoryAttribute and others as though they apply only within the current attribute list

Open jnm2 opened this issue 4 years ago • 6 comments

This has come up more than once, though I'm only able to find this one right now: https://github.com/nunit/nunit/issues/3531#issuecomment-624206302

There should be a warning that code like this is misleading. Attribute brackets in C# do not represent logical groupings.

[SomeAttribute(...), Category(...)]
[SomeAttribute(...), Category(...)]

Should we offer a fix to avoid the warning if this is really something you meant to do?

[SomeAttribute(...)]
[Category(...)]
[SomeAttribute(...)]
[Category(...)]

Besides CategoryAttribute, almost all our non-test-generating attributes could be misunderstood this way. ApartmentAttribute, RepeatAttribute, etc.

jnm2 avatar May 05 '20 17:05 jnm2

I'm pretty sure I've pointed this out to people many times... like a dozen a year.. but I haven't kept track. It's actually quite natural for a naive user to assume that including two attributes in the same bracket pair somehow associates them.When this first started showing up, round about 2009 or 2010, I discussed (somewhere lost) replacing the use of "modifying" attributes like this with properties on the attribute that identifies the test. Older NUnit versions had in fact moved away from propeties to "composable" attributes, but that turns out to confuse some people who imagine that NUnit somehow "sees" the source code.

We need some kind of warning, like what the compiler gives when you may be accidentally using reference equality. The criterion would be multiple attributes within the square brackets when at least one of them is a test-generating attribute and at least one of them modifies the test in some way. (UPDATED)

CharliePoole avatar May 05 '20 18:05 CharliePoole

I've also encountered several variants (both for tests and for fixtures) - like

[TestFixture("A", false), Order(0)]
[TestFixture("B", false), Order(1)]
[TestFixture("A", true), Order(2)]
[TestFixture("B", true), Order(3)]

gives Duplicate 'Order' attribute from nunit/nunit#3056 or why doesn't this run "A", false first

[TestFixture("A", false), Order(0)]
[TestFixture("B", false)]
[TestFixture("A", true)]
[TestFixture("B", true)]

from nunit/nunit#3055

mikkelbu avatar May 05 '20 18:05 mikkelbu

Note that StyleCop will flag this with SA1133: Do not combine attributes. It includes a code fix to separate them.

manfred-brands avatar Sep 05 '20 01:09 manfred-brands

The goal with this one would be to fill a gap that isn't currently being filled.

jnm2 avatar Sep 05 '20 21:09 jnm2

Ordering fixtures with multiple attributes would be a real ++ bring to NUnit. I know about how C# works with brackets and about class instance, but... I use the framework for business cases with logical sequences, multiple browsers, and it definitly break the way i want to use it. So, i m under limitations atm. I expect a fix in a next release. Something like that : FixtureA with 4 parameters, Order 1, executed 4 times in any order BEFORE FixtureB with 3 parameters, Order 2, executed 3 times in any order... etc... Well, to have the capabilitie to order even the param execution from her fixture would be nice, but harder to implement.

Overclock303 avatar Sep 25 '20 14:09 Overclock303

@Overclock303 It sounds like you're asking for a new feature in NUnit. https://github.com/nunit/nunit/issues would be the place to track that.

jnm2 avatar Sep 27 '20 15:09 jnm2