Parallelizable and NonParallelizable attributes on setup and teardown silently ignored

[CharliePoole]

NUnit has many attributes that only work when used on the right element.

In the case of Method vs Class vs Assembly, we rely on the compiler to tell us. However, in the case of methods particularly, there are a lot of different sorts of methods in a test fixture and the compiler can't help us. Usually, NUnit just ignores any attribute that is incorrectly placed: for example, if you put a Timeout attribute on a non-test method. Usually, this doesn't cause a problem. 😄

In the particular case of ParallelAttribute some users think it makes sense to accept them on the OneTimeSetUp method. They try it out and NUnit doesn't complain so they wonder why their tests are not running in parallel. I think we have to give some error in this case. I would extend that to NonParallelAttrubte and to OneTimeTearDown, SetUp and TearDown.

Eventually, we may want to generalize this by having all attributes derived from NUnitAttribute specify how they may be used and having NUnit check them extensively. I don't think we should do that in this issue, because we have an ongoing problem with ParallelAttribute that should be resolved quickly.

[jnm2]

Slightly related to this, it would be fantastic if NUnit shipped with an analyzer in the NuGet package which errored or warned on things like misplaced attributes. It doesn't have to start out comprehensive; it'll just be a place for us to add helpful diagnostics and sometimes fixes as we think of them.

[CharliePoole]

I think it would improve the clarity of this issue if you could spell out the pros and cons of the two approaches: static analyzer vs in-framework error checking. Also, how would they relate? That is, would the errors in one case be a proper subset of those in the other? Are there some things you can only catch one way or the other?

[jnm2]

I think we need both approaches.

Pros of static analysis:

• Best possible feedback loop, comes with description as you type and sometimes fix
• Finds things which would be inefficient for the framework to find at runtime (see example 1)
• Can provide ignorable suggestions on best practices if we think of any, such as telling people when attributes are redundant ([Nunit 3.7.0] Issues with Parallelizable attribute on methods #2212 (comment))
• If shipped in the framework NuGet package, will always be analyzing at build time including your CI as well as analyzing as you type.

Pros of runtime failure:

• I was really going to list something here, but I'm drawing a blank. :-/ I can't think of anything that runtime can catch that static analysis can't, or a way that runtime could provide feedback better than an analyzer.

Example 1

This use of Parallelizable wouldn't be noticed by the framework at runtime but it would be noticed by the static analyzer:

class MyFixture
{
    [Parallelizable]
    public void MyTestButIForgotTheTestAttribute()
    {
        // ...
    }
}

[CharliePoole]

@jnm2

Pros of error checking in the framework...

• It works even if the user hasn't done any static checking
• It works in all environments, including non VS and non-Windows
• Errors display in the test report and are included in the XML
• It is consistent the idea that we try to use whatever the user claims is a test, giving an Invalid status to the test if it isn't.

Re your example... We don't check this but that doesn't mean you can't check it.

Re bundling into the framework... That would mean having a special nuget package that users can install if they want it. I don't think we should force it on them.

[CharliePoole]

Relevant to this, I want to point out the great mission statement that Rob came up with, which is now in our vision document. He wrote that we are unopinionated i.e. we let users do things the way they prefer.

So another bullet to place under both static analysis and runtime checking is

• Some users may prefer to do it this way!

@jnm2 I didn't mean this as a trick question, but I was hoping to lead into a discussion of what users may want as opposed to what we think they should want.

[jnm2]

@CharliePoole Thanks.

It works in all environments, including non VS and non-Windows

This is also a pro of a Roslyn analyzer. There is nothing VS- or Windows-specific about them.

Re your example... We don't check this but that doesn't mean you can't check it.

The con for runtime here is that looking at every method for out-of-place attributes is slow by comparison and really IMO doesn't belong in test run errors. Runtime errors should be limited to incidental errors which actually prevent a test from being run.

Relevant to this, I want to point out the great mission statement that Rob came up with, which is now in our vision document. He wrote that we are unopinionated i.e. we let users do things the way they prefer.

Yes! Totally agree.
We should only make things errors that would actually invalidate the test at runtime, and warnings for things that are misleading like out-of-place attributes, and suggestions for anything else. End users can easily change the default severity of each diagnostic to their liking, including turning it off.

Re bundling into the framework... That would mean having a special nuget package that users can install if they want it. I don't think we should force it on them.

Given that users will 100% be in control of what they want to see and that the analysis will err on the side of being unopinionated, I am disappointed that we would not ship it in the framework package. The experience would be so much better. I am guessing that if we shipped it as a separate package, most people would never know it existed or feel like trying it out to see if it's worth adding a new dependency across all their projects.

It's a question of a) leaving most people in the dark permanently versus b) potentially having to change our minds and publish as two packages if we discover a reason that that would be desirable.

And even if we do change our minds due to unforeseen issues, that's not a bad outcome. It's no worse than any other release where one or two people had to stay on the previous version until a hotfix was available.

If anything, I think people will start showing up here with positive feedback if we bundle the analyzer with the framework.

[CharliePoole]

If you're talking of bundling and packaging, that's a different discussion. Initially, it sounded like you wanted to include it in the framework assembly. If you do that, you take away choice. If you use nuget packaging properly, with dependencies as well as the ability for users to install things separately, then you are giving more choice.

I do think it will be a while before we are ready to bundle these things, however, since one project is experimental and the other is in production.

At any rate, that's all good future stuff. However, it's kind of hijacking this issue, which is about solving a specific problem. Hopefully someone will take it on and do a PR.

[jnm2]

As far as I know you can't mix analyzer assemblies with project reference assemblies. But I was picturing the two side by side in the main nunit.framework package so that no package exists with only the framework. That doesn't take away choice because the user can still turn off the analyzer and just use the framework assembly.

[CharliePoole]

Please, if you want to keep discussing this, can you create an issue to do it? This is a report of a problem that I would like to see fixed now, not at some future time. I don't think anyone is going to take it and do a PR while we are involved in all this debate.

I assume such an issue would be part of the analyzers project.