Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
Dec 12, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7dfed38
Catch Exceptions from disposing runners in AggregatingTestRunner in s…
ChrisMaddock Dec 2, 2017
22b27db
Add NUnitEngineUnloadException
ChrisMaddock Dec 11, 2017
af489eb
Should always attempt to dispose runner, even if exception encountere…
ChrisMaddock Dec 11, 2017
ca59351
Also use NUnitUnloadException for non-aggregate unload failures
ChrisMaddock Dec 11, 2017
5fd644b
Fix NetStandard engine
ChrisMaddock Dec 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 25 additions & 2 deletions src/NUnitEngine/nunit.engine.api/NUnitEngineException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
// ***********************************************************************

using System;
using System.Collections.Generic;
#if !NETSTANDARD1_3
using System.Runtime.Serialization;
#endif
Expand All @@ -38,23 +39,45 @@ namespace NUnit.Engine
#endif
public class NUnitEngineException : Exception
{
private const string AggregatedExceptionsMsg =
"Multiple exceptions encountered. Retrieve AggregatedExceptions property for more information";

/// <summary>
/// Construct with a message
/// </summary>
public NUnitEngineException(string message) : base(message) { }
public NUnitEngineException(string message) : base(message)
{
}

/// <summary>
/// Construct with a message and inner exception
/// </summary>
/// <param name="message"></param>
/// <param name="innerException"></param>
public NUnitEngineException(string message, Exception innerException) : base(message, innerException) { }
public NUnitEngineException(string message, Exception innerException) : base(message, innerException)
{
}

/// <summary>
/// Construct with a message and collection of inner exceptions
/// </summary>
public NUnitEngineException(ICollection<Exception> aggregatedExceptions)
: base(AggregatedExceptionsMsg, new NUnitEngineException(AggregatedExceptionsMsg))
{
AggregatedExceptions = aggregatedExceptions;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is wrong.

/// </summary>
public ICollection<Exception> AggregatedExceptions { get; }

}
}
14 changes: 12 additions & 2 deletions src/NUnitEngine/nunit.engine/Internal/ExceptionHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
using System.Collections.Generic;
using System.Reflection;
using System.Text;
using NUnit.Engine;

namespace NUnit.Common
{
Expand Down Expand Up @@ -100,9 +101,18 @@ private static List<Exception> FlattenExceptionHierarchy(Exception exception)
{
var result = new List<Exception>();

if (exception is ReflectionTypeLoadException)
var engineException = exception as NUnitEngineException;
if (engineException?.AggregatedExceptions != null)
{
result.AddRange(engineException.AggregatedExceptions);

foreach (var aggregatedException in engineException.AggregatedExceptions)
result.AddRange(FlattenExceptionHierarchy(aggregatedException));
}

var reflectionException = exception as ReflectionTypeLoadException;
if (reflectionException != null)
{
var reflectionException = exception as ReflectionTypeLoadException;
result.AddRange(reflectionException.LoaderExceptions);

foreach (var innerException in reflectionException.LoaderExceptions)
Expand Down
53 changes: 47 additions & 6 deletions src/NUnitEngine/nunit.engine/Runners/AggregatingTestRunner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@
// WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
// ***********************************************************************

using System;
using System.Collections.Generic;
using NUnit.Common;
using NUnit.Engine.Internal;

namespace NUnit.Engine.Runners
Expand All @@ -39,6 +41,11 @@ public class AggregatingTestRunner : AbstractTestRunner
// of writing this comment) be either TestDomainRunners or ProcessRunners.
private List<ITestEngineRunner> _runners;

// Exceptions from individual runners are caught and rethrown
// on AggregatingTestRunner disposal, to allow TestResults to be
// written and execution of other runners to continue.
private readonly List<Exception> _thrownExceptions = new List<Exception>();

// Public for testing purposes
public virtual int LevelOfParallelism
{
Expand Down Expand Up @@ -115,7 +122,16 @@ protected override TestEngineResult LoadPackage()
public override void UnloadPackage()
{
foreach (ITestEngineRunner runner in Runners)
runner.Unload();
{
try
{
runner.Unload();
}
catch (Exception e)
{
_thrownExceptions.Add(e);
}
}
}

/// <summary>
Expand Down Expand Up @@ -169,8 +185,9 @@ private void RunTestsSequentially(ITestEventListener listener, TestFilter filter
{
foreach (ITestEngineRunner runner in Runners)
{
results.Add(runner.Run(listener, filter));
if (disposeRunners) runner.Dispose();
var task = new TestExecutionTask(runner, listener, filter, disposeRunners);
task.Execute();
LogResultsFromTask(task, results, _thrownExceptions);
}
}

Expand All @@ -191,7 +208,7 @@ private void RunTestsInParallel(ITestEventListener listener, TestFilter filter,
workerPool.WaitAll();

foreach (var task in tasks)
results.Add(task.Result());
LogResultsFromTask(task, results, _thrownExceptions);
}
#endif

Expand All @@ -210,16 +227,40 @@ protected override void Dispose(bool disposing)
base.Dispose(disposing);

foreach (var runner in Runners)
runner.Dispose();
{
try
{
runner.Dispose();
}
catch (NUnitEngineException e)
{
_thrownExceptions.Add(e);
}
}

Runners.Clear();
throw new NUnitEngineException(_thrownExceptions);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Bring the AggregatingTestRunner inline with the other ITestEngineRunners.
  2. 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:

  1. 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.
  2. 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.
  3. 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it!

}

#endregion
#endregion

protected virtual ITestEngineRunner CreateRunner(TestPackage package)
{
return TestRunnerFactory.MakeTestRunner(package);
}

private static void LogResultsFromTask(TestExecutionTask task, List<TestEngineResult> results, List<Exception> thrownExceptions)
{
var result = task.Result();
if (result != null)
{
results.Add(result);
}

if (task.ThrownException != null)
{
thrownExceptions.Add(task.ThrownException);
}
}
}
}
32 changes: 29 additions & 3 deletions src/NUnitEngine/nunit.engine/Runners/TestExecutionTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
// WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
// ***********************************************************************

using System;

namespace NUnit.Engine.Runners
{
public class TestExecutionTask : ITestExecutionTask
Expand All @@ -30,6 +32,8 @@ public class TestExecutionTask : ITestExecutionTask
private readonly TestFilter _filter;
private volatile TestEngineResult _result;
private readonly bool _disposeRunner;
private bool _hasExecuted = false;
private Exception _thrownException;

public TestExecutionTask(ITestEngineRunner runner, ITestEventListener listener, TestFilter filter, bool disposeRunner)
{
Expand All @@ -41,14 +45,36 @@ public TestExecutionTask(ITestEngineRunner runner, ITestEventListener listener,

public void Execute()
{
_result = _runner.Run(_listener, _filter);
if (_disposeRunner)
_runner.Dispose();
_hasExecuted = true;

try
{
_result = _runner.Run(_listener, _filter);
if (_disposeRunner)
_runner.Dispose();
}
catch (Exception e)
{
_thrownException = e;
}
}

public TestEngineResult Result()
{
Guard.OperationValid(_hasExecuted, "Can not access result until task has been executed");
return _result;
}

/// <summary>
/// Stored exception thrown during task execution
/// </summary>
public Exception ThrownException
{
get
{
Guard.OperationValid(_hasExecuted, "Can not access thrown exceptions until task has been executed");
return _thrownException;
}
}
}
}