nunit-console: Remove -5 exit code on app domain unload failures
Hi,
After updating from 3.7.0 to 3.8.0, NUnit gives an error exit code. Below is the error output from both 3.7.0 and 3.8.0. The way the NUnitEngineUnloadException is handled has changed, but I do believe it hasn’t been for the better… In 3.7.0 I didn’t get an error exit code, but in 3.8.0 I do.
In 3.7 I get this as engine errors:
Unhandled Exception: NUnit.Engine.NUnitEngineException: Exception encountered unloading AppDomain Agent Process was terminated successfully after error. at NUnit.Engine.Runners.ProcessRunner.Dispose(Boolean disposing) at NUnit.Engine.Runners.AbstractTestRunner.Dispose() at NUnit.Engine.Runners.TestExecutionTask.Execute() at NUnit.Engine.Runners.ParallelTaskWorkerPool.ProcessTasksProc() at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx) at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx) at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state) at System.Threading.ThreadHelper.ThreadStart()
powershell gives True as output for this process
In 3.8 I get the following:
NUnit.Engine.NUnitEngineUnloadException : Multiple exceptions encountered. Retrieve AggregatedExceptions property for more information ----> NUnit.Engine.NUnitEngineUnloadException : Agent Process was terminated successfully after error. ----> NUnit.Engine.NUnitEngineUnloadException : Exception encountered unloading application domain ----> NUnit.Engine.NUnitEngineException : Exception encountered unloading application domain: Error while unloading ap pdomain. (Exception from HRESULT: 0x80131015) Application domain name: domain-f7378c0-*.dll Application domain BaseDirectory: Path
powershell gives False as output for this process
About this issue
- Original URL
- State: closed
- Created 6 years ago
- Comments: 43 (22 by maintainers)
We can ignore the code but it’s just more build cruft for something that should be straightforward. Most people don’t give a toss about AppDomain unloading, AppDomains haven’t made it into .Net Core so going forwards they are going to be of less and less relevance. As noble as the sentiments are to make sure people identify related issues I don’t think it’s the job of the test framework and if it is going to do it then it should be opt in. I’ve wasted an afternoon on this, hundreds of others will too.
Charlie - sorry I haven’t replied directly to your points - but I think the above is relevant. I don’t want to design a way for agents to report unload failures to the engine, if there’s never an actual need for an agent to unload an app domain, and therefore get an unload failure. Without looking at the code here, I’m not sure of the edge cases around this.
I am in agreement with you (both) now though that the current solution isn’t what we want.
@ChrisMaddock true that it will select an appropriate error string but my point was that Cake will throw an exception and fail where it wouldn’t have previously, it provides no control over ignoring this code. If we add a switch to revert the behavior then libraries will need updating to be able to pass this option into the runner. I really think this should be reverted as soon as possible before more people upgrade and waste their time on it.
On further reflection this isn’t that straightforward, we use FAKE in our build scripts which abstracts the call to the console runner.
Now we either have to drop use of FAKE’s helper and hand roll the calls or patch FAKE to support this return value. Other teams use CAKE which will also need updating.
In fact any library (both public and private) that helps with running NUnit will need updating to reflect this change.
I’m on the other side. I think we should report the problem but not throw or give a negative exit code. If users don’t notice things that are reported but don’t cause the run to fail, then there are a lot of other things they will miss. We should trust users to read the fine print.
FWIW I’ve reached the conclusion that it makes no sense to say you use semver unless you also give a pretty precise definition of what you mean by a breaking change. We have not done that, I’m afraid, in part due to some disagreements in the team about what the definition should be.
Ah got it, thanks for the answer. I’ll read up on the discussion and see if I have anything to add. We also have an older product in which versioning is a problem, so I understand your concern very well.
Keep up the good work! Cheers