AggregatingTestRunner to handle app domain unload failure in same fashion as single assembly runner #321

ChrisMaddock · 2017-12-02T19:08:04Z

Fixes #191. This still needs tests, but sounds like it might be worth discussion before I go much further.

To clarify current behaviour:

When running a single assembly, if the AppDomain fails to unload as ProcessRunner is disposed, the exception is caught - and the console runner continues to write the already completed TestResults. A warning is then issued, and the console exits with 0. (To be changed in Return error code -5 when AppDomain fails to unload #320)
When running multiple assemblies via AggregatingTestRunner, if EnginePackageSettings.DisposeRunners = false, the same behaviour is maintained.
If EnginePackageSettings.DisposeRunners=true however (default for the console), then the test runners are disposed at the end of each test run, and there is no functionality to handle CannotUnloadAppDomain exceptions, as there is at the other points where TestRunners are disposed.

This is less of a problem when running via AggregatingTestRunner in parallel, as the CannotUnloadAppDomain exception is thrown on a separate thread, after the result has already been set. This means (I think, I haven't dug into this) that the exception just crashes the test running thread right at the end of the run - but the error is silently ignored, and the result can still be retrieved.

However, when AggregatingTestRunner is run sequentially (As the user may choose, or as #116 is currently causing all .nunit project users to see) then the exception is thrown on the main thread, with no handing, and the run is terminated immediately with no results (or only the results achieved so far.)

My opinion is that this situation should be handled the same way regardless of the user's choice of execution method. This PR fixes the early termination for running the AggregatingTestRunner sequentially, and catches the exceptions not being handled properly when running AggregatingTestRunner in parallel. This should standardise the handling of CannotUnloadAppDomain, regardless of the number of assemblies, or parallelisation choices.

WIP as I need to add tests, but it sounded like it would be sensible to put this up for feedback before putting much more work into it. 😄

(Edit: looks like I broke netstandard. Will revisit when I come back to this!)

…ame fashion as main test runners

CharliePoole · 2017-12-02T21:11:08Z

Based solely on the description, this makes sense to me. I'll be flying home tomorrow, so if the jet lag lets me I can look at code the next day.

ChrisMaddock · 2017-12-02T21:16:32Z

Thanks Charlie. No rush, I won't be back to this for a few days now. 🙂

ChrisMaddock · 2017-12-03T20:12:49Z

src/NUnitEngine/nunit.engine/Runners/AggregatingTestRunner.cs


            Runners.Clear();
+            throw new NUnitEngineException(_thrownExceptions);


Just come across this - D'oh! Of course, that should only throw when there are exceptions to be thrown - will fix this before merge.

So, throwing is the whole point of this change. I want to go back and question why we are throwing in the case of failure to unload. I mentioned this elsewhere (on the issue I think) and I wonder why we can't just report this as an error (or warning) associated with the individual assembly as we do in other cases. The difference here, of course, is that only the engine can detect the error as opposed to the framework. Worth discussing?

For the avoidance of doubt, to be clear of the things I'm aiming to do here:

Bring the AggregatingTestRunner inline with the other ITestEngineRunners.

Have the engine throw an exception on domain unload failure, caught by the runner.

The current issue is that the AggregatingTestRunner is throwing the exception mid-run, where as the other ITestEngineRunners would all throw the exception at the end of the test run, as the ITestEngineRunner is disposed, and once the full results have already been returned to the runner.

I think that design currently works. A few reasons:

I don't think we should silently swallow the exception, this exception indicates an unclean shut down in the user's code. By returning a distinct unload-failure-error-code such as in Return error code -5 when AppDomain fails to unload #320 - we still give the user the choice to ignore this if they wish to.

Re: associating with xml - by the time the domain is unloaded and the engine cleaned up, the results have already been returned to the runner. Would we want to alter this behaviour in all situations - e.g. with a GUI, unload wouldn't happen after each run, correct? I don't see the advantage of holding on to the xml, incase we wish to add an error that's not strictly related to the test results.

I feel like app domain unload belongs semantically as an engine exception, rather than a 'test result' in the xml. i.e. I could chose to run in the main domain, in which case I would get no exception.

I don't think that this exception should escape the runner - the current behaviour of handling this within the runner seems correct to me. The main issue in my eyes, is that the exception can currently cause a test run not to be completed, and results which have already ran to be lost. This shouldn't happen.

(The term 'runner' is overloaded in this conversation! I've edited to be clear when I mean ITestEngineRunner - and when I mean runner as in 'console runner'/GUI etc.)

I see your point about the separation of running from unloading. In that case, I suppose it's up to the runner to decide what to do with the exceptions - error, warn, etc. That would mean that unload exceptions need to be distinguishable from others of course.

How about an NUnitUnloadException which inherits from NUnitEngineExceptions, to not break any existing runners by throwning a new Exception type? 🙂

CharliePoole

Questions on the details plus the big one about whether we should really throw for failure to unload.

CharliePoole · 2017-12-05T14:35:17Z

src/NUnitEngine/nunit.engine.api/NUnitEngineException.cs

+            : base(AggregatedExceptionsMsg, new NUnitEngineException(AggregatedExceptionsMsg))
+        {
+            AggregatedExceptions = aggregatedExceptions;
+        }


Not clear what you are doing here. Seems like you end up with an NUnitEngineException with an InnerException that's also an NUnitEngineException, both having the same message. The outer also gets a collection of aggregated exceptions. What's the point of the InnerException?

I think I was maybe thinking of NUnitException in the framework, where I thought we'd always expect an inner exception? I'll get rid of it.

CharliePoole · 2017-12-05T14:35:56Z

src/NUnitEngine/nunit.engine.api/NUnitEngineException.cs


 #if !NETSTANDARD1_3
        /// <summary>
        /// Serialization constructor
        /// </summary>
        public NUnitEngineException(SerializationInfo info, StreamingContext context) : base(info, context) { }
 #endif
+
+        /// <summary>
+        /// Gets the Exception instance that caused the current exception.


Comment is wrong.

CharliePoole · 2017-12-05T14:41:21Z

src/NUnitEngine/nunit.engine/Runners/AggregatingTestRunner.cs


            Runners.Clear();
+            throw new NUnitEngineException(_thrownExceptions);


So, throwing is the whole point of this change. I want to go back and question why we are throwing in the case of failure to unload. I mentioned this elsewhere (on the issue I think) and I wonder why we can't just report this as an error (or warning) associated with the individual assembly as we do in other cases. The difference here, of course, is that only the engine can detect the error as opposed to the framework. Worth discussing?

…d in task execution

CharliePoole · 2017-12-12T09:09:37Z

Still "do not merge"?

ChrisMaddock · 2017-12-12T09:46:32Z

Was just waiting for CI to run last night, and didn't get a chance to come back to it. 😄 Ready for a second review.

ChrisMaddock · 2017-12-12T19:20:08Z

Thanks Charlie. 😄

Catch Exceptions from disposing runners in AggregatingTestRunner in s…

7dfed38

…ame fashion as main test runners

This was referenced Dec 2, 2017

Return error code -5 when AppDomain fails to unload #320

Merged

NUnit3 agent hangs after encountering an "CannotUnloadAppDomainException" #259

Closed

Added a continue-on-unload-error option #308

Closed

ChrisMaddock commented Dec 3, 2017

View reviewed changes

ChrisMaddock mentioned this pull request Dec 3, 2017

Use Environment Exit in Agent to ensure all threads are terminated #322

Merged

CharliePoole requested changes Dec 5, 2017

View reviewed changes

ChrisMaddock added 4 commits December 11, 2017 15:35

Add NUnitEngineUnloadException

22b27db

Should always attempt to dispose runner, even if exception encountere…

af489eb

…d in task execution

Also use NUnitUnloadException for non-aggregate unload failures

ca59351

Fix NetStandard engine

5fd644b

ChrisMaddock changed the title ~~WIP - AggregatingTestRunner to handle app domain unload failure in same fashion as single assembly runner~~ AggregatingTestRunner to handle app domain unload failure in same fashion as single assembly runner Dec 12, 2017

CharliePoole approved these changes Dec 12, 2017

View reviewed changes

ChrisMaddock merged commit f8bc46b into master Dec 12, 2017

ChrisMaddock deleted the issue-191b branch December 12, 2017 19:20

ChrisMaddock mentioned this pull request Dec 20, 2017

SocketException thrown during console run #255

Closed

This was referenced Apr 24, 2018

Exception encountered unloading AppDomain #191

Closed

NUnit-Runner 3.9.0-dev-03972 exit with error: "Exception encountered unloading application domain" #410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AggregatingTestRunner to handle app domain unload failure in same fashion as single assembly runner #321

AggregatingTestRunner to handle app domain unload failure in same fashion as single assembly runner #321

ChrisMaddock commented Dec 2, 2017 •

edited

CharliePoole commented Dec 2, 2017

ChrisMaddock commented Dec 2, 2017

ChrisMaddock Dec 3, 2017

CharliePoole Dec 5, 2017

ChrisMaddock Dec 5, 2017 •

edited

ChrisMaddock Dec 5, 2017

CharliePoole Dec 6, 2017

ChrisMaddock Dec 6, 2017

CharliePoole Dec 6, 2017

CharliePoole left a comment

CharliePoole Dec 5, 2017

ChrisMaddock Dec 5, 2017

CharliePoole Dec 5, 2017

CharliePoole Dec 5, 2017

CharliePoole commented Dec 12, 2017

ChrisMaddock commented Dec 12, 2017

ChrisMaddock commented Dec 12, 2017


		Runners.Clear();
		throw new NUnitEngineException(_thrownExceptions);

AggregatingTestRunner to handle app domain unload failure in same fashion as single assembly runner #321

AggregatingTestRunner to handle app domain unload failure in same fashion as single assembly runner #321

Conversation

ChrisMaddock commented Dec 2, 2017 • edited

CharliePoole commented Dec 2, 2017

ChrisMaddock commented Dec 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisMaddock Dec 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharliePoole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharliePoole commented Dec 12, 2017

ChrisMaddock commented Dec 12, 2017

ChrisMaddock commented Dec 12, 2017

ChrisMaddock commented Dec 2, 2017 •

edited

ChrisMaddock Dec 5, 2017 •

edited