NUnit3 Fails on DLL with no Tests, Unlike NUnit2

rprouse commented 8 years ago

@hypersw commented on Fri Jun 17 2016

Some projects of our codebase are for test purposes only, though not all of them have test cases (some got only mocking infrastructure code).

With NUnit1-NUnit2, we used to submit all of the DLLs from the test area, and NUnit would run all the tests it could find.

With NUnit3, it fails with an error like this:

Errors and Failures
1) Invalid : xxx.dll
No suitable tests found in 'xxx.dll'.
Either assembly contains no tests or proper test driver has not been found.

And returns the ERRORLEVEL of FFFFFFFE.

This is a breaking change from NUnit2, from how I see it. We didn't think up a workaround yet. Is it possible to get the previous version behavior, with an option or something?

@hypersw commented on Fri Jun 17 2016

Errorlevel looks like NUnit.ConsoleRunner.ConsoleRunner::INVALID_ASSEMBLY.

@hypersw commented on Fri Jun 17 2016

The error is probably triggered in NUnit.Engine.Services.DriverService::GetDriver.

@hypersw commented on Fri Jun 17 2016

Looks like it actually checks by referenced assemblies only rather than presence of the actual test cases. That's something I could also check in the build script before spawning tests for each DLL. Might be useful to be able to batch-submit them though, as you'd not always have a build script capable of reading thru assembly references like in our case.

@hypersw commented on Fri Jun 17 2016

I've prototyped a fix at https://github.com/JetBrains/nunit/commit/ad29539dca76c87dd11e2491cf8537f01a9e38e3 for us to try nunit3 full-scale without missing test errors. Might not be a good idea doing this by default (even though this seems to be a breaking change from nunit2), but could prove useful if under an option.

@CharliePoole commented on Fri Jun 17 2016

Well, it's 3.x versus 2.x, so it has a ton of breaking changes. Minimum "fix" for this issue would be to make sure the change is listed on the Breaking Changes page. :-)

That said, I think this is a correct change. In most usage, passing a non-test assembly to NUnit is an error of some kind. It could be a simple, ignorable error as in your case. OTOH, it could be that you are using some third-party framework without installing the driver extension for that framework. There is no way for NUnit to know what is the cause.

This has already given us a lot of trouble with the VS adapter because VS passes us all the assemblies in a solution and it's also a problem for users who try to run the solution from the command line. We deal with the first by special checks in the adapter. The second is the subject of a separate issue (#668), which we will probably take care of by passing in some info about the source.

So, for various reasons, a package setting of "IgnoreNonTestAssemblies" is probably in the future. Once that's available, we could add a console option to activate it.

Note: This should be worked on in conjunction with #668.

@DanHarman commented on Tue Sep 27 2016

Is there any further thinking on this? My particular scenario is passing in a glob to my teamcity nunit build step for:

**\bin\Release\*Tests.dll

As this means we don't need to keep adding test projects into the build server manually (I've already failed to do this once...) However we also have mstest projects in the build which also have *Tests.dll extension, and nunit console test runner errors out as soon as it encounters one. Ideally I'd like a command line param to not error out if it is passed a dll without any tests it understands.

@CharliePoole commented on Tue Sep 27 2016

Well, as a result of earlier discussion, we accepted this as a feature request and added it to our backlog.

The next thing you can watch for is for somebody to take it on as an assignment. That could be a team member or an outside contributor who wants it badly. :smile:

@rprouse commented on Tue Sep 27 2016

If we decide to change this, we should do the same in the .NET Core runner. There is a duplicate issue there with a couple of requests, nunit/dotnet-test-nunit#77

@rprouse commented on Tue Sep 27 2016

This is also a Console issue, I am going to move this issue there.

snowfenix commented 7 years ago

Hello @rprouse

Do you have a sense on when this feature would be available ? This would be very helpful for us.

Thanks

Princy

CharliePoole commented 7 years ago

@princyraza I see this was never actually added to our backlog, which means nobody is giving it any special attention. Probably has to do with the issue having been moved from the framework to the console repository. Now that I've added it, it's a matter of somebody picking up on the issue. We generally only "push" priority high and above issues into planned releases. Normal and low priority get to releases after somebody has been inspired to work on them.

To help this along you could provide a bit more information. In particular, how are you running tests? Is it through nunit3-console? Do you specify a VS project on the command-line? If that's it, you have at least a temporary workaround of only specifying the assemblies with tests.

snowfenix commented 7 years ago

We are running tests using Teamcity through the nunit3-console. We specify the dlls via wildcards like this one: src/Client/Test/bin/%system.Build_Configuration%/*.Test.dll

If I am correct Teamcity creates a .nuit file with list of all the dlls. However, some of them may not have any tests. Our workaround for now is to run tests using NUnit 2.x. I've tried to switched some of our projects to NUnit 3.x but I always run into" No suitable tests found in xxx.dll. Ideally, the dll we pass in should all have tests but it's not always the case.

CharliePoole commented 7 years ago

@princyraza Here's a thought: can you change your naming convention so that only test assemblies match *.Test.dll ?

snowfenix commented 7 years ago

@CharliePoole We are in the process of doing just that. However, I was wondering there was a way for us to still use NUnit 3 in the meantime.

CharliePoole commented 7 years ago

Sorry, I'm afraid it requres an internal change - you could, of course build from source and make the change yourself... then you could contribute it to us!!!

ig-sinicyn commented 7 years ago

Sorry for upping:)

Any updates on the issue? If there's attribute for excluding assembly from the run it will be ok for us.

Real-world use-case: we have a Benchmark.Net-based perftest runner with various testing framework support. We do expect our customers to reference our package, write tests in, push the code, setup appveyor CI and everything should just work.

While the integration with NUnit was the easiest part of the work, there's one drawback we're trying to solve right now.

By default appveyor runs the tests for all assemblies that reference NUnit, commandline looks like this:

nunit3-console "%dir%\UserCode.dll" "%dir%\OurHelperAssembly.dll"

and if there is no NUnit tests in our helper assembly, the test run fails with

Invalid : %dir%\OurHelperAssembly.dll
Has no TestFixtures

message.

As a temp workaround we had to add stub test into OurHelperAssembly. The test completes now, but the stub test is shown on the test results tab. This looks like a ugly hack and we want to find some solution for it.

Note that we have no control over our customer's code and we do not want them to exclude our assembly manually.

Any suggestions are welcome:)

CharliePoole commented 7 years ago

The issue is accepted. Next thing you can expect to see is a team member or contributor signing up to do it. That could even be you! :-)

For normal and low priory issues, we use a pull system of assignment rather than push.

ig-sinicyn commented 7 years ago

Ok, thanks!

JPAhnen commented 7 years ago

Would adding a command-line argument (e.g. "ignore-invalid-assemblies") be ok? I would implement it since we could use this feature too.

ig-sinicyn commented 7 years ago

@HexWrench I'm afraid it will not.

There should be a way to mark an assembly as "Ignore me" and the "Ignore me" attribute should be checked by name like Resharper does with JetBrains.Annotations.

Here's why: as I've said above we had to add a stub test to not break appveyor builds for each project that references our assembly. We cannot remove the test as it'll break builds for customers that did not update nUnit yet. At the same time we want the stub test to disappear entirely after upgrading to NUnit vNext.

The proposed approach is following: we define custom NUnit.Framework.NotATestAssemblyAttribute (provisory name) and apply it to our assembly. Some future version of NUnit will recognize the attribute and will ignore our assembly entirely.

CharliePoole commented 7 years ago

@ig-sinicyn For helper assemblies that reference NUnit, we could look for that attribute in the engine and ensure that it would (a) not be loaded and (b) not return any kind of error state. Would we want this for any other use cases? I ask because it might need to go into different parts of the code depending on whether or not it only applies to assemblies that initially appear as NUnit tests or if it should apply to any assemblies at all, including foreign and custom frameworks.

ig-sinicyn commented 7 years ago

@CharliePoole I have no additional use cases but it's only me :)

CharliePoole commented 7 years ago

Well @princyraza needs it because of using a wildcard expression to locate assemblies. What about you @HexWrench ?

JPAhnen commented 7 years ago

@CharliePoole Unfortunately, we have no separate test assemblies. We use Teamcity for building our software and with NUnit 2 we just ran the tests for all dlls. Since not all assemblies contain tests, it fails with NUnit 3. My suggestion would help people with similar issues migrate to NUnit 3.

CharliePoole commented 7 years ago

I'm assigning this to myself temporarily, for a little design work. This is one of those issues that not only impacts what we are currently doing but also where we might be going in the future. I'd like it to work if somebody, for example, decides to write a driver for xunit.net tests. Once I look at the code, I'll post something here and probably back out so someone else can implement it.

CharliePoole commented 7 years ago

Updated December 7, 2016

OK, here's the proposed design. @nunit/core-team @nunit/framework-team Please comment.

We'll define an attribute NUnit.Framework.NonTestAssemblyAttribute. Folks who create extensions that reference the nunit framework, but which are not supposed to be loaded as tests, can use this so that their users don't have to worry about the problem.
Modify RuntimeFrameworkService.ApplyImageData to notice this attribute and add setting to the test package - something like property IsNonTestAssembly = true.
We'll define a command-line option for the console runner --skipNonTestAssemblies (or some other name) that allows users to indicate that there may be some extraneous assemblies on the command line. The runner will use this to set SkipNonTestAssemblies to true in the package.
When getting assemblies from a Visual Studio solution, we should set SkipNonTestAssemblies to true for that package.
When getting runners for packages, we'll skip any that have the setting NonTestAssembly set to true.
When getting drivers for packages, if SkipNonTestAssembly is set we will not return an error should a driver not be found.

Questions? Comments? Does this make sense?

hypersw commented 7 years ago

This does not feel very good, because of the knowledge of NUnit leaking from test assemblies into non-test assemblies, even if that's not by a DLL reference but just by declaring an NUnit-aware attribute in some non-tests-aware code.

I'm not sure it's worth the problem we are solving — trying to avoid letting a switch which would make NUnit go lightly on assemblies which do not have tests in them.

That it asserts by default is OK, but many people would happily turn that off and have little trouble with that.

CharliePoole commented 7 years ago

@hypersw I see your point. Maybe the problem is that we are mixing up several different situations.

Third-party test frameoworks that reference NUnit. They are falsely identified as nunit tests because of that reference and then no tests are found. The proposed solution works well for them because they are already nunit-aware. My impression was that this was your case.
Tests written for some framework for which no driver is installed - nunit v2 for example. NUnit never finds a suitable driver and so it cannot proceed.
Test assemblies to which you have not yet added any tests.
Assemblies that can't be loaded at all.
Some random valid assembly, that is only being looked at because of wild-card use or because it is part of a VS solution.

Which of those cases are you concerned about? NUnit can't distinguish many of them except so if we use a command-line option it will be up to the user to take responsibility.

Using a command-line option, the design is the same except it's triggered by the option rather than by finding an attribute.

DanHarman commented 7 years ago

My requirement is simply that on a build server I can use a glob to select test assemblies, but that it doesn't assert if it can't find tests in them. This solves two issues:

can use wild cards to find assemblies so don't have to manually maintain the list of test modules on multiple projects. This is much more likely to not miss on the build server.
cope when there are occurrences of mstest projects in the same solution also being picked up by the glob. Agree with the already raised concerns about poluting things with an attribute for nunit when they want nothing to do with the framework! Rewinding a bit, why was a decision made to change the test runner behaviour with v3, and should that be re-examined? Dan

On Wed, Dec 7, 2016 at 12:38 AM +0000, "CharliePoole" notifications@github.com wrote:

@hypersw I see your point. Maybe the problem is that we are mixing up several different situations.

Third-party test frameoworks that reference NUnit. They are falsely identified as nunit tests because of that reference and then no tests are found. The proposed solution works well for them because they are already nunit-aware. My impression was that this was your case.

Tests written for some framework for which no driver is installed - nunit v2 for example. NUnit never finds a suitable driver and so it cannot proceed.

Test assemblies to which you have not yet added any tests.

Assemblies that can't be loaded at all.

Some random valid assembly, that is only being looked at because of wild-card use or because it is part of a VS solution.

Which of those cases are you concerned about? NUnit can't distinguish many of them except so if we use a command-line option it will be up to the user to take responsibility.

Using a command-line option, the design is the same except it's triggered by the option rather than by finding an attribute.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

JPAhnen commented 7 years ago

In our case a simple "ignore all assemblies containing no tests" command line argument would be the simplest approach. We have don't have separate test assemblies, so any assembly could contain tests. And even if it currently contains no tests, it could tomorrow. Therefore, with NUnit 2, we tested all assemblies.

Having a special attribute would mean that the person adding tests for the first time has to remove this attribute. And a person removing tests (e.g. after refactoring) would have to check if there are still tests in this assembly.

Having the command line argument would mean he doesn't have to worry about that. We simply would add the command line argument in our build server and done.

ChrisMaddock commented 7 years ago

I'm also not a fan of requiring an NUnit-specific attribute in non-nunit assemblies. I agree there's a number of different problems, and I wonder if implementing two options would be best - for different use cases.

A NonTestAssemblyAttribute - for use primarily by libraries which reference the framework but contain no tests, e.g. @ig-sinicyn's library. This would allow library authors to define the 'no-tests' property at their end, without their end users needing to care. The attribute could of course also be used by any users who wished to mark all non-NUnit-test assemblies individually, rather than relying on a global ignore all.
A --ignoreNonTestAssembly console runner flag. (With a better name!) This allows the user running the tests to also have control over this behaviour, and is a preferable option for e.g. TC glob's or VS solution running, where the user can declare that they are expecting non-test assemblies in this configuration.

CharliePoole commented 7 years ago

@DanHarman The "decision" was actually the necessary consequence of the new architecture.

In NUnit V2, we loaded every assembly and searched it for tests by examining attributes. The attributes were recognized by name, so if you had an "NUnit.Framework.TestAttribute" that was a test. If there were no tests, no problem, just move on.

In NUnit 3, the engine does not know in advance if it should expect to find NUnit 3 tests, NUnit V2 tests, xUnit.net tests, MS tests, etc. Any of them are possible if a driver is installed. NUnit 3 is built in, NUnit V2 is an available extension, the other examples are hypothetical at the moment.

Because of this, the engine has to examine each assembly and see if it references a known framework. If it doesn't reference a known framework, there are two possibilities: either it references an unknown framework or it doesn't reference any framework at all. That's the reason for the oddly worded message that says there may be no tests or no driver was found.

If the assembly references the nunit 3 framework (or the V2 framework) and there are no tests, we can easily handle that situation. From the reference, we know it's a test assembly. If no tests are found we can handle it just as we do for V2. I'd have to test, but I thought we were already doing that.

If the assembly doesn't reference a known framework, then we are back to the original problem of not knowing what the user intended.

CharliePoole commented 7 years ago

@DanHarman @HexWrench

We could add such a command-line option, understanding that it won't affect any files that actually throw an exception when we try to examine them. You would have to make sure not to include any such files in the directory. IOW, we can suppress the errors that NUnit actually originates but I don't think we should suppress errors that it encounters. Does that make sense?

CharliePoole commented 7 years ago

@ChrisMaddock That's what I'm getting out of this as well. As a refinement, we could do it automatically when expanding a VS Solution. I think we already have an issue for that.

I'll go back to my "design" comment and update it.

DanHarman commented 7 years ago

Ok I see how that came about, and sorry if I sounded critical - trying to contribute whilst typing on a phone between meetings can make me seem a little brusque! I'm probably missing something, but I'm not entirely clear why its a fail if you don't see a recognised framework in a module though. Surely that scenario is a 1 time type deal which would occur during explicit setup of build/test, and a non-failure advisory message would work in that scenario? Dan

    _____________________________

From: CharliePoole notifications@github.com Sent: Wednesday, December 7, 2016 2:44 pm Subject: Re: [nunit/nunit-console] NUnit3 Fails on DLL with no Tests, Unlike NUnit2 (#62) To: nunit/nunit-console nunit-console@noreply.github.com Cc: Mention mention@noreply.github.com, Dan Harman daniel.a.harman@gmail.com

@DanHarman The "decision" was actually the necessary consequence of the new architecture.

In NUnit V2, we loaded every assembly and searched it for tests by examining attributes. The attributes were recognized by name, so if you had an "NUnit.Framework.TestAttribute" that was a test. If there were no tests, no problem, just move on.

In NUnit 3, the engine does not know in advance if it should expect to find NUnit 3 tests, NUnit V2 tests, xUnit.net tests, MS tests, etc. Any of them are possible if a driver is installed. NUnit 3 is built in, NUnit V2 is an available extension, the other examples are hypothetical at the moment.

Because of this, the engine has to examine each assembly and see if it references a known framework. If it doesn't reference a known framework, there are two possibilities: either it references an unknown framework or it doesn't reference any framework at all. That's the reason for the oddly worded message that says there may be no tests or no driver was found.

If the assembly references the nunit 3 framework (or the V2 framework) and there are no tests, we can easily handle that situation. From the reference, we know it's a test assembly. If no tests are found we can handle it just as we do for V2. I'd have to test, but I thought we were already doing that.

If the assembly doesn't reference a known framework, then we are back to the original problem of not knowing what the user intended.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

JPAhnen commented 7 years ago

@CharliePoole In our case we test only "testable" assemblies (i.e. .NET assemblies), because we we test only assemblies with a certain prefix ("ourprefix.*.dll"). But as stated earlier, not all of them contain tests. So, for us this would be absolutely sufficient.

DanHarman commented 7 years ago

That makes sense, and whether to treat it as an error is clearly an architectural decision you are best placed to make. As long as the end result is that I can run my lovely nunit tests with a glob I'm very happy!

Dan Harman

On Wed, Dec 7, 2016 at 2:47 PM +0000, "CharliePoole" notifications@github.com wrote:

@DanHarman @HexWrench

We could add such a command-line option, understanding that it won't affect any files that actually throw an exception when we try to examine them. You would have to make sure not to include any such files in the directory. IOW, we can suppress the errors that NUnit actually originates but I don't think we should suppress errors that it encounters. Does that make sense?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

CharliePoole commented 7 years ago

I updated the original "design" comment above twice. It's now somewhat more complex, reflecting the complexity of the problem I think.

@DanHarman We originally threw an exception and stopped execution when no driver could be found. That was based on the expectation that somebody simply had typed the name of the assembly to load and would not want the run to proceed if there was an error. Globs weren't on my mind. 😄

Failing the test for that assembly was a step back from an error. Internally, we simply assign a driver to the assembly that always fails. In implementing this fix, I'll probably create a driver that just skips the assembly, with reason "No tests found". That seems better than simply dropping it without mention.

CharliePoole commented 7 years ago

@ChrisMaddock The meta-package "nunit" points to the install package, which would probably account for the downloads. Note that a portable package can also "install" things. The difference is that an install package relies on the existing product installer while the portable package does some installing itself (shims for example) and can be enhanced by powershell scripts. It doesn't have to just wrap a zip file as is now done by the automated process.

All that said, we might have to agree to disagree. I'd prefer to drop msis entirely, while you are the guardian of our msis. :smile:

borysl commented 7 years ago

I have the same issue and I have investigated it. I have the same version of NUnit.Console and NUnit, the same code base, but different CLR: 1) 4.0.30319.34209 - my local dev env. 2) 4.0.30319.18063 - the build server

Test DLL build in the second environment for some reason failed to be run on console. Even if I take the dll and put it to the 1 already compiled. There are however dozens of assemblies that doesn't behave in the same way and works on both build server and my local dev. Ildasm shows little difference between them. If it would help I can sent privately the assemblies.

rprouse commented 7 years ago

@borysl we will need to investigate further, but at first look, your issue sounds different than the issue reported here. I am going to leave it here for now, but we may end up splitting it into a separate issue.

Just to track the differences in frameworks,

4.0.30319.34209 is .NET 4.5.2
4.0.30319.18063 is .NET 4.5 for Win 7 with MS14-009 security update

I assume that since you are compiling everything on the build server, all of your assemblies target .NET 4.5 or lower?

Can you show us the output from the console for the failing test assembly?

One thing to note is that AFAIK, we are not handling compatibility mode for anything newer than .NET 4.5, so there may be some compatibility differences that you are hitting between 4.5 and 4.5.2. That usually leads to tests failing on one platform and passing on the other though, so it may not be it.

hypersw commented 7 years ago

@CharliePoole: the new design doc now seemingly solves all problems, thanks.

Though—on the standpoint of correctness—the doc would become complete if it also includes the reasoning behind flagging the error itself, in the first place (this would also explain why have so many bullet points instead of just one for removing the error).

I think this thread is mainly populated by people who are OK without the error, so the reasoning might be useful. The error appeared because of the switch from direct search for attributes to the querying of the drivers, but it does not logically follow why it should be considered an error when no driver claims any tests. I guess it's about foolproofing leaked configurations, like when you've mixed up your nugets on the build server and suddenly lost some tests due to missing drivers, or whatever.

For reference, my case is as follows: — Why assemblies with zero tests are getting into NUnit: —— Got assemblies with base classes for tests. They reference NUnit and have base classes (maybe fixtures even), but no test cases yet. —— It's possible for non-NUnit-aware assemblies to get into the same package with test assemblies, if they're implementing some scaffolding for the testing, like external process executables etc. They're getting picked by the same filelist. — How we mitigate the problem right now: —— The build script which runs NUnit is in C# and it duplicates the error checks from NUnit to filter down the assemblies list. — Why not afraid to go without the error: —— Got a build failure metric for the total number of tests on the build server (<extension type="BuildFailureOnMetric"/> with TC). This also covers other reasons for losing the tests.

CharliePoole commented 7 years ago

@hypersw As a bit of background, it's important to know that NUnit has had a lot of experience with "suppressed" or "ignored" errors. As an example, if you put a TestFixture attribute on a private method, there was a time many years ago when we simply ignored that method. Over time, we realized that it's better to highlight even the errors, so in that case we started detecting "Invalid tests" in cases like that and showing the errors.

This situation is a bit different, but the mindset that errors must be highlighted is still operating.

The logic for this error is that you, the user, have told NUnit that it can find tests in a particular assembly. If NUnit doesn't find any, just failing silently doesn't serve you. NUnit ought to say... "Wait, you said there were tests in here but I don't find them." So, you are right that this is a kind of foolproofing that we are trying to do.

I think we can break out many of the cases more carefully and handle them better.

If there is a reference to NUnit but no test cases are found, a simple warning is probably enough.
If NUnit knows that there may be non-test projects included, we can just skip them. A good example is when NUnit itself is finding the list of assemblies by examining a VS solution file. Another is when the VS Test Window gives NUnit all the assemblies in a certain solution for test discovery.
If you tell NUnit (via a setting) that there may be non-test assemblies in a list of assemblies provided, then again there is less risk in ignoring the error.

hypersw commented 7 years ago

That's definitely a good direction. With available fallbacks to the legacy behavior (ideally, hints on them included in the error message) it would be a 100% improvement.

janssenMA commented 7 years ago

Great to see this feature will be implemented in the new version. We need it for a proper TeamCity integration. Any idea where I can find the roadmap when version 3.6 will be released to NuGet?

rprouse commented 7 years ago

We hope to release 3.6 late next week

snowfenix commented 7 years ago

@rprouse Do you know when NUnit Console 3.6 will be released ?

rprouse commented 7 years ago

@princyraza it will be released sometime between today and Monday 😄

DanHarman commented 7 years ago

Thanks for addressing this. Will make life a lot easier :)

nunit / nunit-console

NUnit3 Fails on DLL with no Tests, Unlike NUnit2 #62