-
Notifications
You must be signed in to change notification settings - Fork 150
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.
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
Return error code -5 when AppDomain fails to unload #320
Conversation
|
||
namespace NUnit.ConsoleRunner.Tests | ||
{ | ||
class ConsoleRunnerTests | ||
{ | ||
[Test, Ignore("Needs to be rewritten")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated, but I wanted to add NSubstitute for my tests, so put this one back while I was here.
@@ -8,13 +8,14 @@ | |||
<AppDesignerFolder>Properties</AppDesignerFolder> | |||
<RootNamespace>NUnit.ConsoleRunner.Tests</RootNamespace> | |||
<AssemblyName>nunit3-console.tests</AssemblyName> | |||
<TargetFrameworkVersion>v2.0</TargetFrameworkVersion> | |||
<TargetFrameworkVersion>v3.5</TargetFrameworkVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required for NSubstitute. nunit.engine.tests is already the same way - a 3.5 test dll referencing a 2.0 product.
@@ -227,11 +228,9 @@ private int RunTests(TestPackage package, TestFilter filter) | |||
RestoreErrorOutput(); | |||
} | |||
|
|||
var writer = new ColorConsoleWriter(!_options.NoColor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anyone know why we recreated the ConsoleWriter here? This meant my mock wasn't being used - and the two halves of console output were coming through two different writers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that output from the test itself, which is handled by the first writer, may be redirected. This code goes back to when only a few people worked on NUnit but should have been explained in a comment anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I wonder if it's possible to re-redirect the original ConsoleWriter instead? That would allow other writers to be passed in to the class. It feels a little odd that the class takes a Writer in the constructor, but then doesn't use that writer to write all the output.
I'll have another look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see it all very well browsing on the phone but I do remember that adding color made it more complicated and o had to work to ensure that two writers still made one smooth output flow. It may have been changed as well since then.
There are two ways to redirect, --output parameter, which affects only test output and not the final report, and command-line she'll redirection, which affects anything that might go to the console. Taken together, that's four possibilities.
_serviceIndex.Remove(typeof(T)); | ||
AddService(newService); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For test purposes. I'd have ideally made this internal and used InternalsVisibleTo - but as nunit.engine is signed, it wasn't possible without additionally signing nunit-console, and I didn't want to start that cycle off!
Hmm - I've just moved on to look at the My thoughts - it isn't always possible to return a CannotUnloadAppDomainException - so it may be better to make that exit code more general. |
Returning an error code when one assembly encounters a problem is a little problematic. We moved away from this so as to give the error on the assembly result in many other cases, as when the assembly can't be loaded. |
@CharliePoole - let's discuss this in more detail on #321. 😄 |
f2a0099
to
e3a8ac5
Compare
e3a8ac5
to
6bd301b
Compare
6bd301b
to
f282e35
Compare
This is ready for re-review. The Console will now return -5 in cases of app domain unload failure, meaning unload failure will return a non-zero code, but is distingushable from other errors, so can be chosen to be ignored, if they user wishes. I removed the test in the end, with the TextWriter being recreated and other remoting issues, it just seemed too awkward to test at this level. I guess this is another one for our integration test wishlist. 😞 |
@@ -238,12 +244,20 @@ private int RunTests(TestPackage package, TestFilter filter) | |||
{ | |||
var outputPath = Path.Combine(_workDirectory, spec.OutputPath); | |||
GetResultWriter(spec).WriteResultFile(result, outputPath); | |||
_outWriter.WriteLine("Results ({0}) saved as {1}", spec.Format, spec.OutputPath); | |||
writer.WriteLine("Results ({0}) saved as {1}", spec.Format, spec.OutputPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not related to the main PR, but believe this should use the 'new' writer instead. See discussion here: #320 (comment) - came across this after my own confusion!
writer.WriteLine(ColorStyle.Warning, Environment.NewLine + ExceptionHelper.BuildMessage(engineException)); | ||
{ | ||
writer.WriteLine(ColorStyle.Error, Environment.NewLine + ExceptionHelper.BuildMessage(engineException)); | ||
return ConsoleRunner.UNEXPECTED_ERROR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set this up as a way for the engine to issue warnings and used it for a while. Since I stopped working on the engine I'm not sure if it is still used that way, but if it is, you would break the behavior here. It may be that you have replaced the last such usage and could then remove this code. In retrospect I should have used a different exception to make it clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought process was we now separate out the unload exceptions to a separate path. Therefore, any other crash at this point is 'unexpected'.
That said, this still returns the same -100 error code at this point - the only actual change is the colour of the text from Yellow -> Red - unless there's something else I'm missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks generally good. I'll leave it to you to decide if my inline comment requires any change.
One question... if running multiple assemblies and one of them fails to unload, do we get to see which one caused the problem?
Not as a 'direct' feature, but you will see the app domain which crashed, and I believe are app domains are named by the assembly they're running, right? 🤔 Left a reply above just to be sure we're thinking along the same lines before merging. |
The domain name will point to the proper assembly a lot of the time, since it uses the "package name". It will not directly indicate the assembly when running multiple assemblies in the same domain. In that case, you'll see either the name of the project file that contains the assemblies or the name of the first assembly specified on the command-line. My general sense is that this is not something people do a lot of the time. I think this could be refined so that the unload exception always included the name of the assembly, but I'll leave it up to you whether that should be part of this PR or not. |
I agree that would be nice - but I want to get this into the next release, and don’t really have the time to revisit this before Robs planned date next weekend. Can we call that a separate enhancement? 🙂 |
This is the first of a few PRs to improve the experience when the appdomain fails to unload. I was hoping to get further that this - unfortunately mocking out the console took a few more hours than expected. 😬
This implements part 4 of the solution proposed here i.e. returning
-5
when the app domain fails to unload. Note this is a 'breaking change' - previously we issued a warning in the console, and then silently exited with code zero. I'm of the opinion that we shouldn't exit silently when these errors come up. With a specific error code, user's can chosoe to accept either 0 or -5 as a pass in their CI if they do wish to ignore the problem, but it will never silently slip by without the user taking action.The reason there's been a few people cropping up over this issue is that, when using a
.nunit
project (i.e. everyone on TeamCity), behaviour seems to be different, and the process does crash mid-result if the appdomain fails to unload. I imagine that could be related to #116 - and will be looking at that as a separate issue.Other than that, the remaining issue to do here is #111 - but I'll put these in as separate PRs, to be easier to review!
Also fixes #256