Added a continue-on-unload-error option

[julienadam]

This is a cleaned up version of a fork I have been testing on our rather large build chain for some time. It logs errors in the internal trace and continues if an unload errors occur instead of throwing exceptions if the flag is set. I left it disabled by default so that people have to knowingly choose to ignore the unload errors.

It's not ideal as once activated it becomes hard to see that errors are happening at all, requiring the users to look into the agent log files which, admittedly, no one does unless there's a problem...

If anyone has suggestions for ways to make the errors more apparent, let me know.

Sorry for the whitespace diffs, I have a VS plugin that removes trailing whitespace when saving a file in VS and forgot to turn it off before porting the changes to the branch.

[ChrisMaddock]

Hey - thanks for sharing this solution. 🙂

I was hoping we'd be able to tackle this problem in a slightly different way - which would mean a) No new command line flag would be needed and b) the user would get a better experience by default, without needing a few failure scenarios first. What I haven't been able to do yet, is work out whether this idea is technically feasible or not! Could we leave this PR open, until we can try out the other method? Alternatively - you're very welcome to try implementing it yourself, if you're interested!

What I was hoping we could do, was something like this:

1. The engine should catch the CannotUnloadAppDomainException, and then rethrow it (wrapped as an NUnitEngineException) as late as possible.
2. As opposed to now, where the console terminates as soon as it gets the exception - it should instead catch this exception, and continue writing out the test results as normal.
3. The console should then write out warning and debug information from the unloaded AppDomain, based on the information inside the exception.
4. The console should then exit with a non-zero exit code.

This would mean, that in the event of app domain failure, the user still get's all their test results - but CI would still fail. It also means, in situations such as your own, where you know that appdomain unload is flakey - you could specify in your build chain that the new exit code should also be seen as a successful build, and therefore not fail your builds based on appdomain unload.

What do you think? I'm not sure when I'll have the chance to try and code this myself, but I'd like to get it in to the next console release. As above, you're welcome to give it a shot if you fancy. I'd prefer not to add any further console flags unless completely necessary - and my concern with this implementation, is that it doesn't improve the initial failure - there's no reason why we can't still write test results, once the app domain has failed to unload.

[CharliePoole]

@ChrisMaddock I'm not looking at the code right now, but I seem to recall adding code at one point so the console recognized that it has a result and therefore doesn't terminate until those results are displayed. Of course, even if I'm right, this particular exception may be getting past that check.

In any case, I like your idea.

[julienadam]

Hello,

This sounds like a much better solution indeed. I'll look into it if I get a chance, this end of year is going to be pretty busy!

[ChrisMaddock]

@julienadam - See #320 and #321. Your thoughts would be welcome. 😄

[ChrisMaddock]

@julienadam Based on #320 being merged - I'd like to close this PR now.

Version 3.8 will be released soon with a more complete fix. In the meantime, It would be great if you could try out the latest build of master, and report any issues you find - you can get this from our MyGet feed, which is linked in the README.

Thanks for sharing this fix regardless. 😄