v4 proposal: Change behaviour of EngineSetting.SkipNonTestAssemblies

[ChrisMaddock]

Change proposed

EngineSetting.SkipNonTestAssemblies currently only skips assemblies marked with the NUnit 3 framework's [NonTestAssembly] attribute. For v4, I propose that we should remove the requirement for [NonTestAssembly].

The new behaviour of --skipnontestassemblies should be that any assemblies passed into the engine in which no valid tests can be found are passed over silently. Assemblies would be designated in the xml as "Skipped" rather than "Invalid". As current behaviour, the console exit code would be unaffected by skipped assemblies.

Reason for change in behaviour

• This change would make the feature better match what seems to be common user expectation
• It would allow new uses of existing engine functionality such as Should --Explore Error On "Invalid" Assemblies? #794
• It would be an improvement for use cases such as passing in a .NET Solution file, which may likely also contain non-test projects a user would not wish to add the [NonTestAssembly] to
• It better matches the goal of the NUnit Engine being framework-independent - it allows greater use of a feature which is only currently usable with the v3 engine.
• It opens up the possibility of using wildcards to discover test assemblies, as commonly used in runners like TeamCity and Azure Devops.

Possible failure cases caused by change

As far as I can think, this would be a safe change to make, there is no way existing test runs could silently being to fail/not run.

The risk (and motivation for the original implementation behaviour) is that we should have a belt/braces solution to skipping tests assemblies, as it could be too easy for a user to inadvertently skip test assemblies they were expecting to run. My personal opinion is that this is unnecessary, and a user changing the configuration of a test run which uses this flag should be doing sufficient checking to ensure that test assemblies are being ran as they expected.

[ChrisMaddock]

@nunit/engine-team - I wanted to create a standard-ish format for proposed v4 changes, partly so we can make sure any changes in behaviour (breaking or otherwise) are well considered and documented. My intention would be to keep the above post updated as the result of any discussion, and it could eventually for the basis of v4's breaking changes documentation. Thought's?

Other thought - having now written this up, I'm not sure it even particularly needs to wait for v4. But it was good to test the process. 🙂

[CharliePoole]

@ChrisMaddock Unless I'm mistaken, that's how SkipNonTestAssemblies was always supposed to work.

But I think we have to clarify what's meant by a "non-test assembly". I intended it to mean an assembly with no reference to a known framework. An assembly with a reference to nunit.framework is a test assembly, even if it currently contains no tests. That definition is needed because you don't know if an assembly has tests or not until you apply a filter to it at run time.

The attribute itself was intended to provide an exception to the definition. You add the attribute onto an assembly that references the framework but is even so not a test assembly... maybe it's some kind of test helper.

Anyway, that's how I remember originally doing it!

[ChrisMaddock]

I think we agree on how it should work, but not how it currently does work! 🙂 It was several months ago I wrote the above now, but I'm fairly sure it's accurate as to what the current functionality is.

[CharliePoole]

It could be that the functionality has changed somewhere along the way. However, if we do a rapid V4, then the fact that it's breaking doesn't matter. 😄