nunit / nunit-console

NUnit Console runner and test engine
MIT License
212 stars 150 forks source link

How to skip non-test .NETStandard assemblies? #900

Open synek317 opened 3 years ago

synek317 commented 3 years ago

I have a solution with multiple projects. My current environment is Linux + Mono + nunit3-console. I used to use nunit3-console ... --skipnontestassemblies *.sln.

Now, I'm trying to make my project compile on both, mono and .net core. So I'm porting my libraries to use framework netstandard20 while keeping test assemblies to use net461.

Unfortunately, nunit3-console no longer skips my non-test assemblies because it discovers unsupported framework before it checks if this assembly has tests or not. What is even worse, I cannot try with <TargetFrameworks> instead of <TargetFramework> because of a bug in the project loader: https://github.com/nunit/vs-project-loader/issues/37

Is there any other way I can keep passing sln path to nunit3-console and skip unsupported assemblies?

ChrisMaddock commented 3 years ago

HI @synek317,

The VS Project Loader hasn't had a maintainer for the last few years, and hasn't really been updated for some of the later features of csproj files. @CharliePoole has recently become maintainer of this project and is currently considering it's future - I'm sure he'd be interested in any offers of help!

Can you clarify - do your netstandard20 assemblies contain the [NonTestAssemblyAttribute]? And you're saying that the engine errors before it checks for the attribute? That does sound like a bug - would appreciate a pull request to fix it, if you're interested? 🙂

CharliePoole commented 3 years ago

@ChrisMaddock Did you want to hang onto this one for a bit rather than transfer to vs-project-loader? That's OK with me, if so.

ChrisMaddock commented 3 years ago

I think this issue is in the correct place, as reported, this is an engine problem. There are a couple of other considerations for the extension beyond this perhaps however, here's what I think:

For the engine:

  1. The Engine should probably give precedence to NonTestAssemblyAttribute ahead of throwing for assemblies that target a platform it doesn't recognise. (As filed in this issue!)

For the Extension:

  1. https://github.com/nunit/vs-project-loader/issues/37, as @synek317 has already filed. That's dependent on whether the extension is updated to fully support the new csproj or not. (And a question for the maintainer, in my eyes. 😉)
  2. What, if anything, should the VS Project Loader extension do by default around EnginePackageSetting.SkipNonTestAssemblies? Here it's being added on the command line. But I'm almost tempted to say the VS Project loader should set that by default when the user is using a sln file? Purely on the basis that I think it's quite unusual to have a solution containing only test assemblies - far more normal would be both production code and test assemblies in the same sln. That change would also depend on #827, perhaps.
CharliePoole commented 3 years ago

Ah that makes sense! I believe that the package created by the VS Project Loader extension for a solution should set SkipNonTestAssemblies because not doing so generates errors. This needs to be done no matter how #827 ends up interpreting the setting - although we should discuss that in #827.

synek317 commented 3 years ago

Thanks for mentioning [NonTestAssemblyAttribute]. It might sound surprising to you, but I haven't known it and my experience was closer to what #827 describes. Tbh, I was pretty sure that nunit-console considers every assembly without the reference to NUnit package as a "non-test".

I will check how adding this attribute works in netstandard + net4x (+net5 maybe) mixed solution.

CharliePoole commented 3 years ago

There's a bit of a problem in that [NonTestAssembly] may have evolved away from it's original intent. However, since I wrote it, here's what that intent was:

  1. Having a reference to a known framework (e.g. nunit) normally makes a test a "test assembly".
  2. Using [NonTestAssembly] on such an assembly creates an exception to the first rule.

IOW, you should only need the attribute on an assembly, which is not a test assembly but has a reference to the framework. An example would be an assembly containing your own extensions to the framework.

Knowing the definition, however, is only useful in deciding whether it's your code or the engine's that needs to be fixed. :smile:

CharliePoole commented 2 years ago

@nunit/engine-team I hadn't really thought about this before, but maybe part of the problem here is that the definition of NonTestAssemblyAttribute is different from what is implied by the console runner option --skipnontestassemblies and the corresponding package setting SkipNonTestAssemblies.

Cases we may need to deal with...

  1. A normal test assembly, with tests
  2. A normal test assembly (i.e. with a reference to the framework) but without tests having been added.
  3. A utility module with a reference to the framework, which should never be treated as a test assembly at all.
  4. A non-test assembly in the sense that it neither has tests nor references the framework.
  5. A test assembly for some framework we don't know about.

The attribute is only useful for case 3, since you can't have the attribute without adding a reference to the framework. Without the attribute, cases 2 and 3 are indistinguishable.

But when a user uses the console option, I think they are really talking about case 4.

What do you think? How should we deal with this?

One possibility: always skip tests with the attribute and additionally skip tests without a reference to a framework if the option is used. But what do we do about case 2, where there's a reference but no tests. Is that common enough to be important?